chore: resolve feedback comments

Author: chungquantin

pallets/nfts/src/features/atomic_swap.rs

@@ -210,19 +210,19 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		// This also removes the swap.
 		Self::do_transfer(
-			Some(&receive_item.owner),
 			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(
-			Some(&send_item.owner),
 			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(Some(&buyer), 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/transfer.rs

@@ -25,11 +25,11 @@ use crate::*;
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// Transfer an NFT to the specified destination account.
 	///
-	/// - `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.
+	/// - `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.
 	///
@@ -47,10 +47,10 @@ 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(
-		depositor: Option<&T::AccountId>,
 		collection: T::CollectionId,
 		item: T::ItemId,
 		dest: T::AccountId,
+		depositor: Option<&T::AccountId>,
 		with_details: impl FnOnce(
 			&CollectionDetailsFor<T, I>,
 			&mut ItemDetailsFor<T, I>,

pallets/nfts/src/impl_nonfungibles.rs

@@ -412,10 +412,10 @@ impl<T: Config<I>, I: 'static> Transfer<T::AccountId> for Pallet<T, I> {
 		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(()))
+		// 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 {

pallets/nfts/src/lib.rs

@@ -1096,7 +1096,7 @@ pub mod pallet {
 			// 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| {
+			Self::do_transfer(collection, item, dest, None, |_, details| {
 				if details.owner != origin {
 					Self::check_approval(&collection, &Some(item), &details.owner, &origin)?;
 				}