Nfts: Multiple approvals

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 {
@@ -1490,6 +1491,7 @@ impl pallet_nfts::Config for Runtime {
  type StringLimit = StringLimit;
  type KeyLimit = KeyLimit;
  type ValueLimit = ValueLimit;
+ type ApprovalsLimit = ApprovalsLimit;
  type WeightInfo = pallet_nfts::weights::SubstrateWeight<Runtime>;
  #[cfg(feature = "runtime-benchmarks")]
  type Helper = ();

frame/nfts/src/benchmarking.rs

@@ -368,9 +368,10 @@ benchmarks_instance_pallet! {
  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)
+ let deadline = T::BlockNumber::max_value();
+ }: _(SystemOrigin::Signed(caller.clone()), collection, item, delegate_lookup, Some(deadline))
  verify {
- assert_last_event::<T, I>(Event::ApprovedTransfer { collection, item, owner: caller, delegate }.into());
+ assert_last_event::<T, I>(Event::ApprovedTransfer { collection, item, owner: caller, delegate, deadline: Some(deadline) }.into());
  }
 
  cancel_approval {
@@ -379,12 +380,26 @@ 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();
- Nfts::<T, I>::approve_transfer(origin, collection, item, delegate_lookup.clone())?;
- }: _(SystemOrigin::Signed(caller.clone()), collection, item, Some(delegate_lookup))
+ let deadline = T::BlockNumber::max_value();
+ Nfts::<T, I>::approve_transfer(origin, collection, item, delegate_lookup.clone(), Some(deadline))?;
+ }: _(SystemOrigin::Signed(caller.clone()), collection, item, delegate_lookup)
  verify {
  assert_last_event::<T, I>(Event::ApprovalCancelled { collection, item, owner: caller, delegate }.into());
  }
 
+ clear_all_transfer_approvals {
+ 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());
+ let origin = SystemOrigin::Signed(caller.clone()).into();
+ let deadline = T::BlockNumber::max_value();
+ Nfts::<T, I>::approve_transfer(origin, collection, item, delegate_lookup.clone(), Some(deadline))?;
+ }: _(SystemOrigin::Signed(caller.clone()), collection, item)
+ verify {
+ assert_last_event::<T, I>(Event::AllApprovalsCancelled {collection, item, owner: caller}.into());
+ }
+
  set_accept_ownership {
  let caller: T::AccountId = whitelisted_caller();
  T::Currency::make_free_balance_be(&caller, DepositBalanceOf::<T, I>::max_value());

frame/nfts/src/functions.rs

@@ -48,6 +48,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
  Account::<T, I>::insert((&dest, &collection, &item), ());
  let origin = details.owner;
  details.owner = dest;
+
+ // 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.approvals.clear();
+
  Item::<T, I>::insert(&collection, &item, &details);
  ItemPriceOf::<T, I>::remove(&collection, &item);
 
@@ -168,7 +174,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
  let owner = owner.clone();
  Account::<T, I>::insert((&owner, &collection, &item), ());
- let details = ItemDetails { owner, approved: None, is_frozen: false, deposit };
+ let details = ItemDetails {
+ owner,
+ approvals: ApprovalsOf::<T, I>::default(),
+ is_frozen: false,
+ deposit,
+ };
  Item::<T, I>::insert(&collection, &item, details);
  Ok(())
  },

frame/nfts/src/lib.rs

@@ -46,7 +46,7 @@ use frame_support::{
  traits::{
  tokens::Locker, BalanceStatus::Reserved, Currency, EnsureOriginWithArg, ReservableCurrency,
  },
- transactional,
+ transactional, BoundedBTreeMap,
 };
 use frame_system::Config as SystemConfig;
 use sp_runtime::{
@@ -59,7 +59,7 @@ pub use pallet::*;
 pub use types::*;
 pub use weights::WeightInfo;
 
-type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
+type AccountIdLookupOf<T> = <<T as SystemConfig>::Lookup as StaticLookup>::Source;
 
 #[frame_support::pallet]
 pub mod pallet {
@@ -149,6 +149,10 @@ pub mod pallet {
  #[pallet::constant]
  type ValueLimit: Get<u32>;
 
+ /// The maximum approvals an item could have.
+ #[pallet::constant]
+ type ApprovalsLimit: Get<u32>;
+
  #[cfg(feature = "runtime-benchmarks")]
  /// A set of helper functions for benchmarking.
  type Helper: BenchmarkHelper<Self::CollectionId, Self::ItemId>;
@@ -157,6 +161,12 @@ pub mod pallet {
  type WeightInfo: WeightInfo;
  }
 
+ pub type ApprovalsOf<T, I = ()> = BoundedBTreeMap<
+ <T as SystemConfig>::AccountId,
+ Option<<T as SystemConfig>::BlockNumber>,
+ <T as Config<I>>::ApprovalsLimit,
+ >;
+
  #[pallet::storage]
  #[pallet::storage_prefix = "Class"]
  /// Details of a collection.
@@ -209,7 +219,7 @@ pub mod pallet {
  T::CollectionId,
  Blake2_128Concat,
  T::ItemId,
- ItemDetails<T::AccountId, DepositBalanceOf<T, I>>,
+ ItemDetails<T::AccountId, DepositBalanceOf<T, I>, ApprovalsOf<T, I>>,
  OptionQuery,
  >;
 
@@ -311,6 +321,7 @@ pub mod pallet {
  item: T::ItemId,
  owner: T::AccountId,
  delegate: T::AccountId,
+ deadline: Option<<T as SystemConfig>::BlockNumber>,
  },
  /// An approval for a `delegate` account to transfer the `item` of an item
  /// `collection` was cancelled by its `owner`.
@@ -320,6 +331,8 @@ pub mod pallet {
  owner: T::AccountId,
  delegate: T::AccountId,
  },
+ /// All approvals of an item got cancelled.
+ 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 },
  /// New metadata has been set for a `collection`.
@@ -385,6 +398,8 @@ pub mod pallet {
  UnknownCollection,
  /// The item ID has already been used for an item.
  AlreadyExists,
+ /// The approval had a deadline that expired, so the approval isn't valid anymore.
+ ApprovalExpired,
  /// The owner turned out to be different to what was expected.
  WrongOwner,
  /// Invalid witness data given.
@@ -393,10 +408,10 @@ pub mod pallet {
  InUse,
  /// The item or collection is frozen.
  Frozen,
+ /// The provided account is not a delegate.
+ NotDelegate,
  /// The delegate turned out to be different to what was expected.
  WrongDelegate,
- /// There is no delegate approved.
- NoDelegate,
  /// No approval exists that would allow the transfer.
  Unapproved,
  /// The named owner has not signed ownership of the collection is acceptable.
@@ -415,6 +430,8 @@ pub mod pallet {
  NotForSale,
  /// The provided bid is too low.
  BidTooLow,
+ /// The item has reached its approval limit.
+ ReachedApprovalLimit,
  }
 
  impl<T: Config<I>, I: 'static> Pallet<T, I> {
@@ -630,8 +647,13 @@ 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::<T, I>::NoPermission);
+
+ let deadline =
+ details.approvals.get(&origin).ok_or(Error::<T, I>::NoPermission)?;
+ if let Some(d) = deadline {
+ let block_number = frame_system::Pallet::<T>::block_number();
+ ensure!(block_number <= *d, Error::<T, I>::ApprovalExpired);
+ }
  }
  Ok(())
  })
@@ -912,6 +934,8 @@ pub mod pallet {
  /// - `collection`: The collection of the item to be approved for delegated transfer.
  /// - `item`: The item of the item to be approved for delegated transfer.
  /// - `delegate`: The account to delegate permission to transfer the item.
+ /// - `maybe_deadline`: Optional deadline for the approval. Specified by providing the
+ /// number of blocks after which the approval will expire
  ///
  /// Emits `ApprovedTransfer` on success.
  ///
@@ -922,6 +946,7 @@ pub mod pallet {
  collection: T::CollectionId,
  item: T::ItemId,
  delegate: AccountIdLookupOf<T>,
+ maybe_deadline: Option<<T as SystemConfig>::BlockNumber>,
  ) -> DispatchResult {
  let maybe_check: Option<T::AccountId> = T::ForceOrigin::try_origin(origin)
  .map(|_| None)
@@ -939,21 +964,27 @@ pub mod pallet {
  ensure!(permitted, Error::<T, I>::NoPermission);
  }
 
- details.approved = Some(delegate);
+ let now = frame_system::Pallet::<T>::block_number();
+ let deadline = maybe_deadline.map(|d| d.saturating_add(now));
+
+ details
+ .approvals
+ .try_insert(delegate.clone(), deadline)
+ .map_err(|_| Error::<T, I>::ReachedApprovalLimit)?;
  Item::<T, I>::insert(&collection, &item, &details);
 
- let delegate = details.approved.expect("set as Some above; qed");
  Self::deposit_event(Event::ApprovedTransfer {
  collection,
  item,
  owner: details.owner,
  delegate,
+ deadline,
  });
 
  Ok(())
  }
 
- /// Cancel the prior approval for the transfer of an item by a delegate.
+ /// Cancel one of the transfer approvals for a specific item.
  ///
  /// Origin must be either:
  /// - the `Force` origin;
@@ -962,9 +993,8 @@ pub mod pallet {
  ///
  /// Arguments:
  /// - `collection`: The collection of the item of whose approval will be cancelled.
- /// - `item`: The item of the item of whose approval will be cancelled.
- /// - `maybe_check_delegate`: If `Some` will ensure that the given account is the one to
- /// which permission of transfer is delegated.
+ /// - `item`: The item of the collection of whose approval will be cancelled.
+ /// - `delegate`: The account that is going to loose their approval.
  ///
  /// Emits `ApprovalCancelled` on success.
  ///
@@ -974,7 +1004,67 @@ pub mod pallet {
  origin: OriginFor<T>,
  collection: T::CollectionId,
  item: T::ItemId,
- maybe_check_delegate: Option<AccountIdLookupOf<T>>,
+ delegate: AccountIdLookupOf<T>,
+ ) -> DispatchResult {
+ let maybe_check: Option<T::AccountId> = 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::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
+ let mut details =
+ Item::<T, I>::get(&collection, &item).ok_or(Error::<T, I>::UnknownCollection)?;
+
+ let mut is_past_deadline = false;
+ let deadline = details.approvals.get(&delegate).ok_or(Error::<T, I>::NotDelegate)?;
+
+ if let Some(d) = deadline {
+ let now = frame_system::Pallet::<T>::block_number();
+ if *d < now {
+ is_past_deadline = true;
+ }
+ }
+
+ if !is_past_deadline {
+ if let Some(check) = maybe_check {
+ let permitted = check == collection_details.admin || check == details.owner;
+ ensure!(permitted, Error::<T, I>::NoPermission);
+ }
+ }
+
+ details.approvals.remove(&delegate);
+ Item::<T, I>::insert(&collection, &item, &details);
+ Self::deposit_event(Event::ApprovalCancelled {
+ collection,
+ item,
+ owner: details.owner,
+ delegate,
+ });
+
+ Ok(())
+ }
+
+ /// Cancel all the approvals of a specific item.
+ ///
+ /// Origin must be either:
+ /// - the `Force` origin;
+ /// - `Signed` with the signer being the Admin of the `collection`;
+ /// - `Signed` with the signer being the Owner of the `item`;
+ ///
+ /// Arguments:
+ /// - `collection`: The collection of the item of whose approvals will be cleared.
+ /// - `item`: The item of the collection of whose approvals will be cleared.
+ ///
+ /// Emits `AllApprovalsCancelled` on success.
+ ///
+ /// Weight: `O(1)`
+ #[pallet::weight(T::WeightInfo::clear_all_transfer_approvals())]
+ pub fn clear_all_transfer_approvals(
+ origin: OriginFor<T>,
+ collection: T::CollectionId,
+ item: T::ItemId,
  ) -> DispatchResult {
  let maybe_check: Option<T::AccountId> = T::ForceOrigin::try_origin(origin)
  .map(|_| None)
@@ -988,18 +1078,13 @@ pub mod pallet {
  let permitted = check == collection_details.admin || check == details.owner;
  ensure!(permitted, Error::<T, I>::NoPermission);
  }
- let maybe_check_delegate = maybe_check_delegate.map(T::Lookup::lookup).transpose()?;
- let old = details.approved.take().ok_or(Error::<T, I>::NoDelegate)?;
- if let Some(check_delegate) = maybe_check_delegate {
- ensure!(check_delegate == old, Error::<T, I>::WrongDelegate);
- }
 
+ details.approvals.clear();
  Item::<T, I>::insert(&collection, &item, &details);
- Self::deposit_event(Event::ApprovalCancelled {
+ Self::deposit_event(Event::AllApprovalsCancelled {
  collection,
  item,
  owner: details.owner,
- delegate: old,
  });
 
  Ok(())

frame/nfts/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 = ();