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

@@ -380,7 +380,7 @@ benchmarks_instance_pallet! {
 		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))
+	}: _(SystemOrigin::Signed(caller.clone()), collection, item, delegate_lookup)
 	verify {
 		assert_last_event::<T, I>(Event::ApprovalCancelled { collection, item, owner: caller, delegate }.into());
 	}

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::{
@@ -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 frame_system::Config>::AccountId,
+		Option<<T as frame_system::Config>::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,
 	>;
 
@@ -271,13 +281,26 @@ pub mod pallet {
 	#[pallet::generate_deposit(pub(super) fn deposit_event)]
 	pub enum Event<T: Config<I>, 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,
@@ -286,17 +309,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,
@@ -320,16 +360,26 @@ 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 },
+		ItemStatusChanged {
+			collection: T::CollectionId,
+		},
 		/// New metadata has been set for a `collection`.
 		CollectionMetadataSet {
 			collection: T::CollectionId,
 			data: BoundedVec<u8, T::StringLimit>,
 			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,
@@ -338,9 +388,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<T::ItemId> },
+		Redeposited {
+			collection: T::CollectionId,
+			successful_items: Vec<T::ItemId>,
+		},
 		/// New attribute metadata has been set for a `collection` or `item`.
 		AttributeSet {
 			collection: T::CollectionId,
@@ -355,9 +411,15 @@ pub mod pallet {
 			key: BoundedVec<u8, T::KeyLimit>,
 		},
 		/// Ownership acceptance has changed for an account.
-		OwnershipAcceptanceChanged { who: T::AccountId, maybe_collection: Option<T::CollectionId> },
+		OwnershipAcceptanceChanged {
+			who: T::AccountId,
+			maybe_collection: Option<T::CollectionId>,
+		},
 		/// 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,
@@ -366,7 +428,10 @@ pub mod pallet {
 			whitelisted_buyer: Option<T::AccountId>,
 		},
 		/// 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,
@@ -393,10 +458,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 +480,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,7 +697,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);
+					let approved = details.approvals.contains_key(&origin);
 					ensure!(approved, Error::<T, I>::NoPermission);
 				}
 				Ok(())
@@ -939,10 +1006,12 @@ pub mod pallet {
 				ensure!(permitted, Error::<T, I>::NoPermission);
 			}
 
-			details.approved = Some(delegate);
+			details
+				.approvals
+				.try_insert(delegate.clone(), None)
+				.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,
@@ -974,12 +1043,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 +1059,46 @@ 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(())
+		}
+
+		#[pallet::weight(10_000)]
+		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 = ();