refactor: remove CollectionApprovalCount storage item

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::*, sp_runtime::ArithmeticError};
+use frame_support::pallet_prelude::*;
 use frame_system::pallet_prelude::BlockNumberFor;
 
 use crate::*;
@@ -158,18 +158,17 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		collection: T::CollectionId,
 		item: T::ItemId,
 	) -> DispatchResult {
-		let collection_details =
-			Collection::<T, I>::get(collection).ok_or(Error::<T, I>::UnknownCollection)?;
-
-		ensure!(
-			CollectionApprovalCount::<T, I>::get(collection, Some(collection_details.owner)) == 0,
-			Error::<T, I>::DelegateApprovalConflict
-		);
-
 		let mut details =
 			Item::<T, I>::get(collection, item).ok_or(Error::<T, I>::UnknownCollection)?;
 
 		if let Some(check_origin) = maybe_check_origin {
+			ensure!(
+				CollectionApprovals::<T, I>::iter_prefix((collection, &check_origin))
+					.take(1)
+					.next()
+					.is_none(),
+				Error::<T, I>::DelegateApprovalConflict
+			);
 			ensure!(check_origin == details.owner, Error::<T, I>::NoPermission);
 		}
 
@@ -228,29 +227,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				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()
-					},
+					None => Zero::zero(),
 				};
 
 				if current_deposit < deposit_required {
@@ -291,18 +268,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		let (_, deposit) = CollectionApprovals::<T, I>::take((&collection, &origin, &delegate))
 			.ok_or(Error::<T, I>::UnknownCollection)?;
 
-		// Update the approval count for the `origin` account and the whole collection.
-		for key in [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 {
@@ -330,40 +295,17 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		origin: T::AccountId,
 		collection: T::CollectionId,
 		witness_approvals: u32,
-	) -> Result<u32, DispatchError> {
+	) -> DispatchResult {
 		let mut removed_approvals: u32 = 0;
-		CollectionApprovalCount::<T, I>::try_mutate_exists(
-			collection,
-			Option::<T::AccountId>::None,
-			|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)
+		// 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();
+		}
+		ensure!(removed_approvals == witness_approvals, Error::<T, I>::BadWitness);
+		Self::deposit_event(Event::AllApprovalsCancelled { collection, item: None, owner: origin });
+		Ok(())
 	}
 
 	/// Checks whether the `delegate` has the necessary allowance to transfer items in the

pallets/nfts/src/features/create_delete_collection.rs

@@ -109,13 +109,14 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		Collection::<T, I>::try_mutate_exists(collection, |maybe_details| {
 			let collection_details =
 				maybe_details.take().ok_or(Error::<T, I>::UnknownCollection)?;
-			let collection_approvals =
-				CollectionApprovalCount::<T, I>::take(collection, Option::<T::AccountId>::None);
 			if let Some(check_owner) = maybe_check_owner {
 				ensure!(collection_details.owner == check_owner, Error::<T, I>::NoPermission);
 			}
 			ensure!(collection_details.items == 0, Error::<T, I>::CollectionNotEmpty);
-			ensure!(collection_approvals == 0, Error::<T, I>::CollectionApprovalsExist);
+			ensure!(
+				CollectionApprovals::<T, I>::iter_prefix((collection,)).take(1).next().is_none(),
+				Error::<T, I>::CollectionApprovalsExist
+			);
 			ensure!(collection_details.attributes == witness.attributes, Error::<T, I>::BadWitness);
 			ensure!(
 				collection_details.item_metadatas == witness.item_metadatas,

pallets/nfts/src/features/create_delete_item.rs

@@ -265,9 +265,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		ItemAttributesApprovalsOf::<T, I>::remove(collection, item);
 
 		let balance = AccountBalance::<T, I>::get(collection, &owner)
-			.checked_sub(1)
+			.checked_sub(&One::one())
 			.ok_or(ArithmeticError::Overflow)?;
-		if balance == 0 {
+		if balance == Zero::zero() {
 			AccountBalance::<T, I>::remove(collection, &owner);
 		} else {
 			AccountBalance::<T, I>::insert(collection, &owner, balance);

pallets/nfts/src/features/transfer.rs

@@ -92,9 +92,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		});
 		// Update account balance of the owner.
 		let balance = AccountBalance::<T, I>::get(collection, &details.owner)
-			.checked_sub(1)
+			.checked_sub(&One::one())
 			.ok_or(ArithmeticError::Overflow)?;
-		if balance == 0 {
+		if balance == Zero::zero() {
 			AccountBalance::<T, I>::remove(collection, &details.owner);
 		} else {
 			AccountBalance::<T, I>::insert(collection, &details.owner, balance);

pallets/nfts/src/lib.rs

@@ -63,7 +63,7 @@ use frame_support::traits::{
 use frame_system::Config as SystemConfig;
 pub use pallet::*;
 use sp_runtime::{
-	traits::{IdentifyAccount, Saturating, StaticLookup, Verify, Zero},
+	traits::{CheckedSub, IdentifyAccount, One, Saturating, StaticLookup, Verify, Zero},
 	RuntimeDebug,
 };
 pub use types::*;
@@ -151,7 +151,15 @@ pub mod pallet {
 		type CollectionId: Member + Parameter + MaxEncodedLen + Copy + Incrementable;
 
 		/// The type used to identify a unique item within a collection.
-		type ItemId: Member + Parameter + MaxEncodedLen + Copy;
+		type ItemId: Member
+			+ Parameter
+			+ MaxEncodedLen
+			+ Copy
+			+ Default
+			+ One
+			+ Zero
+			+ CheckedSub
+			+ Saturating;
 
 		/// The currency mechanism, used for paying for reserves.
 		type Currency: ReservableCurrency<Self::AccountId>;
@@ -418,7 +426,7 @@ pub mod pallet {
 		T::CollectionId,
 		Blake2_128Concat,
 		T::AccountId,
-		u32,
+		T::ItemId,
 		ValueQuery,
 	>;
 
@@ -437,18 +445,6 @@ pub mod pallet {
 		(Option<BlockNumberFor<T>>, DepositBalanceOf<T, I>),
 	>;
 
-	/// Total number approvals of a whole collection or a specified account.
-	#[pallet::storage]
-	pub type CollectionApprovalCount<T: Config<I>, I: 'static = ()> = StorageDoubleMap<
-		_,
-		Blake2_128Concat,
-		T::CollectionId,
-		Blake2_128Concat,
-		Option<T::AccountId>,
-		u32,
-		ValueQuery,
-	>;
-
 	/// Config of an item.
 	#[pallet::storage]
 	pub type ItemConfigOf<T: Config<I>, I: 'static = ()> = StorageDoubleMap<
@@ -702,7 +698,7 @@ pub mod pallet {
 		NotForSale,
 		/// The provided bid is too low.
 		BidTooLow,
-		/// The collection or item has reached its approval limit.
+		/// The item has reached its approval limit.
 		ReachedApprovalLimit,
 		/// The deadline has already expired.
 		DeadlineExpired,
@@ -1460,12 +1456,9 @@ pub mod pallet {
 				},
 				None => {
 					let origin = ensure_signed(origin)?;
-					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)
+					let witness_approvals = witness_approvals.unwrap_or_default();
+					Self::do_clear_all_collection_approvals(origin, collection, witness_approvals)?;
+					T::WeightInfo::clear_all_transfer_approvals(0, witness_approvals)
 				},
 			};
 			Ok(Some(weight).into())

pallets/nfts/src/tests.rs

@@ -36,8 +36,6 @@ use crate::{mock::*, Event, SystemConfig, *};
 
 type AccountIdOf<Test> = <Test as frame_system::Config>::AccountId;
 
-const NO_ACCOUNT: Option<AccountId> = Option::<AccountId>::None;
-
 fn account(id: u8) -> AccountIdOf<Test> {
 	[id; 32].into()
 }
@@ -355,10 +353,7 @@ fn destroy_should_work() {
 		));
 		assert_eq!(AccountBalance::<Test>::get(0, account(1)), 0);
 		assert_eq!(AccountBalance::<Test>::contains_key(0, account(5)), false);
-		assert_eq!(CollectionApprovalCount::<Test>::contains_key(0, NO_ACCOUNT), false);
 		assert_eq!(CollectionApprovals::<Test>::iter_prefix((0,)).count(), 0);
-		assert_eq!(CollectionApprovalCount::<Test>::contains_key(0, Some(account(5))), false);
-		assert_eq!(CollectionApprovalCount::<Test>::contains_key(0, NO_ACCOUNT), false);
 		assert!(!ItemConfigOf::<Test>::contains_key(0, 42));
 		assert_eq!(ItemConfigOf::<Test>::iter_prefix(0).count() as u32, 0);
 	});
@@ -2097,12 +2092,6 @@ fn cancel_approval_collection_works() {
 			delegate: account(3)
 		}));
 		assert_eq!(CollectionApprovals::<Test>::get((0, account(2), account(3))), None);
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, Some(account(2))), 0);
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, NO_ACCOUNT), 0);
-		assert_eq!(
-			CollectionApprovalCount::<Test>::get(0, Some(account(2))),
-			CollectionApprovals::<Test>::iter_prefix((0, account(2))).count() as u32
-		);
 
 		assert_noop!(
 			Nfts::transfer(RuntimeOrigin::signed(account(3)), 0, 42, account(4)),
@@ -2206,32 +2195,6 @@ fn approvals_limit_works() {
 	});
 }
 
-#[test]
-fn collection_approvals_limit_works() {
-	new_test_ext().execute_with(|| {
-		Balances::make_free_balance_be(&account(1), 100);
-		assert_ok!(Nfts::force_create(
-			RuntimeOrigin::root(),
-			account(1),
-			default_collection_config()
-		));
-		for i in 3..13 {
-			assert_ok!(Nfts::approve_transfer(
-				RuntimeOrigin::signed(account(1)),
-				0,
-				None,
-				account(i),
-				None
-			));
-		}
-		// the limit is 10
-		assert_noop!(
-			Nfts::approve_transfer(RuntimeOrigin::signed(account(1)), 0, None, account(14), None),
-			Error::<Test>::ReachedApprovalLimit
-		);
-	});
-}
-
 #[test]
 fn approve_transfer_collection_works() {
 	new_test_ext().execute_with(|| {
@@ -2279,8 +2242,6 @@ fn approve_transfer_collection_works() {
 			account(3),
 			None
 		));
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, Some(account(2))), 1);
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, NO_ACCOUNT), 1);
 		assert_eq!(Balances::reserved_balance(&account(2)), 1);
 
 		// must not update the total collection approvals.
@@ -2291,8 +2252,6 @@ fn approve_transfer_collection_works() {
 			account(3),
 			None
 		));
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, Some(account(2))), 1);
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, NO_ACCOUNT), 1);
 		assert_eq!(Balances::reserved_balance(&account(2)), 1);
 
 		assert_ok!(Nfts::approve_transfer(
@@ -2303,8 +2262,6 @@ fn approve_transfer_collection_works() {
 			None
 		));
 		assert_eq!(Balances::reserved_balance(&account(2)), 1);
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, Some(account(3))), 1);
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, NO_ACCOUNT), 2);
 		assert!(events().contains(&Event::<Test>::TransferApproved {
 			collection: 0,
 			item: None,
@@ -2313,10 +2270,6 @@ fn approve_transfer_collection_works() {
 			deadline: None
 		}));
 		assert_eq!(CollectionApprovals::<Test>::get((0, account(2), account(3))), Some((None, 1)));
-		assert_eq!(
-			CollectionApprovalCount::<Test>::get(0, Some(account(2))),
-			CollectionApprovals::<Test>::iter_prefix((0, account(2))).count() as u32
-		);
 		assert_ok!(Nfts::transfer(RuntimeOrigin::signed(account(3)), 0, 42, account(4)));
 	});
 }
@@ -2546,7 +2499,6 @@ fn clear_all_transfer_approvals_works() {
 			owner: account(2),
 		}));
 		assert_eq!(approvals(0, 42), vec![]);
-		assert_eq!(CollectionApprovalCount::<Test>::contains_key(0, Some(account(2))), false);
 		assert_eq!(CollectionApprovals::<Test>::iter_prefix((0, account(2))).count(), 0);
 
 		assert_noop!(
@@ -2611,12 +2563,6 @@ fn clear_all_collection_approvals_works() {
 		}));
 		assert_eq!(Balances::free_balance(&account(1)), 100);
 		assert_eq!(CollectionApprovals::<Test>::iter_prefix((0, account(1))).count(), 0);
-		assert_eq!(CollectionApprovalCount::<Test>::contains_key(0, Some(account(1))), false);
-		assert_eq!(CollectionApprovalCount::<Test>::get(0, NO_ACCOUNT), 0);
-		assert_eq!(
-			CollectionApprovalCount::<Test>::get(0, Some(account(1))),
-			CollectionApprovals::<Test>::iter_prefix((0, account(1))).count() as u32
-		);
 
 		assert_noop!(
 			Nfts::transfer(RuntimeOrigin::signed(account(3)), 0, 42, account(5)),