feat(nfts): AccountBalance deposit

pallets/nfts/src/benchmarking.rs

@@ -274,6 +274,7 @@ benchmarks_instance_pallet! {
 	mint {
 		let (collection, caller, caller_lookup) = create_collection::<T, I>();
 		let item = T::Helper::item(0);
+		T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance() + T::CollectionDeposit::get() + T::CollectionBalanceDeposit::get());
 	}: _(SystemOrigin::Signed(caller.clone()), collection, item, caller_lookup, None)
 	verify {
 		assert_last_event::<T, I>(Event::Issued { collection, item, owner: caller }.into());
@@ -282,6 +283,7 @@ benchmarks_instance_pallet! {
 	force_mint {
 		let (collection, caller, caller_lookup) = create_collection::<T, I>();
 		let item = T::Helper::item(0);
+		T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance() + T::CollectionDeposit::get() + T::CollectionBalanceDeposit::get());
 	}: _(SystemOrigin::Signed(caller.clone()), collection, item, caller_lookup, default_item_config())
 	verify {
 		assert_last_event::<T, I>(Event::Issued { collection, item, owner: caller }.into());
@@ -667,7 +669,7 @@ benchmarks_instance_pallet! {
 		let price = ItemPrice::<T, I>::from(0u32);
 		let origin = SystemOrigin::Signed(seller.clone()).into();
 		Nfts::<T, I>::set_price(origin, collection, item, Some(price), Some(buyer_lookup))?;
-		T::Currency::make_free_balance_be(&buyer, DepositBalanceOf::<T, I>::max_value());
+		T::Currency::make_free_balance_be(&buyer, T::Currency::minimum_balance() + price + T::CollectionBalanceDeposit::get());
 	}: _(SystemOrigin::Signed(buyer.clone()), collection, item, price)
 	verify {
 		assert_last_event::<T, I>(Event::ItemBought {
@@ -757,7 +759,7 @@ benchmarks_instance_pallet! {
 		let duration = T::MaxDeadlineDuration::get();
 		let target: T::AccountId = account("target", 0, SEED);
 		let target_lookup = T::Lookup::unlookup(target.clone());
-		T::Currency::make_free_balance_be(&target, T::Currency::minimum_balance());
+		T::Currency::make_free_balance_be(&target, T::Currency::minimum_balance() + T::CollectionBalanceDeposit::get());
 		let origin = SystemOrigin::Signed(caller.clone());
 		frame_system::Pallet::<T>::set_block_number(One::one());
 		Nfts::<T, I>::transfer(origin.clone().into(), collection, item2, target_lookup)?;

pallets/nfts/src/common_functions.rs

@@ -19,7 +19,7 @@
 
 use alloc::vec::Vec;
 
-use frame_support::pallet_prelude::*;
+use frame_support::{pallet_prelude::*, sp_runtime::ArithmeticError};
 
 use crate::*;
 
@@ -92,6 +92,52 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		Self::deposit_event(Event::NextCollectionIdIncremented { next_id });
 	}
 
+	/// Increment the number of items in the `collection` owned by the `owner`. If no entry exists
+	/// for the `owner` in `AccountBalance`, create a new record and reserve `deposit_amount` from
+	/// the `deposit_account`.
+	pub(crate) fn increment_account_balance(
+		collection: T::CollectionId,
+		owner: &T::AccountId,
+		(deposit_account, deposit_amount): (&T::AccountId, DepositBalanceOf<T, I>),
+	) -> DispatchResult {
+		AccountBalance::<T, I>::mutate(collection, owner, |maybe_balance| -> DispatchResult {
+			match maybe_balance {
+				None => {
+					T::Currency::reserve(deposit_account, deposit_amount)?;
+					*maybe_balance = Some((1, (deposit_account.clone(), deposit_amount)));
+				},
+				Some((balance, _deposit)) => {
+					balance.saturating_inc();
+				},
+			}
+			Ok(())
+		})
+	}
+
+	/// Decrement the number of `collection` items owned by the `owner`. If the `owner`'s item
+	/// count reaches zero after the reduction, remove the `AccountBalance` record and unreserve
+	/// the deposited funds.
+	pub(crate) fn decrement_account_balance(
+		collection: T::CollectionId,
+		owner: &T::AccountId,
+	) -> DispatchResult {
+		AccountBalance::<T, I>::try_mutate_exists(
+			collection,
+			owner,
+			|maybe_balance| -> DispatchResult {
+				let (balance, (deposit_account, deposit_amount)) =
+					maybe_balance.as_mut().ok_or(Error::<T, I>::NoItemOwned)?;
+
+				*balance = balance.checked_sub(1).ok_or(ArithmeticError::Underflow)?;
+				if balance.is_zero() {
+					T::Currency::unreserve(deposit_account, *deposit_amount);
+					*maybe_balance = None;
+				}
+				Ok(())
+			},
+		)
+	}
+
 	#[allow(missing_docs)]
 	#[cfg(any(test, feature = "runtime-benchmarks"))]
 	pub fn set_next_id(id: T::CollectionId) {
@@ -105,3 +151,87 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			.expect("Failed to get next collection ID")
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use crate::{mock::*, tests::*, Currency, Error, ReservableCurrency};
+
+	#[test]
+	fn increment_account_balance_works() {
+		new_test_ext().execute_with(|| {
+			let collection_id = 0;
+			let deposit_account = account(1);
+			let deposit_amount = balance_deposit();
+			let owner = account(2);
+			assert_noop!(
+				Nfts::increment_account_balance(
+					collection_id,
+					&owner,
+					(&deposit_account, deposit_amount)
+				),
+				BalancesError::<Test>::InsufficientBalance
+			);
+			Balances::make_free_balance_be(&deposit_account, 100);
+			// Initialize `AccountBalance` and increase the collection item count for the new
+			// account.
+			assert_ok!(Nfts::increment_account_balance(
+				collection_id,
+				&owner,
+				(&deposit_account, deposit_amount)
+			));
+			assert_eq!(Balances::reserved_balance(&deposit_account), deposit_amount);
+			assert_eq!(
+				AccountBalance::get(collection_id, &owner),
+				Some((1, (deposit_account.clone(), deposit_amount)))
+			);
+			// Increment the balance of a non-zero balance account. No additional reserves.
+			assert_ok!(Nfts::increment_account_balance(
+				collection_id,
+				&owner,
+				(&deposit_account, deposit_amount)
+			));
+			assert_eq!(Balances::reserved_balance(&deposit_account), deposit_amount);
+			assert_eq!(
+				AccountBalance::get(collection_id, &owner),
+				Some((2, (deposit_account.clone(), deposit_amount)))
+			);
+		});
+	}
+
+	#[test]
+	fn decrement_account_balance_works() {
+		new_test_ext().execute_with(|| {
+			let collection_id = 0;
+			let balance = 2u32;
+			let deposit_account = account(1);
+			let deposit_amount = balance_deposit();
+			let owner = account(2);
+
+			Balances::make_free_balance_be(&deposit_account, 100);
+			// Decrement non-existing `AccountBalance` record.
+			assert_noop!(
+				Nfts::decrement_account_balance(collection_id, &deposit_account),
+				Error::<Test>::NoItemOwned
+			);
+			// Set account balance and reserve `DepositBalance`.
+			AccountBalance::insert(
+				collection_id,
+				&owner,
+				(&balance, (&deposit_account, deposit_amount)),
+			);
+			Balances::reserve(&deposit_account, deposit_amount).expect("should work");
+			// Successfully decrement the value of the `AccountBalance` entry.
+			assert_ok!(Nfts::decrement_account_balance(collection_id, &owner));
+			assert_eq!(
+				AccountBalance::get(collection_id, &owner),
+				Some((balance - 1, (deposit_account.clone(), deposit_amount)))
+			);
+			assert_eq!(Balances::reserved_balance(&deposit_account), deposit_amount);
+			// `AccountBalance` record is deleted, and the depositor's funds are unreserved if
+			// the `AccountBalance` value reaches zero after decrementing.
+			assert_ok!(Nfts::decrement_account_balance(collection_id, &owner));
+			assert_eq!(Balances::reserved_balance(&deposit_account), 0);
+			assert!(!AccountBalance::contains_key(collection_id, &owner));
+		});
+	}
+}

pallets/nfts/src/features/approvals.rs

@@ -216,7 +216,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			Self::is_pallet_feature_enabled(PalletFeature::Approvals),
 			Error::<T, I>::MethodDisabled
 		);
-		ensure!(AccountBalance::<T, I>::get(collection, &owner) > 0, Error::<T, I>::NoItemOwned);
+		ensure!(
+			AccountBalance::<T, I>::get(collection, &owner)
+				.filter(|(balance, _)| !balance.is_zero())
+				.is_some(),
+			Error::<T, I>::NoItemOwned
+		);
 
 		let collection_config = Self::get_collection_config(&collection)?;
 		ensure!(

pallets/nfts/src/features/atomic_swap.rs

@@ -209,13 +209,20 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		}
 
 		// This also removes the swap.
-		Self::do_transfer(send_collection_id, send_item_id, receive_item.owner.clone(), |_, _| {
-			Ok(())
-		})?;
+		Self::do_transfer(
+			send_collection_id,
+			send_item_id,
+			receive_item.owner.clone(),
+			Some(&receive_item.owner),
+			|_, _| Ok(()),
+		)?;
+		// Owner of `send_item` is responsible for the deposit if the collection balance
+		// went to zero due to the previous transfer.
 		Self::do_transfer(
 			receive_collection_id,
 			receive_item_id,
 			send_item.owner.clone(),
+			Some(&send_item.owner),
 			|_, _| Ok(()),
 		)?;
 

pallets/nfts/src/features/buy_sell.rs

@@ -158,7 +158,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		let old_owner = details.owner.clone();
 
-		Self::do_transfer(collection, item, buyer.clone(), |_, _| Ok(()))?;
+		Self::do_transfer(collection, item, buyer.clone(), Some(&buyer), |_, _| Ok(()))?;
 
 		Self::deposit_event(Event::ItemBought {
 			collection,

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::*, sp_runtime::ArithmeticError, traits::ExistenceRequirement};
+use frame_support::{pallet_prelude::*, traits::ExistenceRequirement};
 
 use crate::*;
 
@@ -68,20 +68,20 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 			collection_details.items.saturating_inc();
 
-			AccountBalance::<T, I>::mutate(collection, &mint_to, |balance| {
-				balance.saturating_inc();
-			});
-
 			let collection_config = Self::get_collection_config(&collection)?;
-			let deposit_amount =
-				match collection_config.is_setting_enabled(CollectionSetting::DepositRequired) {
-					true => T::ItemDeposit::get(),
-					false => Zero::zero(),
-				};
-			let deposit_account = match maybe_depositor {
-				None => collection_details.owner.clone(),
-				Some(depositor) => depositor,
-			};
+			let deposit_required =
+				collection_config.is_setting_enabled(CollectionSetting::DepositRequired);
+			let deposit_account =
+				maybe_depositor.unwrap_or_else(|| collection_details.owner.clone());
+
+			let balance_deposit = deposit_required
+				.then_some(T::CollectionBalanceDeposit::get())
+				.unwrap_or_default();
+			Self::increment_account_balance(
+				collection,
+				&mint_to,
+				(&deposit_account, balance_deposit),
+			)?;
 
 			let item_owner = mint_to.clone();
 			Account::<T, I>::insert((&item_owner, &collection, &item), ());
@@ -93,9 +93,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				collection_details.item_configs.saturating_inc();
 			}
 
-			T::Currency::reserve(&deposit_account, deposit_amount)?;
+			let item_deposit =
+				deposit_required.then_some(T::ItemDeposit::get()).unwrap_or_default();
+			T::Currency::reserve(&deposit_account, item_deposit)?;
 
-			let deposit = ItemDeposit { account: deposit_account, amount: deposit_amount };
+			let deposit = ItemDeposit { account: deposit_account, amount: item_deposit };
 			let details = ItemDetails {
 				owner: item_owner,
 				approvals: ApprovalsOf::<T, I>::default(),
@@ -264,12 +266,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		PendingSwapOf::<T, I>::remove(collection, item);
 		ItemAttributesApprovalsOf::<T, I>::remove(collection, item);
 
-		let balance = AccountBalance::<T, I>::take(collection, &owner)
-			.checked_sub(1)
-			.ok_or(ArithmeticError::Underflow)?;
-		if balance > 0 {
-			AccountBalance::<T, I>::insert(collection, &owner, balance);
-		}
+		Self::decrement_account_balance(collection, &owner)?;
 
 		if remove_config {
 			ItemConfigOf::<T, I>::remove(collection, item);

pallets/nfts/src/features/transfer.rs

@@ -28,6 +28,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// - `collection`: The ID of the collection to which the NFT belongs.
 	/// - `item`: The ID of the NFT to transfer.
 	/// - `dest`: The destination account to which the NFT will be transferred.
+	/// - `depositor`: The account reserving the `CollectionBalanceDeposit` from if `dest` holds no
+	///   items in the collection.
 	/// - `with_details`: A closure that provides access to the collection and item details,
 	///   allowing customization of the transfer process.
 	///
@@ -48,6 +50,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		collection: T::CollectionId,
 		item: T::ItemId,
 		dest: T::AccountId,
+		depositor: Option<&T::AccountId>,
 		with_details: impl FnOnce(
 			&CollectionDetailsFor<T, I>,
 			&mut ItemDetailsFor<T, I>,
@@ -87,16 +90,18 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		with_details(&collection_details, &mut details)?;
 
 		// Update account balance of the owner.
-		let balance = AccountBalance::<T, I>::take(collection, &details.owner)
-			.checked_sub(1)
-			.ok_or(Error::<T, I>::NoItemOwned)?;
-		if balance > 0 {
-			AccountBalance::<T, I>::insert(collection, &details.owner, balance);
-		}
+		Self::decrement_account_balance(collection, &details.owner)?;
+
+		let deposit_amount = collection_config
+			.is_setting_enabled(CollectionSetting::DepositRequired)
+			.then_some(T::CollectionBalanceDeposit::get())
+			.unwrap_or_default();
+		// Reserve `CollectionBalanceDeposit` from the depositor if provided. Otherwise, reserve
+		// from the item's owner.
+		let deposit_account = depositor.unwrap_or(&details.owner);
+
 		// Update account balance of the destination account.
-		AccountBalance::<T, I>::mutate(collection, &dest, |balance| {
-			balance.saturating_inc();
-		});
+		Self::increment_account_balance(collection, &dest, (deposit_account, deposit_amount))?;
 
 		// Update account ownership information.
 		Account::<T, I>::remove((&details.owner, &collection, &item));

pallets/nfts/src/impl_nonfungibles.rs

@@ -411,7 +411,11 @@ impl<T: Config<I>, I: 'static> Transfer<T::AccountId> for Pallet<T, I> {
 		item: &Self::ItemId,
 		destination: &T::AccountId,
 	) -> DispatchResult {
-		Self::do_transfer(*collection, *item, destination.clone(), |_, _| Ok(()))
+		// The item's owner pays for the deposit of `AccountBalance` if the `dest` holds no items
+		// in `collection`. If `dest` paid the deposit, a malicious actor could transfer NFTs to
+		// reserve involuntary deposits for `dest` . The deposit amount can be accounted for in the
+		// off chain price of the NFT.
+		Self::do_transfer(*collection, *item, destination.clone(), None, |_, _| Ok(()))
 	}
 
 	fn disable_transfer(collection: &Self::CollectionId, item: &Self::ItemId) -> DispatchResult {