diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 8ef7e7852ad02..3bcc02deeb48a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1453,6 +1453,7 @@ parameter_types! { pub const ItemDeposit: Balance = 1 * DOLLARS; pub const KeyLimit: u32 = 32; pub const ValueLimit: u32 = 256; + pub const ApprovalsLimit: u32 = 20; } impl pallet_uniques::Config for Runtime { @@ -1469,6 +1470,7 @@ impl pallet_uniques::Config for Runtime { type StringLimit = StringLimit; type KeyLimit = KeyLimit; type ValueLimit = ValueLimit; + type ApprovalsLimit = ApprovalsLimit; type WeightInfo = pallet_uniques::weights::SubstrateWeight; #[cfg(feature = "runtime-benchmarks")] type Helper = (); diff --git a/frame/uniques/src/benchmarking.rs b/frame/uniques/src/benchmarking.rs index 3e3148b5b5fc2..8e21b73aa5e99 100644 --- a/frame/uniques/src/benchmarking.rs +++ b/frame/uniques/src/benchmarking.rs @@ -380,7 +380,7 @@ benchmarks_instance_pallet! { let delegate_lookup = T::Lookup::unlookup(delegate.clone()); let origin = SystemOrigin::Signed(caller.clone()).into(); Uniques::::approve_transfer(origin, collection, item, delegate_lookup.clone())?; - }: _(SystemOrigin::Signed(caller.clone()), collection, item, Some(delegate_lookup)) + }: _(SystemOrigin::Signed(caller.clone()), collection, item, delegate_lookup) verify { assert_last_event::(Event::ApprovalCancelled { collection, item, owner: caller, delegate }.into()); } diff --git a/frame/uniques/src/functions.rs b/frame/uniques/src/functions.rs index 4e68f1139420d..8d15008996a2d 100644 --- a/frame/uniques/src/functions.rs +++ b/frame/uniques/src/functions.rs @@ -21,6 +21,7 @@ use super::*; use frame_support::{ ensure, traits::{ExistenceRequirement, Get}, + BoundedVec, }; use sp_runtime::{DispatchError, DispatchResult}; @@ -47,12 +48,13 @@ impl, I: 'static> Pallet { Account::::remove((&details.owner, &collection, &item)); Account::::insert((&dest, &collection, &item), ()); let origin = details.owner; + details.owner = dest; - // The approved account has to be reset to None, because otherwise pre-approve attack would - // be possible, where the owner can approve his second account before making the transaction - // and then claiming the item back. - details.approved = None; + // The approved accounts have to be reset to None, because otherwise pre-approve attack + // would be possible, where the owner can approve his second account before making the + // transaction and then claiming the item back. + details.approved = BoundedVec::default(); Item::::insert(&collection, &item, &details); ItemPriceOf::::remove(&collection, &item); @@ -174,7 +176,12 @@ impl, I: 'static> Pallet { let owner = owner.clone(); Account::::insert((&owner, &collection, &item), ()); - let details = ItemDetails { owner, approved: None, is_frozen: false, deposit }; + let details = ItemDetails { + owner, + approved: BoundedVec::default(), + is_frozen: false, + deposit, + }; Item::::insert(&collection, &item, details); Ok(()) }, diff --git a/frame/uniques/src/lib.rs b/frame/uniques/src/lib.rs index 3b0cf6dc673c9..abaa8c87a21c4 100644 --- a/frame/uniques/src/lib.rs +++ b/frame/uniques/src/lib.rs @@ -150,6 +150,10 @@ pub mod pallet { #[pallet::constant] type ValueLimit: Get; + /// The maximum approvals an item could have. + #[pallet::constant] + type ApprovalsLimit: Get; + #[cfg(feature = "runtime-benchmarks")] /// A set of helper functions for benchmarking. type Helper: BenchmarkHelper; @@ -210,7 +214,7 @@ pub mod pallet { T::CollectionId, Blake2_128Concat, T::ItemId, - ItemDetails>, + ItemDetails, T::ApprovalsLimit>, OptionQuery, >; @@ -272,13 +276,26 @@ pub mod pallet { #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event, I: 'static = ()> { /// A `collection` was created. - Created { collection: T::CollectionId, creator: T::AccountId, owner: T::AccountId }, + Created { + collection: T::CollectionId, + creator: T::AccountId, + owner: T::AccountId, + }, /// A `collection` was force-created. - ForceCreated { collection: T::CollectionId, owner: T::AccountId }, + ForceCreated { + collection: T::CollectionId, + owner: T::AccountId, + }, /// A `collection` was destroyed. - Destroyed { collection: T::CollectionId }, + Destroyed { + collection: T::CollectionId, + }, /// An `item` was issued. - Issued { collection: T::CollectionId, item: T::ItemId, owner: T::AccountId }, + Issued { + collection: T::CollectionId, + item: T::ItemId, + owner: T::AccountId, + }, /// An `item` was transferred. Transferred { collection: T::CollectionId, @@ -287,17 +304,34 @@ pub mod pallet { to: T::AccountId, }, /// An `item` was destroyed. - Burned { collection: T::CollectionId, item: T::ItemId, owner: T::AccountId }, + Burned { + collection: T::CollectionId, + item: T::ItemId, + owner: T::AccountId, + }, /// Some `item` was frozen. - Frozen { collection: T::CollectionId, item: T::ItemId }, + Frozen { + collection: T::CollectionId, + item: T::ItemId, + }, /// Some `item` was thawed. - Thawed { collection: T::CollectionId, item: T::ItemId }, + Thawed { + collection: T::CollectionId, + item: T::ItemId, + }, /// Some `collection` was frozen. - CollectionFrozen { collection: T::CollectionId }, + CollectionFrozen { + collection: T::CollectionId, + }, /// Some `collection` was thawed. - CollectionThawed { collection: T::CollectionId }, + CollectionThawed { + collection: T::CollectionId, + }, /// The owner changed. - OwnerChanged { collection: T::CollectionId, new_owner: T::AccountId }, + OwnerChanged { + collection: T::CollectionId, + new_owner: T::AccountId, + }, /// The management team changed. TeamChanged { collection: T::CollectionId, @@ -321,8 +355,15 @@ pub mod pallet { owner: T::AccountId, delegate: T::AccountId, }, + AllApprovalsCancelled { + collection: T::CollectionId, + item: T::ItemId, + owner: T::AccountId, + }, /// A `collection` has had its attributes changed by the `Force` origin. - ItemStatusChanged { collection: T::CollectionId }, + ItemStatusChanged { + collection: T::CollectionId, + }, /// New metadata has been set for a `collection`. CollectionMetadataSet { collection: T::CollectionId, @@ -330,7 +371,9 @@ pub mod pallet { is_frozen: bool, }, /// Metadata has been cleared for a `collection`. - CollectionMetadataCleared { collection: T::CollectionId }, + CollectionMetadataCleared { + collection: T::CollectionId, + }, /// New metadata has been set for an item. MetadataSet { collection: T::CollectionId, @@ -339,9 +382,15 @@ pub mod pallet { is_frozen: bool, }, /// Metadata has been cleared for an item. - MetadataCleared { collection: T::CollectionId, item: T::ItemId }, + MetadataCleared { + collection: T::CollectionId, + item: T::ItemId, + }, /// Metadata has been cleared for an item. - Redeposited { collection: T::CollectionId, successful_items: Vec }, + Redeposited { + collection: T::CollectionId, + successful_items: Vec, + }, /// New attribute metadata has been set for a `collection` or `item`. AttributeSet { collection: T::CollectionId, @@ -356,9 +405,15 @@ pub mod pallet { key: BoundedVec, }, /// Ownership acceptance has changed for an account. - OwnershipAcceptanceChanged { who: T::AccountId, maybe_collection: Option }, + OwnershipAcceptanceChanged { + who: T::AccountId, + maybe_collection: Option, + }, /// Max supply has been set for a collection. - CollectionMaxSupplySet { collection: T::CollectionId, max_supply: u32 }, + CollectionMaxSupplySet { + collection: T::CollectionId, + max_supply: u32, + }, /// The price was set for the instance. ItemPriceSet { collection: T::CollectionId, @@ -367,7 +422,10 @@ pub mod pallet { whitelisted_buyer: Option, }, /// The price for the instance was removed. - ItemPriceRemoved { collection: T::CollectionId, item: T::ItemId }, + ItemPriceRemoved { + collection: T::CollectionId, + item: T::ItemId, + }, /// An item was bought. ItemBought { collection: T::CollectionId, @@ -394,8 +452,8 @@ pub mod pallet { InUse, /// The item or collection is frozen. Frozen, - /// The delegate turned out to be different to what was expected. - WrongDelegate, + /// The provided account is not a delegate. + NotDelegate, /// There is no delegate approved. NoDelegate, /// No approval exists that would allow the transfer. @@ -416,6 +474,8 @@ pub mod pallet { NotForSale, /// The provided bid is too low. BidTooLow, + /// The item has reached its approval limit. + ReachedApprovalLimit, } impl, I: 'static> Pallet { @@ -633,8 +693,7 @@ pub mod pallet { Self::do_transfer(collection, item, dest, |collection_details, details| { if details.owner != origin && collection_details.admin != origin { - let approved = details.approved.take().map_or(false, |i| i == origin); - ensure!(approved, Error::::NoPermission); + ensure!(details.approved.contains(&origin), Error::::NoPermission); } Ok(()) }) @@ -944,10 +1003,12 @@ pub mod pallet { ensure!(permitted, Error::::NoPermission); } - details.approved = Some(delegate); + let _ = details + .approved + .try_push(delegate.clone()) + .map_err(|_| Error::::ReachedApprovalLimit)?; Item::::insert(&collection, &item, &details); - let delegate = details.approved.expect("set as Some above; qed"); Self::deposit_event(Event::ApprovedTransfer { collection, item, @@ -979,12 +1040,14 @@ pub mod pallet { origin: OriginFor, collection: T::CollectionId, item: T::ItemId, - maybe_check_delegate: Option>, + delegate: AccountIdLookupOf, ) -> DispatchResult { let maybe_check: Option = T::ForceOrigin::try_origin(origin) .map(|_| None) .or_else(|origin| ensure_signed(origin).map(Some).map_err(DispatchError::from))?; + let delegate = T::Lookup::lookup(delegate)?; + let collection_details = Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; let mut details = @@ -993,18 +1056,48 @@ pub mod pallet { let permitted = check == collection_details.admin || check == details.owner; ensure!(permitted, Error::::NoPermission); } - let maybe_check_delegate = maybe_check_delegate.map(T::Lookup::lookup).transpose()?; - let old = details.approved.take().ok_or(Error::::NoDelegate)?; - if let Some(check_delegate) = maybe_check_delegate { - ensure!(check_delegate == old, Error::::WrongDelegate); - } + ensure!(!details.approved.is_empty(), Error::::NoDelegate); + ensure!(details.approved.contains(&delegate), Error::::NotDelegate); + + details.approved.retain(|d| *d != delegate); Item::::insert(&collection, &item, &details); Self::deposit_event(Event::ApprovalCancelled { collection, item, owner: details.owner, - delegate: old, + delegate, + }); + + Ok(()) + } + + #[pallet::weight(10_000)] + pub fn clear_all_transfer_approvals( + origin: OriginFor, + collection: T::CollectionId, + item: T::ItemId, + ) -> DispatchResult { + let maybe_check: Option = T::ForceOrigin::try_origin(origin) + .map(|_| None) + .or_else(|origin| ensure_signed(origin).map(Some).map_err(DispatchError::from))?; + + let collection_details = + Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; + let mut details = + Item::::get(&collection, &item).ok_or(Error::::UnknownCollection)?; + if let Some(check) = maybe_check { + let permitted = check == collection_details.admin || check == details.owner; + ensure!(permitted, Error::::NoPermission); + } + + // clear all approvals. + details.approved = BoundedVec::default(); + Item::::insert(&collection, &item, &details); + Self::deposit_event(Event::AllApprovalsCancelled { + collection, + item, + owner: details.owner, }); Ok(()) diff --git a/frame/uniques/src/mock.rs b/frame/uniques/src/mock.rs index ff7b791de4950..bf684e32235b8 100644 --- a/frame/uniques/src/mock.rs +++ b/frame/uniques/src/mock.rs @@ -100,6 +100,7 @@ impl Config for Test { type StringLimit = ConstU32<50>; type KeyLimit = ConstU32<50>; type ValueLimit = ConstU32<50>; + type ApprovalsLimit = ConstU32<10>; type WeightInfo = (); #[cfg(feature = "runtime-benchmarks")] type Helper = (); diff --git a/frame/uniques/src/tests.rs b/frame/uniques/src/tests.rs index 85d1bec574cf0..1e48addbbd7d9 100644 --- a/frame/uniques/src/tests.rs +++ b/frame/uniques/src/tests.rs @@ -550,7 +550,7 @@ fn approval_lifecycle_works() { assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 3)); assert_ok!(Uniques::transfer(Origin::signed(3), 0, 42, 4)); assert_noop!(Uniques::transfer(Origin::signed(3), 0, 42, 3), Error::::NoPermission); - assert!(Item::::get(0, 42).unwrap().approved.is_none()); + assert!(Item::::get(0, 42).unwrap().approved.is_empty()); assert_ok!(Uniques::approve_transfer(Origin::signed(4), 0, 42, 2)); assert_ok!(Uniques::transfer(Origin::signed(2), 0, 42, 2)); @@ -595,6 +595,39 @@ fn approved_account_gets_reset_after_buy_item() { }); } +#[test] +fn approving_multiple_accounts_works() { + new_test_ext().execute_with(|| { + assert_ok!(Uniques::force_create(Origin::root(), 0, 1, true)); + assert_ok!(Uniques::mint(Origin::signed(1), 0, 42, 2)); + + assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 3)); + assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 4)); + assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 5)); + + assert_ok!(Uniques::transfer(Origin::signed(4), 0, 42, 6)); + assert_noop!(Uniques::transfer(Origin::signed(3), 0, 42, 7), Error::::NoPermission); + assert_noop!(Uniques::transfer(Origin::signed(5), 0, 42, 8), Error::::NoPermission); + }); +} + +#[test] +fn approvals_limit_works() { + new_test_ext().execute_with(|| { + assert_ok!(Uniques::force_create(Origin::root(), 0, 1, true)); + assert_ok!(Uniques::mint(Origin::signed(1), 0, 42, 2)); + + for i in 3..13 { + assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, i)); + } + // the limit is 10 + assert_noop!( + Uniques::approve_transfer(Origin::signed(2), 0, 42, 14), + Error::::ReachedApprovalLimit + ); + }); +} + #[test] fn cancel_approval_works() { new_test_ext().execute_with(|| { @@ -603,25 +636,25 @@ fn cancel_approval_works() { assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 3)); assert_noop!( - Uniques::cancel_approval(Origin::signed(2), 1, 42, None), + Uniques::cancel_approval(Origin::signed(2), 1, 42, 3), Error::::UnknownCollection ); assert_noop!( - Uniques::cancel_approval(Origin::signed(2), 0, 43, None), + Uniques::cancel_approval(Origin::signed(2), 0, 43, 3), Error::::UnknownCollection ); assert_noop!( - Uniques::cancel_approval(Origin::signed(3), 0, 42, None), + Uniques::cancel_approval(Origin::signed(3), 0, 42, 3), Error::::NoPermission ); assert_noop!( - Uniques::cancel_approval(Origin::signed(2), 0, 42, Some(4)), - Error::::WrongDelegate + Uniques::cancel_approval(Origin::signed(2), 0, 42, 4), + Error::::NotDelegate ); - assert_ok!(Uniques::cancel_approval(Origin::signed(2), 0, 42, Some(3))); + assert_ok!(Uniques::cancel_approval(Origin::signed(2), 0, 42, 3)); assert_noop!( - Uniques::cancel_approval(Origin::signed(2), 0, 42, None), + Uniques::cancel_approval(Origin::signed(2), 0, 42, 3), Error::::NoDelegate ); }); @@ -635,21 +668,21 @@ fn cancel_approval_works_with_admin() { assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 3)); assert_noop!( - Uniques::cancel_approval(Origin::signed(1), 1, 42, None), + Uniques::cancel_approval(Origin::signed(1), 1, 42, 1), Error::::UnknownCollection ); assert_noop!( - Uniques::cancel_approval(Origin::signed(1), 0, 43, None), + Uniques::cancel_approval(Origin::signed(1), 0, 43, 1), Error::::UnknownCollection ); assert_noop!( - Uniques::cancel_approval(Origin::signed(1), 0, 42, Some(4)), - Error::::WrongDelegate + Uniques::cancel_approval(Origin::signed(1), 0, 42, 4), + Error::::NotDelegate ); - assert_ok!(Uniques::cancel_approval(Origin::signed(1), 0, 42, Some(3))); + assert_ok!(Uniques::cancel_approval(Origin::signed(1), 0, 42, 3)); assert_noop!( - Uniques::cancel_approval(Origin::signed(1), 0, 42, None), + Uniques::cancel_approval(Origin::signed(1), 0, 42, 1), Error::::NoDelegate ); }); @@ -663,23 +696,47 @@ fn cancel_approval_works_with_force() { assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 3)); assert_noop!( - Uniques::cancel_approval(Origin::root(), 1, 42, None), + Uniques::cancel_approval(Origin::root(), 1, 42, 1), Error::::UnknownCollection ); assert_noop!( - Uniques::cancel_approval(Origin::root(), 0, 43, None), + Uniques::cancel_approval(Origin::root(), 0, 43, 1), Error::::UnknownCollection ); assert_noop!( - Uniques::cancel_approval(Origin::root(), 0, 42, Some(4)), - Error::::WrongDelegate + Uniques::cancel_approval(Origin::root(), 0, 42, 4), + Error::::NotDelegate ); - assert_ok!(Uniques::cancel_approval(Origin::root(), 0, 42, Some(3))); + assert_ok!(Uniques::cancel_approval(Origin::root(), 0, 42, 3)); + assert_noop!(Uniques::cancel_approval(Origin::root(), 0, 42, 1), Error::::NoDelegate); + }); +} + +#[test] +fn clear_all_transfer_approvals_works() { + new_test_ext().execute_with(|| { + assert_ok!(Uniques::force_create(Origin::root(), 0, 1, true)); + assert_ok!(Uniques::mint(Origin::signed(1), 0, 42, 2)); + + assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 3)); + assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 4)); + assert_noop!( - Uniques::cancel_approval(Origin::root(), 0, 42, None), - Error::::NoDelegate + Uniques::clear_all_transfer_approvals(Origin::signed(3), 0, 42), + Error::::NoPermission ); + + assert_ok!(Uniques::clear_all_transfer_approvals(Origin::signed(2), 0, 42)); + + assert!(events().contains(&Event::::AllApprovalsCancelled { + collection: 0, + item: 42, + owner: 2, + })); + + assert_noop!(Uniques::transfer(Origin::signed(3), 0, 42, 5), Error::::NoPermission); + assert_noop!(Uniques::transfer(Origin::signed(4), 0, 42, 5), Error::::NoPermission); }); } diff --git a/frame/uniques/src/types.rs b/frame/uniques/src/types.rs index 98e056163d28d..308b40de0ddc9 100644 --- a/frame/uniques/src/types.rs +++ b/frame/uniques/src/types.rs @@ -28,8 +28,11 @@ pub(super) type DepositBalanceOf = <>::Currency as Currency<::AccountId>>::Balance; pub(super) type CollectionDetailsFor = CollectionDetails<::AccountId, DepositBalanceOf>; -pub(super) type ItemDetailsFor = - ItemDetails<::AccountId, DepositBalanceOf>; +pub(super) type ItemDetailsFor = ItemDetails< + ::AccountId, + DepositBalanceOf, + >::ApprovalsLimit, +>; pub(super) type ItemPrice = <>::Currency as Currency<::AccountId>>::Balance; @@ -84,11 +87,12 @@ impl CollectionDetails { /// Information concerning the ownership of a single unique item. #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, Default, TypeInfo, MaxEncodedLen)] -pub struct ItemDetails { +#[scale_info(skip_type_params(ApprovalsLimit))] +pub struct ItemDetails> { /// The owner of this item. pub(super) owner: AccountId, /// The approved transferrer of this item, if one is set. - pub(super) approved: Option, + pub(super) approved: BoundedVec, /// Whether the item can be transferred or not. pub(super) is_frozen: bool, /// The amount held in the pallet's default account for this item. Free-hold items will have