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,15 @@ 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);
+					let approved = details.approvals.contains_key(&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!(*d >= block_number, Error::<T, I>::ApprovalExpired);
+					}
 				}
 				Ok(())
 			})
@@ -912,6 +936,7 @@ 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.
 		///
 		/// Emits `ApprovedTransfer` on success.
 		///
@@ -922,6 +947,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 +965,29 @@ pub mod pallet {
 				ensure!(permitted, Error::<T, I>::NoPermission);
 			}
 
-			details.approved = Some(delegate);
+			let deadline = match maybe_deadline {
+				Some(d) => Some(d.saturating_add(frame_system::Pallet::<T>::block_number())),
+				None => None,
+			};
+
+			details
+				.approvals
+				.try_insert(delegate.clone(), deadline.clone())
+				.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 +996,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,12 +1007,14 @@ 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 =
@@ -988,18 +1023,60 @@ 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);
-			}
 
+			ensure!(details.approvals.contains_key(&delegate), Error::<T, I>::NotDelegate);
+
+			details.approvals.remove(&delegate);
 			Item::<T, I>::insert(&collection, &item, &details);
 			Self::deposit_event(Event::ApprovalCancelled {
 				collection,
 				item,
 				owner: details.owner,
-				delegate: old,
+				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)
+				.or_else(|origin| ensure_signed(origin).map(Some).map_err(DispatchError::from))?;
+
+			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)?;
+			if let Some(check) = maybe_check {
+				let permitted = check == collection_details.admin || check == details.owner;
+				ensure!(permitted, Error::<T, I>::NoPermission);
+			}
+
+			details.approvals.clear();
+			Item::<T, I>::insert(&collection, &item, &details);
+			Self::deposit_event(Event::AllApprovalsCancelled {
+				collection,
+				item,
+				owner: details.owner,
 			});
 
 			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 = ();