Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement set_attribute & set_class_attribute nonfungibles trait functions for Uniques pallet #9206

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion frame/uniques/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! Various pieces of common functionality.

use super::*;
use frame_support::{ensure, traits::Get};
use frame_support::{ensure, traits::Get, BoundedVec};
use sp_runtime::{DispatchResult, DispatchError};

impl<T: Config<I>, I: 'static> Pallet<T, I> {
Expand Down Expand Up @@ -112,4 +112,47 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Self::deposit_event(Event::Burned(class, instance, owner));
Ok(())
}

pub(super) fn do_set_attribute(
class: T::ClassId,
maybe_instance: Option<T::InstanceId>,
maybe_check_owner: &Option<T::AccountId>,
key: BoundedVec<u8, T::KeyLimit>,
value: BoundedVec<u8, T::ValueLimit>,
with_details: impl FnOnce(&ClassDetailsFor<T, I>) -> DispatchResult,
) -> DispatchResult {
let mut class_details = Class::<T, I>::get(&class).ok_or(Error::<T, I>::Unknown)?;
with_details(&class_details)?;

let maybe_is_frozen = match maybe_instance {
None => ClassMetadataOf::<T, I>::get(class).map(|v| v.is_frozen),
Some(instance) => InstanceMetadataOf::<T, I>::get(class, instance).map(|v| v.is_frozen),
};
ensure!(!maybe_is_frozen.unwrap_or(false), Error::<T, I>::Frozen);

let attribute = Attribute::<T, I>::get((class, maybe_instance, &key));
if attribute.is_none() {
class_details.attributes.saturating_inc();
}
let old_deposit = attribute.map_or(Zero::zero(), |m| m.1);
class_details.total_deposit.saturating_reduce(old_deposit);
let mut deposit = Zero::zero();
if !class_details.free_holding && maybe_check_owner.is_some() {
deposit = T::DepositPerByte::get()
.saturating_mul(((key.len() + value.len()) as u32).into())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.saturating_mul(((key.len() + value.len()) as u32).into())
.saturating_mul((key.len().saturating_add(value.len()) as u32).into())

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion, done here 6fabf25

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one i think is okay, but would want you to look closer before actually making this change

and then a test

.saturating_add(T::AttributeDepositBase::get());
}
class_details.total_deposit.saturating_accrue(deposit);
if deposit > old_deposit {
T::Currency::reserve(&class_details.owner, deposit - old_deposit)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
T::Currency::reserve(&class_details.owner, deposit - old_deposit)?;
T::Currency::reserve(&class_details.owner, deposit.saturating_sub(old_deposit))?;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion, done here 6fabf25

Copy link
Member

@shawntabrizi shawntabrizi Aug 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is NOT needed, and actually probably some additional overhead.

Note the check just 1 line above: deposit > old_deposit

This subtraction can never panic

please undo

} else if deposit < old_deposit {
T::Currency::unreserve(&class_details.owner, old_deposit - deposit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
T::Currency::unreserve(&class_details.owner, old_deposit - deposit);
T::Currency::unreserve(&class_details.owner, old_deposit.saturating_sub(deposit));

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion, done here 6fabf25

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo

}

Attribute::<T, I>::insert((&class, maybe_instance, &key), (&value, deposit));
Class::<T, I>::insert(class, &class_details);

Self::deposit_event(Event::AttributeSet(class, maybe_instance, key, value));
Ok(())
}
}
39 changes: 38 additions & 1 deletion frame/uniques/src/impl_nonfungibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! Implementations for `nonfungibles` traits.

use super::*;
use sp_std::convert::TryFrom;
use sp_std::convert::{TryFrom, TryInto};
use frame_support::traits::tokens::nonfungibles::{Inspect, Mutate, Transfer};
use frame_support::BoundedSlice;
use sp_runtime::DispatchResult;
Expand Down Expand Up @@ -95,6 +95,43 @@ impl<T: Config<I>, I: 'static> Mutate<<T as SystemConfig>::AccountId> for Pallet
fn burn_from(class: &Self::ClassId, instance: &Self::InstanceId) -> DispatchResult {
Self::do_burn(class.clone(), instance.clone(), |_, _| Ok(()))
}

fn set_attribute(
class: &Self::ClassId,
instance: &Self::InstanceId,
key: &[u8],
value: &[u8],
) -> DispatchResult {
let bounded_key = key.to_vec().try_into().map_err(|_| Error::<T, I>::KeyUpperBoundExceeded)?;
let bounded_value = value.to_vec().try_into().map_err(|_| Error::<T, I>::ValueUpperBoundExceeded)?;

Self::do_set_attribute(
class.clone(),
Some(instance.clone()),
&None,
bounded_key,
bounded_value,
|_| Ok(()),
)
}
apopiak marked this conversation as resolved.
Show resolved Hide resolved

fn set_class_attribute(
class: &Self::ClassId,
key: &[u8],
value: &[u8],
) -> DispatchResult {
let bounded_key = key.to_vec().try_into().map_err(|_| Error::<T, I>::KeyUpperBoundExceeded)?;
let bounded_value = value.to_vec().try_into().map_err(|_| Error::<T, I>::ValueUpperBoundExceeded)?;

Self::do_set_attribute(
class.clone(),
None,
&None,
bounded_key,
bounded_value,
|_| Ok(()),
)
}
}

impl<T: Config<I>, I: 'static> Transfer<T::AccountId> for Pallet<T, I> {
Expand Down
78 changes: 32 additions & 46 deletions frame/uniques/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,10 @@ pub mod pallet {
NoDelegate,
/// No approval exists that would allow the transfer.
Unapproved,
/// Attribute key upper bound exceeded.
KeyUpperBoundExceeded,
/// Attribute value upper bound exceeded.
ValueUpperBoundExceeded,
}

#[pallet::hooks]
Expand Down Expand Up @@ -927,52 +931,34 @@ pub mod pallet {
///
/// Weight: `O(1)`
#[pallet::weight(T::WeightInfo::set_attribute())]
pub fn set_attribute(
origin: OriginFor<T>,
#[pallet::compact] class: T::ClassId,
maybe_instance: Option<T::InstanceId>,
key: BoundedVec<u8, T::KeyLimit>,
value: BoundedVec<u8, T::ValueLimit>,
) -> DispatchResult {
let maybe_check_owner = T::ForceOrigin::try_origin(origin)
.map(|_| None)
.or_else(|origin| ensure_signed(origin).map(Some))?;

let mut class_details = Class::<T, I>::get(&class).ok_or(Error::<T, I>::Unknown)?;
if let Some(check_owner) = &maybe_check_owner {
ensure!(check_owner == &class_details.owner, Error::<T, I>::NoPermission);
}
let maybe_is_frozen = match maybe_instance {
None => ClassMetadataOf::<T, I>::get(class).map(|v| v.is_frozen),
Some(instance) =>
InstanceMetadataOf::<T, I>::get(class, instance).map(|v| v.is_frozen),
};
ensure!(!maybe_is_frozen.unwrap_or(false), Error::<T, I>::Frozen);

let attribute = Attribute::<T, I>::get((class, maybe_instance, &key));
if attribute.is_none() {
class_details.attributes.saturating_inc();
}
let old_deposit = attribute.map_or(Zero::zero(), |m| m.1);
class_details.total_deposit.saturating_reduce(old_deposit);
let mut deposit = Zero::zero();
if !class_details.free_holding && maybe_check_owner.is_some() {
deposit = T::DepositPerByte::get()
.saturating_mul(((key.len() + value.len()) as u32).into())
.saturating_add(T::AttributeDepositBase::get());
}
class_details.total_deposit.saturating_accrue(deposit);
if deposit > old_deposit {
T::Currency::reserve(&class_details.owner, deposit - old_deposit)?;
} else if deposit < old_deposit {
T::Currency::unreserve(&class_details.owner, old_deposit - deposit);
}

Attribute::<T, I>::insert((&class, maybe_instance, &key), (&value, deposit));
Class::<T, I>::insert(class, &class_details);
Self::deposit_event(Event::AttributeSet(class, maybe_instance, key, value));
Ok(())
}
pub fn set_attribute(
origin: OriginFor<T>,
#[pallet::compact] class: T::ClassId,
maybe_instance: Option<T::InstanceId>,
key: BoundedVec<u8, T::KeyLimit>,
value: BoundedVec<u8, T::ValueLimit>,
) -> DispatchResult {
let maybe_check_owner = T::ForceOrigin::try_origin(origin)
.map(|_| None)
.or_else(|origin| ensure_signed(origin).map(Some))?;

Self::do_set_attribute(
class,
maybe_instance,
&maybe_check_owner,
key,
value,
|class_details| {
if let Some(check_owner) = &maybe_check_owner {
ensure!(
check_owner == &class_details.owner,
Error::<T, I>::NoPermission
);
}
Ok(())
},
)
}

/// Set an attribute for an asset class or instance.
///
Expand Down