refactor: account balance (#421)

Author: Daanvdplas

pallets/nfts/src/common_functions.rs

@@ -151,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/atomic_swap.rs

@@ -210,14 +210,16 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		// This also removes the swap.
 		Self::do_transfer(
-			Some(&caller),
+			Some(&receive_item.owner),
 			send_collection_id,
 			send_item_id,
 			receive_item.owner.clone(),
 			|_, _| 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(
-			Some(&caller),
+			Some(&send_item.owner),
 			receive_collection_id,
 			receive_item_id,
 			send_item.owner.clone(),

pallets/nfts/src/features/transfer.rs

@@ -25,7 +25,8 @@ use crate::*;
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// Transfer an NFT to the specified destination account.
 	///
-	/// - `caller`: The account transferring the collection item.
+	/// - `depositor`: The account reserving the `CollectionBalanceDeposit` from if `dest` holds no
+	///   items in the collection.
 	/// - `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.
@@ -46,7 +47,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// - If the collection or item is non-transferable
 	///   ([`ItemsNonTransferable`](crate::Error::ItemsNonTransferable)).
 	pub fn do_transfer(
-		caller: Option<&T::AccountId>,
+		depositor: Option<&T::AccountId>,
 		collection: T::CollectionId,
 		item: T::ItemId,
 		dest: T::AccountId,
@@ -91,16 +92,15 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		// Update account balance of the owner.
 		Self::decrement_account_balance(collection, &details.owner)?;
 
-		// Update account balance of the destination account.
-		let deposit_amount =
-			match collection_config.is_setting_enabled(CollectionSetting::DepositRequired) {
-				true => T::CollectionBalanceDeposit::get(),
-				false => Zero::zero(),
-			};
-		// Reserve `CollectionBalanceDeposit` from the caller if provided. Otherwise, reserve from
-		// the collection item's owner.
-		let deposit_account = caller.unwrap_or(&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.
 		Self::increment_account_balance(collection, &dest, (deposit_account, deposit_amount))?;
 
 		// Update account ownership information.

pallets/nfts/src/impl_nonfungibles.rs

@@ -411,6 +411,10 @@ impl<T: Config<I>, I: 'static> Transfer<T::AccountId> for Pallet<T, I> {
 		item: &Self::ItemId,
 		destination: &T::AccountId,
 	) -> DispatchResult {
+		// The item's owner pays for the deposit of `AccountBalance` if the `dest` holds no items
+		// in `collection`. A malicious actor could have a deposit reserved from `dest` without
+		// them knowing about the transfer. The deposit amount can be accounted for in the off chain
+		// price of the NFT.
 		Self::do_transfer(None, *collection, *item, destination.clone(), |_, _| Ok(()))
 	}
 

pallets/nfts/src/lib.rs

@@ -1092,6 +1092,10 @@ pub mod pallet {
 			let origin = ensure_signed(origin)?;
 			let dest = T::Lookup::lookup(dest)?;
 
+			// The item's owner pays for the deposit of `AccountBalance` if the `dest` holds no
+			// items in `collection`. A malicious actor could have a deposit reserved from `dest`
+			// without them knowing about the transfer. The deposit amount can be accounted for
+			// in the off chain price of the NFT.
 			Self::do_transfer(None, collection, item, dest, |_, details| {
 				if details.owner != origin {
 					Self::check_approval(&collection, &Some(item), &details.owner, &origin)?;