[NFTs] Track item's metadata depositor (paritytech#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

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)?;
-
-			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
-					);
-				}
+		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)?;
 
-				// 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(())
 	}
 

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)?;
 

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);
 		}
 

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);
+
+			// 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);
 			}
-			collection_details.owner_deposit.saturating_accrue(deposit);
 
-			*metadata = Some(ItemMetadata { deposit, data: data.clone() });
+			*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(

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(()),
 		)
 	}