feat(nfts): collection approval deposit

pallets/nfts/src/features/approvals.rs

@@ -19,7 +19,7 @@
 //! The bitflag [`PalletFeature::Approvals`] needs to be set in [`Config::Features`] for NFTs
 //! to have the functionality defined in this module.
 
-use frame_support::pallet_prelude::*;
+use frame_support::{pallet_prelude::*, sp_runtime::ArithmeticError};
 
 use crate::*;
 
@@ -187,12 +187,15 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// Approves the transfer of items in the collection that owned by the origin to a delegate.
 	///
 	/// This function is used to approve the transfer of items in the `collection` that owned by the
-	/// `origin` to a `delegate`.The `delegate` is the account that will be allowed to take control
+	/// `origin` to a `delegate`. The `delegate` is the account that will be allowed to take control
 	/// of items in the collection that owned by the `origin`. Optionally, a `deadline` can be
 	/// specified to set a time limit for the approval. The `deadline` is expressed in block
 	/// numbers and is added to the current block number to determine the absolute deadline for the
 	/// approval. After approving the transfer, the function emits the `TransferApproved` event.
 	///
+	/// This function reserves the required deposit from the owner's account. If an approval already
+	/// exists, the new amount is added to such existing approval.
+	///
 	/// - `origin`: The account grants permission to approve the transfer.
 	/// - `collection`: The identifier of the collection.
 	/// - `delegate`: The account that will be allowed to take control of items in the collection
@@ -220,31 +223,40 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		CollectionApprovals::<T, I>::try_mutate_exists(
 			(&collection, &origin, &delegate),
-			|maybe_approval| -> Result<(), DispatchError> {
-				if maybe_approval.is_none() {
-					// Increment approval counts for the `origin`, ensuring limits are respected.
-					CollectionApprovalCount::<T, I>::try_mutate(
-						collection,
-						Some(&origin),
-						|approvals| -> Result<(), DispatchError> {
-							ensure!(
-								*approvals < T::ApprovalsLimit::get(),
-								Error::<T, I>::ReachedApprovalLimit
-							);
-							approvals.saturating_inc();
-							Ok(())
-						},
-					)?;
-
-					// Increment the total approval count for the collection.
-					CollectionApprovalCount::<T, I>::mutate(
-						collection,
-						Option::<T::AccountId>::None,
-						|approvals| approvals.saturating_inc(),
-					);
+			|maybe_approval| -> DispatchResult {
+				let deposit_required = T::CollectionApprovalDeposit::get();
+				let mut current_deposit = match maybe_approval.take() {
+					Some((_, deposit)) => deposit,
+					None => {
+						// Increment approval counts for the `origin`, ensuring limits are
+						// respected.
+						CollectionApprovalCount::<T, I>::try_mutate(
+							collection,
+							Some(&origin),
+							|approvals| -> DispatchResult {
+								ensure!(
+									*approvals < T::ApprovalsLimit::get(),
+									Error::<T, I>::ReachedApprovalLimit
+								);
+								approvals.saturating_inc();
+								Ok(())
+							},
+						)?;
+						// Increment the total approval count for the collection.
+						CollectionApprovalCount::<T, I>::mutate(
+							collection,
+							Option::<T::AccountId>::None,
+							|approvals| approvals.saturating_inc(),
+						);
+						Zero::zero()
+					},
+				};
+
+				if current_deposit < deposit_required {
+					T::Currency::reserve(&origin, deposit_required - current_deposit)?;
+					current_deposit = deposit_required;
 				}
-				*maybe_approval = Some(deadline);
-
+				*maybe_approval = Some((deadline, current_deposit));
 				Ok(())
 			},
 		)?;
@@ -264,8 +276,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// a delegate.
 	///
 	/// This function is used to cancel the approval for the transfer of items in the `collection`
-	/// that owned by the `origin` to a `delegate`. After canceling the approval, the function emits
-	/// the `ApprovalCancelled` event.
+	/// that owned by the `origin` to a `delegate`. After canceling the approval, the function
+	/// returns the `origin` back the deposited fund and emits the `ApprovalCancelled` event.
 	///
 	/// - `origin`: The account grants permission to cancel the transfer.
 	/// - `collection`: The identifier of the collection.
@@ -276,16 +288,22 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		collection: T::CollectionId,
 		delegate: T::AccountId,
 	) -> DispatchResult {
-		CollectionApprovals::<T, I>::take((&collection, &origin, &delegate))
+		let (_, deposit) = CollectionApprovals::<T, I>::take((&collection, &origin, &delegate))
 			.ok_or(Error::<T, I>::UnknownCollection)?;
-		CollectionApprovalCount::<T, I>::mutate(collection, Some(&origin), |approvals| {
-			approvals.saturating_dec();
-		});
-		CollectionApprovalCount::<T, I>::mutate(
-			collection,
-			Option::<T::AccountId>::None,
-			|approvals| approvals.saturating_dec(),
-		);
+
+		// Update the approval count for the `origin` account and the whole collection.
+		for key in vec![Some(&origin), Option::<&T::AccountId>::None] {
+			let count = CollectionApprovalCount::<T, I>::get(collection, key)
+				.checked_sub(1)
+				.ok_or(ArithmeticError::Overflow)?;
+			if count == 0 {
+				CollectionApprovalCount::<T, I>::remove(collection, key);
+			} else {
+				CollectionApprovalCount::<T, I>::insert(collection, key, count);
+			}
+		}
+
+		T::Currency::unreserve(&origin, deposit);
 
 		Self::deposit_event(Event::ApprovalCancelled {
 			collection,
@@ -300,7 +318,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// Clears all collection approvals.
 	///
 	/// This function is used to clear all approvals to transfer items in the `collection` that
-	/// owned by the `origin` to a `delegate`. After clearing all approvals, the function emits the
+	/// owned by the `origin` to a `delegate`. After clearing all approvals, the function returns
+	/// the `origin` back the deposited fund of each collection approval and emits the
 	/// `AllApprovalsCancelled` event.
 	///
 	/// - `origin`: The account grants permission to clear the transfer.
@@ -311,19 +330,40 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		origin: T::AccountId,
 		collection: T::CollectionId,
 		witness_approvals: u32,
-	) -> DispatchResult {
-		let approvals = CollectionApprovalCount::<T, I>::take(collection, Some(&origin));
-		ensure!(approvals == witness_approvals, Error::<T, I>::BadWitness);
-		let _ = CollectionApprovals::<T, I>::clear_prefix((collection, &origin), approvals, None);
-		CollectionApprovalCount::<T, I>::mutate(
+	) -> Result<u32, DispatchError> {
+		let mut removed_approvals: u32 = 0;
+		CollectionApprovalCount::<T, I>::try_mutate_exists(
 			collection,
 			Option::<T::AccountId>::None,
-			|total_approvals| *total_approvals = total_approvals.saturating_sub(approvals),
-		);
-
-		Self::deposit_event(Event::AllApprovalsCancelled { collection, item: None, owner: origin });
-
-		Ok(())
+			|maybe_collection| -> DispatchResult {
+				let total_approvals =
+					maybe_collection.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;
+
+				// Remove the total number of collection approvals from the `origin`.
+				let approvals = CollectionApprovalCount::<T, I>::take(collection, Some(&origin));
+				ensure!(approvals == witness_approvals, Error::<T, I>::BadWitness);
+
+				// Iterate and remove each collection approval, return the deposited fund back to
+				// the `origin`.
+				for (_, (_, deposit)) in
+					CollectionApprovals::<T, I>::drain_prefix((collection, &origin))
+				{
+					T::Currency::unreserve(&origin, deposit);
+					removed_approvals.saturating_inc();
+					total_approvals.saturating_dec();
+					if removed_approvals >= approvals {
+						break
+					}
+				}
+				Self::deposit_event(Event::AllApprovalsCancelled {
+					collection,
+					item: None,
+					owner: origin,
+				});
+				Ok(())
+			},
+		)?;
+		Ok(removed_approvals)
 	}
 
 	/// Checks whether the `delegate` has the necessary allowance to transfer items in the
@@ -340,9 +380,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		collection: &T::CollectionId,
 		account: &T::AccountId,
 		delegate: &T::AccountId,
-	) -> Result<(), DispatchError> {
-		let maybe_deadline = CollectionApprovals::<T, I>::get((&collection, &account, &delegate))
-			.ok_or(Error::<T, I>::NoPermission)?;
+	) -> DispatchResult {
+		let (maybe_deadline, _) =
+			CollectionApprovals::<T, I>::get((&collection, &account, &delegate))
+				.ok_or(Error::<T, I>::NoPermission)?;
 		if let Some(deadline) = maybe_deadline {
 			let block_number = frame_system::Pallet::<T>::block_number();
 			ensure!(block_number <= deadline, Error::<T, I>::ApprovalExpired);
@@ -368,7 +409,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		maybe_item: &Option<T::ItemId>,
 		account: &T::AccountId,
 		delegate: &T::AccountId,
-	) -> Result<(), DispatchError> {
+	) -> DispatchResult {
 		// Check if a `delegate` has a permission to transfer items in the collection that owned by
 		// the `owner`.
 		let error = match Self::check_collection_approval(collection, account, delegate) {

pallets/nfts/src/features/create_delete_item.rs

@@ -18,7 +18,7 @@
 //! This module contains helper methods to perform functionality associated with minting and burning
 //! items for the NFTs pallet.
 
-use frame_support::{pallet_prelude::*, traits::ExistenceRequirement};
+use frame_support::{pallet_prelude::*, sp_runtime::ArithmeticError, traits::ExistenceRequirement};
 
 use crate::*;
 
@@ -68,7 +68,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 			collection_details.items.saturating_inc();
 
-			AccountBalance::<T, I>::mutate(&mint_to, collection, |balance| {
+			AccountBalance::<T, I>::mutate(collection, &mint_to, |balance| {
 				balance.saturating_inc();
 			});
 
@@ -263,9 +263,15 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		ItemPriceOf::<T, I>::remove(collection, item);
 		PendingSwapOf::<T, I>::remove(collection, item);
 		ItemAttributesApprovalsOf::<T, I>::remove(collection, item);
-		AccountBalance::<T, I>::mutate(&owner, collection, |balance| {
-			balance.saturating_dec();
-		});
+
+		let balance = AccountBalance::<T, I>::get(collection, &owner)
+			.checked_sub(1)
+			.ok_or(ArithmeticError::Overflow)?;
+		if balance == 0 {
+			AccountBalance::<T, I>::remove(collection, &owner);
+		} else {
+			AccountBalance::<T, I>::insert(collection, &owner, balance);
+		}
 
 		if remove_config {
 			ItemConfigOf::<T, I>::remove(collection, item);

pallets/nfts/src/features/transfer.rs

@@ -87,11 +87,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		with_details(&collection_details, &mut details)?;
 
 		// Update account balance of the owner.
-		AccountBalance::<T, I>::mutate(&details.owner, collection, |balance| {
+		AccountBalance::<T, I>::mutate(collection, &details.owner, |balance| {
 			balance.saturating_dec();
 		});
 		// Update account balance of the destination account.
-		AccountBalance::<T, I>::mutate(&dest, collection, |balance| {
+		AccountBalance::<T, I>::mutate(collection, &dest, |balance| {
 			balance.saturating_inc();
 		});
 

pallets/nfts/src/lib.rs

@@ -175,6 +175,10 @@ pub mod pallet {
 		#[pallet::constant]
 		type CollectionDeposit: Get<DepositBalanceOf<Self, I>>;
 
+		/// The basic amount of funds that must be reserved for collection approvals.
+		#[pallet::constant]
+		type CollectionApprovalDeposit: Get<DepositBalanceOf<Self, I>>;
+
 		/// The basic amount of funds that must be reserved for an item.
 		#[pallet::constant]
 		type ItemDeposit: Get<DepositBalanceOf<Self, I>>;
@@ -411,9 +415,9 @@ pub mod pallet {
 	pub type AccountBalance<T: Config<I>, I: 'static = ()> = StorageDoubleMap<
 		_,
 		Blake2_128Concat,
-		T::AccountId,
-		Blake2_128Concat,
 		T::CollectionId,
+		Blake2_128Concat,
+		T::AccountId,
 		u32,
 		ValueQuery,
 	>;
@@ -425,12 +429,12 @@ pub mod pallet {
 		(
 			// Collection ID.
 			NMapKey<Blake2_128Concat, T::CollectionId>,
-			// Collection Owner Id.
+			// Collection Account Id.
 			NMapKey<Blake2_128Concat, T::AccountId>,
 			// Delegate Id.
 			NMapKey<Blake2_128Concat, T::AccountId>,
 		),
-		Option<BlockNumberFor<T>>,
+		(Option<BlockNumberFor<T>>, DepositBalanceOf<T, I>),
 	>;
 
 	/// Total number approvals of a whole collection or a specified account.
@@ -1325,7 +1329,8 @@ pub mod pallet {
 		/// Approve an item to be transferred by a delegated third-party account.
 		///
 		/// Origin must be either `ForceOrigin` or Signed and the sender should be the Owner of the
-		/// `item`.
+		/// `item` if the `item` is provided. If not, the sender must have sufficient funds to grant
+		/// a collection approval.
 		///
 		/// - `collection`: The collection of the item to be approved for delegated transfer.
 		/// - `maybe_item`: The optional item to be approved for delegated transfer. If not
@@ -1378,7 +1383,7 @@ pub mod pallet {
 		///
 		/// Origin must be either:
 		/// - the `Force` origin;
-		/// - `Signed` with the signer being the Owner of the `item`;
+		/// - `Signed` with the signer being the Owner of the `item` if the `item` is provided;
 		///
 		/// Arguments:
 		/// - `collection`: The collection of the item of whose approval will be cancelled.
@@ -1418,7 +1423,7 @@ pub mod pallet {
 		///
 		/// Origin must be either:
 		/// - the `Force` origin;
-		/// - `Signed` with the signer being the Owner of the `item`;
+		/// - `Signed` with the signer being the Owner of the `item` if the `item` is provided;
 		///
 		/// Arguments:
 		/// - `collection`: The collection of the item of whose approvals will be cleared.
@@ -1435,32 +1440,35 @@ pub mod pallet {
 		#[pallet::weight(
 		    T::WeightInfo::clear_all_transfer_approvals(
 				maybe_item.is_some() as u32,
-				witness_approvals.unwrap_or_default()
+				T::ApprovalsLimit::get()
 			)
 		)]
 		pub fn clear_all_transfer_approvals(
 			origin: OriginFor<T>,
 			collection: T::CollectionId,
 			maybe_item: Option<T::ItemId>,
 			witness_approvals: Option<u32>,
-		) -> DispatchResult {
-			match maybe_item {
+		) -> DispatchResultWithPostInfo {
+			let weight = match maybe_item {
 				Some(item) => {
 					let maybe_check_origin =
 						T::ForceOrigin::try_origin(origin).map(|_| None).or_else(|origin| {
 							ensure_signed(origin).map(Some).map_err(DispatchError::from)
 						})?;
-					Self::do_clear_all_transfer_approvals(maybe_check_origin, collection, item)
+					Self::do_clear_all_transfer_approvals(maybe_check_origin, collection, item)?;
+					T::WeightInfo::clear_all_transfer_approvals(1, 0)
 				},
 				None => {
 					let origin = ensure_signed(origin)?;
-					Self::do_clear_all_collection_approvals(
+					let removed_approvals = Self::do_clear_all_collection_approvals(
 						origin,
 						collection,
 						witness_approvals.unwrap_or_default(),
-					)
+					)?;
+					T::WeightInfo::clear_all_transfer_approvals(0, removed_approvals)
 				},
-			}
+			};
+			Ok(Some(weight).into())
 		}
 
 		/// Disallows changing the metadata or attributes of the item.

pallets/nfts/src/mock.rs

@@ -65,6 +65,7 @@ parameter_types! {
 impl Config for Test {
 	type ApprovalsLimit = ConstU32<10>;
 	type AttributeDepositBase = ConstU64<1>;
+	type CollectionApprovalDeposit = ConstU64<1>;
 	type CollectionDeposit = ConstU64<2>;
 	type CollectionId = u32;
 	type CreateOrigin = AsEnsureOriginWithArg<frame_system::EnsureSigned<Self::AccountId>>;