Skip to content

Commit

Permalink
Remove the Copy bound on CollectionId in the uniques pallet (pari…
Browse files Browse the repository at this point in the history 
…tytech#14111)

* Remove the `Copy` bound on `CollectionId` in the uniques pallet

* Also add `clone`s in benchmarks
  • Loading branch information
@koute @nathanwhit
koute authored and nathanwhit committed Jul 19, 2023
1 parent d64f24f commit 98c24be
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 85 deletions.
106 changes: 53 additions & 53 deletions frame/uniques/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ fn create_collection<T: Config<I>, I: 'static>(
T::Currency::make_free_balance_be(&caller, DepositBalanceOf::<T, I>::max_value());
assert!(Uniques::<T, I>::force_create(
SystemOrigin::Root.into(),
collection,
collection.clone(),
caller_lookup.clone(),
false,
)
Expand Down Expand Up @@ -173,26 +173,26 @@ benchmarks_instance_pallet! {
for i in 0..a {
add_item_attribute::<T, I>(T::Helper::item(i as u16));
}
let witness = Collection::<T, I>::get(collection).unwrap().destroy_witness();
}: _(SystemOrigin::Signed(caller), collection, witness)
let witness = Collection::<T, I>::get(collection.clone()).unwrap().destroy_witness();
}: _(SystemOrigin::Signed(caller), collection.clone(), witness)
verify {
assert_last_event::<T, I>(Event::Destroyed { collection }.into());
assert_last_event::<T, I>(Event::Destroyed { collection: collection.clone() }.into());
}

mint {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let item = T::Helper::item(0);
}: _(SystemOrigin::Signed(caller.clone()), collection, item, caller_lookup)
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), item, caller_lookup)
verify {
assert_last_event::<T, I>(Event::Issued { collection, item, owner: caller }.into());
assert_last_event::<T, I>(Event::Issued { collection: collection.clone(), item, owner: caller }.into());
}

burn {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
}: _(SystemOrigin::Signed(caller.clone()), collection, item, Some(caller_lookup))
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), item, Some(caller_lookup))
verify {
assert_last_event::<T, I>(Event::Burned { collection, item, owner: caller }.into());
assert_last_event::<T, I>(Event::Burned { collection: collection.clone(), item, owner: caller }.into());
}

transfer {
Expand All @@ -201,9 +201,9 @@ benchmarks_instance_pallet! {

let target: T::AccountId = account("target", 0, SEED);
let target_lookup = T::Lookup::unlookup(target.clone());
}: _(SystemOrigin::Signed(caller.clone()), collection, item, target_lookup)
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), item, target_lookup)
verify {
assert_last_event::<T, I>(Event::Transferred { collection, item, from: caller, to: target }.into());
assert_last_event::<T, I>(Event::Transferred { collection: collection.clone(), item, from: caller, to: target }.into());
}

redeposit {
Expand All @@ -212,17 +212,17 @@ benchmarks_instance_pallet! {
let items = (0..i).map(|x| mint_item::<T, I>(x as u16).0).collect::<Vec<_>>();
Uniques::<T, I>::force_item_status(
SystemOrigin::Root.into(),
collection,
collection.clone(),
caller_lookup.clone(),
caller_lookup.clone(),
caller_lookup.clone(),
caller_lookup,
true,
false,
)?;
}: _(SystemOrigin::Signed(caller.clone()), collection, items.clone())
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), items.clone())
verify {
assert_last_event::<T, I>(Event::Redeposited { collection, successful_items: items }.into());
assert_last_event::<T, I>(Event::Redeposited { collection: collection.clone(), successful_items: items }.into());
}

freeze {
Expand All @@ -238,28 +238,28 @@ benchmarks_instance_pallet! {
let (item, ..) = mint_item::<T, I>(0);
Uniques::<T, I>::freeze(
SystemOrigin::Signed(caller.clone()).into(),
collection,
collection.clone(),
item,
)?;
}: _(SystemOrigin::Signed(caller.clone()), collection, item)
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), item)
verify {
assert_last_event::<T, I>(Event::Thawed { collection, item }.into());
assert_last_event::<T, I>(Event::Thawed { collection: collection.clone(), item }.into());
}

freeze_collection {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
}: _(SystemOrigin::Signed(caller.clone()), collection)
}: _(SystemOrigin::Signed(caller.clone()), collection.clone())
verify {
assert_last_event::<T, I>(Event::CollectionFrozen { collection }.into());
assert_last_event::<T, I>(Event::CollectionFrozen { collection: collection.clone() }.into());
}

thaw_collection {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let origin = SystemOrigin::Signed(caller.clone()).into();
Uniques::<T, I>::freeze_collection(origin, collection)?;
}: _(SystemOrigin::Signed(caller.clone()), collection)
Uniques::<T, I>::freeze_collection(origin, collection.clone())?;
}: _(SystemOrigin::Signed(caller.clone()), collection.clone())
verify {
assert_last_event::<T, I>(Event::CollectionThawed { collection }.into());
assert_last_event::<T, I>(Event::CollectionThawed { collection: collection.clone() }.into());
}

transfer_ownership {
Expand All @@ -268,21 +268,21 @@ benchmarks_instance_pallet! {
let target_lookup = T::Lookup::unlookup(target.clone());
T::Currency::make_free_balance_be(&target, T::Currency::minimum_balance());
let origin = SystemOrigin::Signed(target.clone()).into();
Uniques::<T, I>::set_accept_ownership(origin, Some(collection))?;
}: _(SystemOrigin::Signed(caller), collection, target_lookup)
Uniques::<T, I>::set_accept_ownership(origin, Some(collection.clone()))?;
}: _(SystemOrigin::Signed(caller), collection.clone(), target_lookup)
verify {
assert_last_event::<T, I>(Event::OwnerChanged { collection, new_owner: target }.into());
assert_last_event::<T, I>(Event::OwnerChanged { collection: collection.clone(), new_owner: target }.into());
}

set_team {
let (collection, caller, _) = create_collection::<T, I>();
let target0 = T::Lookup::unlookup(account("target", 0, SEED));
let target1 = T::Lookup::unlookup(account("target", 1, SEED));
let target2 = T::Lookup::unlookup(account("target", 2, SEED));
}: _(SystemOrigin::Signed(caller), collection, target0, target1, target2)
}: _(SystemOrigin::Signed(caller), collection.clone(), target0, target1, target2)
verify {
assert_last_event::<T, I>(Event::TeamChanged{
collection,
collection: collection.clone(),
issuer: account("target", 0, SEED),
admin: account("target", 1, SEED),
freezer: account("target", 2, SEED),
Expand All @@ -294,7 +294,7 @@ benchmarks_instance_pallet! {
let origin =
T::ForceOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let call = Call::<T, I>::force_item_status {
collection,
collection: collection.clone(),
owner: caller_lookup.clone(),
issuer: caller_lookup.clone(),
admin: caller_lookup.clone(),
Expand All @@ -304,7 +304,7 @@ benchmarks_instance_pallet! {
};
}: { call.dispatch_bypass_filter(origin)? }
verify {
assert_last_event::<T, I>(Event::ItemStatusChanged { collection }.into());
assert_last_event::<T, I>(Event::ItemStatusChanged { collection: collection.clone() }.into());
}

set_attribute {
Expand All @@ -314,65 +314,65 @@ benchmarks_instance_pallet! {
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
add_item_metadata::<T, I>(item);
}: _(SystemOrigin::Signed(caller), collection, Some(item), key.clone(), value.clone())
}: _(SystemOrigin::Signed(caller), collection.clone(), Some(item), key.clone(), value.clone())
verify {
assert_last_event::<T, I>(Event::AttributeSet { collection, maybe_item: Some(item), key, value }.into());
assert_last_event::<T, I>(Event::AttributeSet { collection: collection.clone(), maybe_item: Some(item), key, value }.into());
}

clear_attribute {
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
add_item_metadata::<T, I>(item);
let (key, ..) = add_item_attribute::<T, I>(item);
}: _(SystemOrigin::Signed(caller), collection, Some(item), key.clone())
}: _(SystemOrigin::Signed(caller), collection.clone(), Some(item), key.clone())
verify {
assert_last_event::<T, I>(Event::AttributeCleared { collection, maybe_item: Some(item), key }.into());
assert_last_event::<T, I>(Event::AttributeCleared { collection: collection.clone(), maybe_item: Some(item), key }.into());
}

set_metadata {
let data: BoundedVec<_, _> = vec![0u8; T::StringLimit::get() as usize].try_into().unwrap();

let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
}: _(SystemOrigin::Signed(caller), collection, item, data.clone(), false)
}: _(SystemOrigin::Signed(caller), collection.clone(), item, data.clone(), false)
verify {
assert_last_event::<T, I>(Event::MetadataSet { collection, item, data, is_frozen: false }.into());
assert_last_event::<T, I>(Event::MetadataSet { collection: collection.clone(), item, data, is_frozen: false }.into());
}

clear_metadata {
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
add_item_metadata::<T, I>(item);
}: _(SystemOrigin::Signed(caller), collection, item)
}: _(SystemOrigin::Signed(caller), collection.clone(), item)
verify {
assert_last_event::<T, I>(Event::MetadataCleared { collection, item }.into());
assert_last_event::<T, I>(Event::MetadataCleared { collection: collection.clone(), item }.into());
}

set_collection_metadata {
let data: BoundedVec<_, _> = vec![0u8; T::StringLimit::get() as usize].try_into().unwrap();

let (collection, caller, _) = create_collection::<T, I>();
}: _(SystemOrigin::Signed(caller), collection, data.clone(), false)
}: _(SystemOrigin::Signed(caller), collection.clone(), data.clone(), false)
verify {
assert_last_event::<T, I>(Event::CollectionMetadataSet { collection, data, is_frozen: false }.into());
assert_last_event::<T, I>(Event::CollectionMetadataSet { collection: collection.clone(), data, is_frozen: false }.into());
}

clear_collection_metadata {
let (collection, caller, _) = create_collection::<T, I>();
add_collection_metadata::<T, I>();
}: _(SystemOrigin::Signed(caller), collection)
}: _(SystemOrigin::Signed(caller), collection.clone())
verify {
assert_last_event::<T, I>(Event::CollectionMetadataCleared { collection }.into());
assert_last_event::<T, I>(Event::CollectionMetadataCleared { collection: collection.clone() }.into());
}

approve_transfer {
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
let delegate: T::AccountId = account("delegate", 0, SEED);
let delegate_lookup = T::Lookup::unlookup(delegate.clone());
}: _(SystemOrigin::Signed(caller.clone()), collection, item, delegate_lookup)
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), item, delegate_lookup)
verify {
assert_last_event::<T, I>(Event::ApprovedTransfer { collection, item, owner: caller, delegate }.into());
assert_last_event::<T, I>(Event::ApprovedTransfer { collection: collection.clone(), item, owner: caller, delegate }.into());
}

cancel_approval {
Expand All @@ -381,17 +381,17 @@ benchmarks_instance_pallet! {
let delegate: T::AccountId = account("delegate", 0, SEED);
let delegate_lookup = T::Lookup::unlookup(delegate.clone());
let origin = SystemOrigin::Signed(caller.clone()).into();
Uniques::<T, I>::approve_transfer(origin, collection, item, delegate_lookup.clone())?;
}: _(SystemOrigin::Signed(caller.clone()), collection, item, Some(delegate_lookup))
Uniques::<T, I>::approve_transfer(origin, collection.clone(), item, delegate_lookup.clone())?;
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), item, Some(delegate_lookup))
verify {
assert_last_event::<T, I>(Event::ApprovalCancelled { collection, item, owner: caller, delegate }.into());
assert_last_event::<T, I>(Event::ApprovalCancelled { collection: collection.clone(), item, owner: caller, delegate }.into());
}

set_accept_ownership {
let caller: T::AccountId = whitelisted_caller();
T::Currency::make_free_balance_be(&caller, DepositBalanceOf::<T, I>::max_value());
let collection = T::Helper::collection(0);
}: _(SystemOrigin::Signed(caller.clone()), Some(collection))
}: _(SystemOrigin::Signed(caller.clone()), Some(collection.clone()))
verify {
assert_last_event::<T, I>(Event::OwnershipAcceptanceChanged {
who: caller,
Expand All @@ -401,10 +401,10 @@ benchmarks_instance_pallet! {

set_collection_max_supply {
let (collection, caller, _) = create_collection::<T, I>();
}: _(SystemOrigin::Signed(caller.clone()), collection, u32::MAX)
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), u32::MAX)
verify {
assert_last_event::<T, I>(Event::CollectionMaxSupplySet {
collection,
collection: collection.clone(),
max_supply: u32::MAX,
}.into());
}
Expand All @@ -415,10 +415,10 @@ benchmarks_instance_pallet! {
let delegate: T::AccountId = account("delegate", 0, SEED);
let delegate_lookup = T::Lookup::unlookup(delegate.clone());
let price = ItemPrice::<T, I>::from(100u32);
}: _(SystemOrigin::Signed(caller.clone()), collection, item, Some(price), Some(delegate_lookup))
}: _(SystemOrigin::Signed(caller.clone()), collection.clone(), item, Some(price), Some(delegate_lookup))
verify {
assert_last_event::<T, I>(Event::ItemPriceSet {
collection,
collection: collection.clone(),
item,
price,
whitelisted_buyer: Some(delegate),
Expand All @@ -432,12 +432,12 @@ benchmarks_instance_pallet! {
let buyer_lookup = T::Lookup::unlookup(buyer.clone());
let price = ItemPrice::<T, I>::from(0u32);
let origin = SystemOrigin::Signed(seller.clone()).into();
Uniques::<T, I>::set_price(origin, collection, item, Some(price.clone()), Some(buyer_lookup))?;
Uniques::<T, I>::set_price(origin, collection.clone(), item, Some(price.clone()), Some(buyer_lookup))?;
T::Currency::make_free_balance_be(&buyer, DepositBalanceOf::<T, I>::max_value());
}: _(SystemOrigin::Signed(buyer.clone()), collection, item, price.clone())
}: _(SystemOrigin::Signed(buyer.clone()), collection.clone(), item, price.clone())
verify {
assert_last_event::<T, I>(Event::ItemBought {
collection,
collection: collection.clone(),
item,
price,
seller,
Expand Down
17 changes: 10 additions & 7 deletions frame/uniques/src/functions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
let collection_details =
Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
ensure!(!collection_details.is_frozen, Error::<T, I>::Frozen);
ensure!(!T::Locker::is_locked(collection, item), Error::<T, I>::Locked);
ensure!(!T::Locker::is_locked(collection.clone(), item), Error::<T, I>::Locked);

let mut details =
Item::<T, I>::get(&collection, &item).ok_or(Error::<T, I>::UnknownCollection)?;
Expand Down Expand Up @@ -74,12 +74,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
free_holding: bool,
event: Event<T, I>,
) -> DispatchResult {
ensure!(!Collection::<T, I>::contains_key(collection), Error::<T, I>::InUse);
ensure!(!Collection::<T, I>::contains_key(collection.clone()), Error::<T, I>::InUse);

T::Currency::reserve(&owner, deposit)?;

Collection::<T, I>::insert(
collection,
collection.clone(),
CollectionDetails {
owner: owner.clone(),
issuer: admin.clone(),
Expand All @@ -104,7 +104,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
witness: DestroyWitness,
maybe_check_owner: Option<T::AccountId>,
) -> Result<DestroyWitness, DispatchError> {
Collection::<T, I>::try_mutate_exists(collection, |maybe_details| {
Collection::<T, I>::try_mutate_exists(collection.clone(), |maybe_details| {
let collection_details =
maybe_details.take().ok_or(Error::<T, I>::UnknownCollection)?;
if let Some(check_owner) = maybe_check_owner {
Expand Down Expand Up @@ -147,7 +147,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
owner: T::AccountId,
with_details: impl FnOnce(&CollectionDetailsFor<T, I>) -> DispatchResult,
) -> DispatchResult {
ensure!(!Item::<T, I>::contains_key(collection, item), Error::<T, I>::AlreadyExists);
ensure!(
!Item::<T, I>::contains_key(collection.clone(), item),
Error::<T, I>::AlreadyExists
);

Collection::<T, I>::try_mutate(
&collection,
Expand Down Expand Up @@ -189,7 +192,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
item: T::ItemId,
with_details: impl FnOnce(&CollectionDetailsFor<T, I>, &ItemDetailsFor<T, I>) -> DispatchResult,
) -> DispatchResult {
ensure!(!T::Locker::is_locked(collection, item), Error::<T, I>::Locked);
ensure!(!T::Locker::is_locked(collection.clone(), item), Error::<T, I>::Locked);
let owner = Collection::<T, I>::try_mutate(
&collection,
|maybe_collection_details| -> Result<T::AccountId, DispatchError> {
Expand Down Expand Up @@ -268,7 +271,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {

let old_owner = details.owner.clone();

Self::do_transfer(collection, item, buyer.clone(), |_, _| Ok(()))?;
Self::do_transfer(collection.clone(), item, buyer.clone(), |_, _| Ok(()))?;

Self::deposit_event(Event::ItemBought {
collection,
Expand Down
Loading

0 comments on commit 98c24be

Please sign in to comment.