[NFTs] Track item's metadata depositor (#13124)

* Refactor do_mint()

* Track the depositor of item's metadata

* Revert back the access control

* On collection destroy return the metadata deposit

* Clear the metadata on item burn returning the deposit

* Address comments

* Fix clippy

* Don't return Ok on non-existing attribute removal

substrate/frame/nfts/src/features/attributes.rs

@@ -152,65 +152,68 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		namespace: AttributeNamespace<T::AccountId>,
 		key: BoundedVec<u8, T::KeyLimit>,
 	) -> DispatchResult {
-		if let Some((_, deposit)) =
-			Attribute::<T, I>::take((collection, maybe_item, &namespace, &key))
-		{
-			let mut collection_details =
-				Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
+		let (_, deposit) = Attribute::<T, I>::take((collection, maybe_item, &namespace, &key))
+			.ok_or(Error::<T, I>::AttributeNotFound)?;
+		let mut collection_details =
+			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
 
-			if let Some(check_owner) = &maybe_check_owner {
-				if deposit.account != maybe_check_owner {
-					ensure!(
-						Self::is_valid_namespace(
-							&check_owner,
-							&namespace,
-							&collection,
-							&collection_details.owner,
-							&maybe_item,
-						)?,
-						Error::<T, I>::NoPermission
-					);
-				}
-
-				// can't clear `CollectionOwner` type attributes if the collection/item is locked
-				match namespace {
-					AttributeNamespace::CollectionOwner => match maybe_item {
-						None => {
-							let collection_config = Self::get_collection_config(&collection)?;
-							ensure!(
-								collection_config
-									.is_setting_enabled(CollectionSetting::UnlockedAttributes),
-								Error::<T, I>::LockedCollectionAttributes
-							)
-						},
-						Some(item) => {
-							// NOTE: if the item was previously burned, the ItemConfigOf record
-							// might not exist. In that case, we allow to clear the attribute.
-							let maybe_is_locked = Self::get_item_config(&collection, &item)
-								.map_or(false, |c| {
-									c.has_disabled_setting(ItemSetting::UnlockedAttributes)
-								});
-							ensure!(!maybe_is_locked, Error::<T, I>::LockedItemAttributes);
-						},
-					},
-					_ => (),
-				};
+		if let Some(check_owner) = &maybe_check_owner {
+			// validate the provided namespace when it's not a root call and the caller is not
+			// the same as the `deposit.account` (e.g. the deposit was paid by different account)
+			if deposit.account != maybe_check_owner {
+				ensure!(
+					Self::is_valid_namespace(
+						&check_owner,
+						&namespace,
+						&collection,
+						&collection_details.owner,
+						&maybe_item,
+					)?,
+					Error::<T, I>::NoPermission
+				);
 			}
 
-			collection_details.attributes.saturating_dec();
+			// can't clear `CollectionOwner` type attributes if the collection/item is locked
 			match namespace {
-				AttributeNamespace::CollectionOwner => {
-					collection_details.owner_deposit.saturating_reduce(deposit.amount);
-					T::Currency::unreserve(&collection_details.owner, deposit.amount);
+				AttributeNamespace::CollectionOwner => match maybe_item {
+					None => {
+						let collection_config = Self::get_collection_config(&collection)?;
+						ensure!(
+							collection_config
+								.is_setting_enabled(CollectionSetting::UnlockedAttributes),
+							Error::<T, I>::LockedCollectionAttributes
+						)
+					},
+					Some(item) => {
+						// NOTE: if the item was previously burned, the ItemConfigOf record
+						// might not exist. In that case, we allow to clear the attribute.
+						let maybe_is_locked = Self::get_item_config(&collection, &item)
+							.map_or(false, |c| {
+								c.has_disabled_setting(ItemSetting::UnlockedAttributes)
+							});
+						ensure!(!maybe_is_locked, Error::<T, I>::LockedItemAttributes);
+					},
 				},
 				_ => (),
 			};
-			if let Some(deposit_account) = deposit.account {
-				T::Currency::unreserve(&deposit_account, deposit.amount);
-			}
-			Collection::<T, I>::insert(collection, &collection_details);
-			Self::deposit_event(Event::AttributeCleared { collection, maybe_item, key, namespace });
 		}
+
+		collection_details.attributes.saturating_dec();
+		match namespace {
+			AttributeNamespace::CollectionOwner => {
+				collection_details.owner_deposit.saturating_reduce(deposit.amount);
+				T::Currency::unreserve(&collection_details.owner, deposit.amount);
+			},
+			_ => (),
+		};
+
+		if let Some(deposit_account) = deposit.account {
+			T::Currency::unreserve(&deposit_account, deposit.amount);
+		}
+
+		Collection::<T, I>::insert(collection, &collection_details);
+		Self::deposit_event(Event::AttributeCleared { collection, maybe_item, key, namespace });
+
 		Ok(())
 	}
 

substrate/frame/nfts/src/features/create_delete_collection.rs

@@ -82,12 +82,13 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				Account::<T, I>::remove((&details.owner, &collection, &item));
 				T::Currency::unreserve(&details.deposit.account, details.deposit.amount);
 			}
-			#[allow(deprecated)]
-			ItemMetadataOf::<T, I>::remove_prefix(&collection, None);
-			#[allow(deprecated)]
-			ItemPriceOf::<T, I>::remove_prefix(&collection, None);
-			#[allow(deprecated)]
-			PendingSwapOf::<T, I>::remove_prefix(&collection, None);
+			for (_, metadata) in ItemMetadataOf::<T, I>::drain_prefix(&collection) {
+				if let Some(depositor) = metadata.deposit.account {
+					T::Currency::unreserve(&depositor, metadata.deposit.amount);
+				}
+			}
+			let _ = ItemPriceOf::<T, I>::clear_prefix(&collection, witness.items, None);
+			let _ = PendingSwapOf::<T, I>::clear_prefix(&collection, witness.items, None);
 			CollectionMetadataOf::<T, I>::remove(&collection);
 			Self::clear_roles(&collection)?;
 

substrate/frame/nfts/src/features/create_delete_item.rs

@@ -22,10 +22,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	pub fn do_mint(
 		collection: T::CollectionId,
 		item: T::ItemId,
-		depositor: T::AccountId,
+		maybe_depositor: Option<T::AccountId>,
 		mint_to: T::AccountId,
 		item_config: ItemConfig,
-		deposit_collection_owner: bool,
 		with_details_and_config: impl FnOnce(
 			&CollectionDetailsFor<T, I>,
 			&CollectionConfigFor<T, I>,
@@ -55,9 +54,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 					true => T::ItemDeposit::get(),
 					false => Zero::zero(),
 				};
-				let deposit_account = match deposit_collection_owner {
-					true => collection_details.owner.clone(),
-					false => depositor,
+				let deposit_account = match maybe_depositor {
+					None => collection_details.owner.clone(),
+					Some(depositor) => depositor,
 				};
 
 				let item_owner = mint_to.clone();
@@ -92,6 +91,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		with_details: impl FnOnce(&ItemDetailsFor<T, I>) -> DispatchResult,
 	) -> DispatchResult {
 		ensure!(!T::Locker::is_locked(collection, item), Error::<T, I>::ItemLocked);
+		let item_config = Self::get_item_config(&collection, &item)?;
 		let owner = Collection::<T, I>::try_mutate(
 			&collection,
 			|maybe_collection_details| -> Result<T::AccountId, DispatchError> {
@@ -104,6 +104,24 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				// Return the deposit.
 				T::Currency::unreserve(&details.deposit.account, details.deposit.amount);
 				collection_details.items.saturating_dec();
+
+				// Clear the metadata if it's not locked.
+				if item_config.is_setting_enabled(ItemSetting::UnlockedMetadata) {
+					if let Some(metadata) = ItemMetadataOf::<T, I>::take(&collection, &item) {
+						let depositor_account =
+							metadata.deposit.account.unwrap_or(collection_details.owner.clone());
+
+						T::Currency::unreserve(&depositor_account, metadata.deposit.amount);
+						collection_details.item_metadatas.saturating_dec();
+
+						if depositor_account == collection_details.owner {
+							collection_details
+								.owner_deposit
+								.saturating_reduce(metadata.deposit.amount);
+						}
+					}
+				}
+
 				Ok(details.owner)
 			},
 		)?;
@@ -116,8 +134,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		// NOTE: if item's settings are not empty (e.g. item's metadata is locked)
 		// then we keep the record and don't remove it
-		let config = Self::get_item_config(&collection, &item)?;
-		if !config.has_disabled_settings() {
+		if !item_config.has_disabled_settings() {
 			ItemConfigOf::<T, I>::remove(&collection, &item);
 		}
 

substrate/frame/nfts/src/features/metadata.rs

@@ -19,19 +19,21 @@ use crate::*;
 use frame_support::pallet_prelude::*;
 
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
+	/// Note: if `maybe_depositor` is None, that means the depositor will be a collection's owner
 	pub(crate) fn do_set_item_metadata(
 		maybe_check_owner: Option<T::AccountId>,
 		collection: T::CollectionId,
 		item: T::ItemId,
 		data: BoundedVec<u8, T::StringLimit>,
+		maybe_depositor: Option<T::AccountId>,
 	) -> DispatchResult {
+		let is_root = maybe_check_owner.is_none();
 		let mut collection_details =
 			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
 
 		let item_config = Self::get_item_config(&collection, &item)?;
 		ensure!(
-			maybe_check_owner.is_none() ||
-				item_config.is_setting_enabled(ItemSetting::UnlockedMetadata),
+			is_root || item_config.is_setting_enabled(ItemSetting::UnlockedMetadata),
 			Error::<T, I>::LockedItemMetadata
 		);
 
@@ -45,24 +47,38 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			if metadata.is_none() {
 				collection_details.item_metadatas.saturating_inc();
 			}
-			let old_deposit = metadata.take().map_or(Zero::zero(), |m| m.deposit);
-			collection_details.owner_deposit.saturating_reduce(old_deposit);
+
+			let old_deposit = metadata
+				.take()
+				.map_or(ItemMetadataDeposit { account: None, amount: Zero::zero() }, |m| m.deposit);
+
 			let mut deposit = Zero::zero();
-			if collection_config.is_setting_enabled(CollectionSetting::DepositRequired) &&
-				maybe_check_owner.is_some()
+			if collection_config.is_setting_enabled(CollectionSetting::DepositRequired) && !is_root
 			{
 				deposit = T::DepositPerByte::get()
 					.saturating_mul(((data.len()) as u32).into())
 					.saturating_add(T::MetadataDepositBase::get());
 			}
-			if deposit > old_deposit {
-				T::Currency::reserve(&collection_details.owner, deposit - old_deposit)?;
-			} else if deposit < old_deposit {
-				T::Currency::unreserve(&collection_details.owner, old_deposit - deposit);
-			}
-			collection_details.owner_deposit.saturating_accrue(deposit);
 
-			*metadata = Some(ItemMetadata { deposit, data: data.clone() });
+			// the previous deposit was taken from the item's owner
+			if old_deposit.account.is_some() && maybe_depositor.is_none() {
+				T::Currency::unreserve(&old_deposit.account.unwrap(), old_deposit.amount);
+				T::Currency::reserve(&collection_details.owner, deposit)?;
+			} else if deposit > old_deposit.amount {
+				T::Currency::reserve(&collection_details.owner, deposit - old_deposit.amount)?;
+			} else if deposit < old_deposit.amount {
+				T::Currency::unreserve(&collection_details.owner, old_deposit.amount - deposit);
+			}
+
+			if maybe_depositor.is_none() {
+				collection_details.owner_deposit.saturating_accrue(deposit);
+				collection_details.owner_deposit.saturating_reduce(old_deposit.amount);
+			}
+
+			*metadata = Some(ItemMetadata {
+				deposit: ItemMetadataDeposit { account: maybe_depositor, amount: deposit },
+				data: data.clone(),
+			});
 
 			Collection::<T, I>::insert(&collection, &collection_details);
 			Self::deposit_event(Event::ItemMetadataSet { collection, item, data });
@@ -75,8 +91,14 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		collection: T::CollectionId,
 		item: T::ItemId,
 	) -> DispatchResult {
+		let is_root = maybe_check_owner.is_none();
+		let metadata = ItemMetadataOf::<T, I>::take(collection, item)
+			.ok_or(Error::<T, I>::MetadataNotFound)?;
 		let mut collection_details =
 			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
+		let depositor_account =
+			metadata.deposit.account.unwrap_or(collection_details.owner.clone());
+
 		if let Some(check_owner) = &maybe_check_owner {
 			ensure!(check_owner == &collection_details.owner, Error::<T, I>::NoPermission);
 		}
@@ -85,20 +107,19 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		let is_locked = Self::get_item_config(&collection, &item)
 			.map_or(false, |c| c.has_disabled_setting(ItemSetting::UnlockedMetadata));
 
-		ensure!(maybe_check_owner.is_none() || !is_locked, Error::<T, I>::LockedItemMetadata);
+		ensure!(is_root || !is_locked, Error::<T, I>::LockedItemMetadata);
 
-		ItemMetadataOf::<T, I>::try_mutate_exists(collection, item, |metadata| {
-			if metadata.is_some() {
-				collection_details.item_metadatas.saturating_dec();
-			}
-			let deposit = metadata.take().ok_or(Error::<T, I>::UnknownItem)?.deposit;
-			T::Currency::unreserve(&collection_details.owner, deposit);
-			collection_details.owner_deposit.saturating_reduce(deposit);
+		collection_details.item_metadatas.saturating_dec();
+		T::Currency::unreserve(&depositor_account, metadata.deposit.amount);
 
-			Collection::<T, I>::insert(&collection, &collection_details);
-			Self::deposit_event(Event::ItemMetadataCleared { collection, item });
-			Ok(())
-		})
+		if depositor_account == collection_details.owner {
+			collection_details.owner_deposit.saturating_reduce(metadata.deposit.amount);
+		}
+
+		Collection::<T, I>::insert(&collection, &collection_details);
+		Self::deposit_event(Event::ItemMetadataCleared { collection, item });
+
+		Ok(())
 	}
 
 	pub(crate) fn do_set_collection_metadata(

substrate/frame/nfts/src/impl_nonfungibles.rs

@@ -157,10 +157,12 @@ impl<T: Config<I>, I: 'static> Mutate<<T as SystemConfig>::AccountId, ItemConfig
 		Self::do_mint(
 			*collection,
 			*item,
-			who.clone(),
+			match deposit_collection_owner {
+				true => None,
+				false => Some(who.clone()),
+			},
 			who.clone(),
 			*item_config,
-			deposit_collection_owner,
 			|_, _| Ok(()),
 		)
 	}

substrate/frame/nfts/src/lib.rs

@@ -263,7 +263,7 @@ pub mod pallet {
 		T::CollectionId,
 		Blake2_128Concat,
 		T::ItemId,
-		ItemMetadata<DepositBalanceOf<T, I>, T::StringLimit>,
+		ItemMetadata<ItemMetadataDepositOf<T, I>, T::StringLimit>,
 		OptionQuery,
 	>;
 
@@ -559,6 +559,10 @@ pub mod pallet {
 		UnknownItem,
 		/// Swap doesn't exist.
 		UnknownSwap,
+		/// The given item has no metadata set.
+		MetadataNotFound,
+		/// The provided attribute can't be found.
+		AttributeNotFound,
 		/// Item is not for sale.
 		NotForSale,
 		/// The provided bid is too low.
@@ -746,10 +750,9 @@ pub mod pallet {
 			Self::do_mint(
 				collection,
 				item,
-				caller.clone(),
+				Some(caller.clone()),
 				mint_to.clone(),
 				item_config,
-				false,
 				|collection_details, collection_config| {
 					// Issuer can mint regardless of mint settings
 					if Self::has_role(&collection, &caller, CollectionRole::Issuer) {
@@ -849,9 +852,7 @@ pub mod pallet {
 					Error::<T, I>::NoPermission
 				);
 			}
-			Self::do_mint(collection, item, mint_to.clone(), mint_to, item_config, true, |_, _| {
-				Ok(())
-			})
+			Self::do_mint(collection, item, None, mint_to, item_config, |_, _| Ok(()))
 		}
 
 		/// Destroy a single item.
@@ -1362,7 +1363,7 @@ pub mod pallet {
 		/// Clear an attribute for a collection or item.
 		///
 		/// Origin must be either `ForceOrigin` or Signed and the sender should be the Owner of the
-		/// `collection`.
+		/// attribute.
 		///
 		/// Any deposit is freed for the collection's owner.
 		///
@@ -1464,7 +1465,7 @@ pub mod pallet {
 			let maybe_check_owner = T::ForceOrigin::try_origin(origin)
 				.map(|_| None)
 				.or_else(|origin| ensure_signed(origin).map(Some).map_err(DispatchError::from))?;
-			Self::do_set_item_metadata(maybe_check_owner, collection, item, data)
+			Self::do_set_item_metadata(maybe_check_owner, collection, item, data, None)
 		}
 
 		/// Clear the metadata for an item.

substrate/frame/nfts/src/tests.rs

@@ -608,7 +608,7 @@ fn set_item_metadata_should_work() {
 		);
 		assert_noop!(
 			Nfts::clear_metadata(RuntimeOrigin::signed(1), 1, 42),
-			Error::<Test>::UnknownCollection,
+			Error::<Test>::MetadataNotFound,
 		);
 		assert_ok!(Nfts::clear_metadata(RuntimeOrigin::root(), 0, 42));
 		assert!(!ItemMetadataOf::<Test>::contains_key(0, 42));
@@ -1267,7 +1267,7 @@ fn burn_works() {
 
 		assert_noop!(
 			Nfts::burn(RuntimeOrigin::signed(5), 0, 42, Some(5)),
-			Error::<Test>::UnknownCollection
+			Error::<Test>::UnknownItem
 		);
 
 		assert_ok!(Nfts::force_mint(RuntimeOrigin::signed(2), 0, 42, 5, default_item_config()));

substrate/frame/nfts/src/types.rs

@@ -43,6 +43,8 @@ pub(super) type ItemDepositOf<T, I> =
 	ItemDeposit<DepositBalanceOf<T, I>, <T as SystemConfig>::AccountId>;
 pub(super) type AttributeDepositOf<T, I> =
 	AttributeDeposit<DepositBalanceOf<T, I>, <T as SystemConfig>::AccountId>;
+pub(super) type ItemMetadataDepositOf<T, I> =
+	ItemMetadataDeposit<DepositBalanceOf<T, I>, <T as SystemConfig>::AccountId>;
 pub(super) type ItemDetailsFor<T, I> =
 	ItemDetails<<T as SystemConfig>::AccountId, ItemDepositOf<T, I>, ApprovalsOf<T, I>>;
 pub(super) type BalanceOf<T, I = ()> =
@@ -137,12 +139,12 @@ pub struct ItemDeposit<DepositBalance, AccountId> {
 /// Information about the collection's metadata.
 #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, Default, TypeInfo, MaxEncodedLen)]
 #[scale_info(skip_type_params(StringLimit))]
-#[codec(mel_bound(DepositBalance: MaxEncodedLen))]
-pub struct CollectionMetadata<DepositBalance, StringLimit: Get<u32>> {
+#[codec(mel_bound(Deposit: MaxEncodedLen))]
+pub struct CollectionMetadata<Deposit, StringLimit: Get<u32>> {
 	/// The balance deposited for this metadata.
 	///
 	/// This pays for the data stored in this struct.
-	pub(super) deposit: DepositBalance,
+	pub(super) deposit: Deposit,
 	/// General information concerning this collection. Limited in length by `StringLimit`. This
 	/// will generally be either a JSON dump or the hash of some JSON which can be found on a
 	/// hash-addressable global publication system such as IPFS.
@@ -152,12 +154,11 @@ pub struct CollectionMetadata<DepositBalance, StringLimit: Get<u32>> {
 /// Information about the item's metadata.
 #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, Default, TypeInfo, MaxEncodedLen)]
 #[scale_info(skip_type_params(StringLimit))]
-#[codec(mel_bound(DepositBalance: MaxEncodedLen))]
-pub struct ItemMetadata<DepositBalance, StringLimit: Get<u32>> {
+pub struct ItemMetadata<Deposit, StringLimit: Get<u32>> {
 	/// The balance deposited for this metadata.
 	///
 	/// This pays for the data stored in this struct.
-	pub(super) deposit: DepositBalance,
+	pub(super) deposit: Deposit,
 	/// General information concerning this item. Limited in length by `StringLimit`. This will
 	/// generally be either a JSON dump or the hash of some JSON which can be found on a
 	/// hash-addressable global publication system such as IPFS.
@@ -199,6 +200,15 @@ pub struct AttributeDeposit<DepositBalance, AccountId> {
 	pub(super) amount: DepositBalance,
 }
 
+/// Information about the reserved item's metadata deposit.
+#[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
+pub struct ItemMetadataDeposit<DepositBalance, AccountId> {
+	/// A depositor account, None means the deposit is collection's owner.
+	pub(super) account: Option<AccountId>,
+	/// An amount that gets reserved.
+	pub(super) amount: DepositBalance,
+}
+
 /// Specifies whether the tokens will be sent or received.
 #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
 pub enum PriceDirection {