From b5f22c11d57ff175ab6baeaea16ed6f530be98af Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Tue, 10 Jan 2023 16:29:41 +0200 Subject: [PATCH 1/8] Refactor do_mint() --- frame/nfts/src/features/create_delete_item.rs | 9 ++++----- frame/nfts/src/impl_nonfungibles.rs | 6 ++++-- frame/nfts/src/lib.rs | 7 ++----- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/frame/nfts/src/features/create_delete_item.rs b/frame/nfts/src/features/create_delete_item.rs index 8d6c8a280956c..8ba012a63b606 100644 --- a/frame/nfts/src/features/create_delete_item.rs +++ b/frame/nfts/src/features/create_delete_item.rs @@ -22,10 +22,9 @@ impl, I: 'static> Pallet { pub fn do_mint( collection: T::CollectionId, item: T::ItemId, - depositor: T::AccountId, + maybe_depositor: Option, mint_to: T::AccountId, item_config: ItemConfig, - deposit_collection_owner: bool, with_details_and_config: impl FnOnce( &CollectionDetailsFor, &CollectionConfigFor, @@ -55,9 +54,9 @@ impl, I: 'static> Pallet { 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(); diff --git a/frame/nfts/src/impl_nonfungibles.rs b/frame/nfts/src/impl_nonfungibles.rs index ab4d7a16ec215..86d82b4021517 100644 --- a/frame/nfts/src/impl_nonfungibles.rs +++ b/frame/nfts/src/impl_nonfungibles.rs @@ -157,10 +157,12 @@ impl, I: 'static> Mutate<::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(()), ) } diff --git a/frame/nfts/src/lib.rs b/frame/nfts/src/lib.rs index 9a6242143aedb..c89ebf7d89713 100644 --- a/frame/nfts/src/lib.rs +++ b/frame/nfts/src/lib.rs @@ -746,10 +746,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 +848,7 @@ pub mod pallet { Error::::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. From b4b6243a3fc75e67ecdabc926b7821ccd854f2ea Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Tue, 10 Jan 2023 19:05:44 +0200 Subject: [PATCH 2/8] Track the depositor of item's metadata --- frame/nfts/src/features/attributes.rs | 1 + frame/nfts/src/features/metadata.rs | 76 ++++++++++++++++++--------- frame/nfts/src/lib.rs | 10 ++-- frame/nfts/src/tests.rs | 2 +- frame/nfts/src/types.rs | 22 +++++--- 5 files changed, 74 insertions(+), 37 deletions(-) diff --git a/frame/nfts/src/features/attributes.rs b/frame/nfts/src/features/attributes.rs index da663d39a4ef5..7f2038488c746 100644 --- a/frame/nfts/src/features/attributes.rs +++ b/frame/nfts/src/features/attributes.rs @@ -159,6 +159,7 @@ impl, I: 'static> Pallet { Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; if let Some(check_owner) = &maybe_check_owner { + // deposit.account.is_none(); maybe_check_owner.is_some() if deposit.account != maybe_check_owner { ensure!( Self::is_valid_namespace( diff --git a/frame/nfts/src/features/metadata.rs b/frame/nfts/src/features/metadata.rs index 942f377141a33..923402423eec7 100644 --- a/frame/nfts/src/features/metadata.rs +++ b/frame/nfts/src/features/metadata.rs @@ -19,19 +19,21 @@ use crate::*; use frame_support::pallet_prelude::*; impl, I: 'static> Pallet { + /// 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, collection: T::CollectionId, item: T::ItemId, data: BoundedVec, + maybe_depositor: Option, ) -> DispatchResult { + let is_root = maybe_check_owner.is_none(); let mut collection_details = Collection::::get(&collection).ok_or(Error::::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::::LockedItemMetadata ); @@ -45,24 +47,38 @@ impl, I: 'static> Pallet { 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); } - collection_details.owner_deposit.saturating_accrue(deposit); - *metadata = Some(ItemMetadata { deposit, data: data.clone() }); + 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::::insert(&collection, &collection_details); Self::deposit_event(Event::ItemMetadataSet { collection, item, data }); @@ -75,30 +91,38 @@ impl, I: 'static> Pallet { collection: T::CollectionId, item: T::ItemId, ) -> DispatchResult { + let is_root = maybe_check_owner.is_none(); + let metadata = ItemMetadataOf::::take(collection, item) + .ok_or(Error::::MetadataNotFound)?; let mut collection_details = Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; - if let Some(check_owner) = &maybe_check_owner { - ensure!(check_owner == &collection_details.owner, Error::::NoPermission); + let depositor_account = + metadata.deposit.account.unwrap_or(collection_details.owner.clone()); + + if !is_root { + ensure!( + Some(depositor_account.clone()) == maybe_check_owner, + Error::::NoPermission + ); } // NOTE: if the item was previously burned, the ItemConfigOf record might not exist 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::::LockedItemMetadata); + ensure!(is_root || !is_locked, Error::::LockedItemMetadata); - ItemMetadataOf::::try_mutate_exists(collection, item, |metadata| { - if metadata.is_some() { - collection_details.item_metadatas.saturating_dec(); - } - let deposit = metadata.take().ok_or(Error::::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.clone()); - Collection::::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::::insert(&collection, &collection_details); + Self::deposit_event(Event::ItemMetadataCleared { collection, item }); + + Ok(()) } pub(crate) fn do_set_collection_metadata( diff --git a/frame/nfts/src/lib.rs b/frame/nfts/src/lib.rs index c89ebf7d89713..c0f3c7bc43c8f 100644 --- a/frame/nfts/src/lib.rs +++ b/frame/nfts/src/lib.rs @@ -263,7 +263,7 @@ pub mod pallet { T::CollectionId, Blake2_128Concat, T::ItemId, - ItemMetadata, T::StringLimit>, + ItemMetadata, T::StringLimit>, OptionQuery, >; @@ -559,6 +559,8 @@ pub mod pallet { UnknownItem, /// Swap doesn't exist. UnknownSwap, + /// The given item has no metadata set. + MetadataNotFound, /// Item is not for sale. NotForSale, /// The provided bid is too low. @@ -1359,7 +1361,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. /// @@ -1461,13 +1463,13 @@ 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. /// /// Origin must be either `ForceOrigin` or Signed and the sender should be the Owner of the - /// `collection`. + /// metadata. /// /// Any deposit is freed for the collection's owner. /// diff --git a/frame/nfts/src/tests.rs b/frame/nfts/src/tests.rs index f8187b3777ebd..2b0c26962e5cd 100644 --- a/frame/nfts/src/tests.rs +++ b/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::::UnknownCollection, + Error::::MetadataNotFound, ); assert_ok!(Nfts::clear_metadata(RuntimeOrigin::root(), 0, 42)); assert!(!ItemMetadataOf::::contains_key(0, 42)); diff --git a/frame/nfts/src/types.rs b/frame/nfts/src/types.rs index 9b89fb964b316..d8938aab4377a 100644 --- a/frame/nfts/src/types.rs +++ b/frame/nfts/src/types.rs @@ -43,6 +43,8 @@ pub(super) type ItemDepositOf = ItemDeposit, ::AccountId>; pub(super) type AttributeDepositOf = AttributeDeposit, ::AccountId>; +pub(super) type ItemMetadataDepositOf = + ItemMetadataDeposit, ::AccountId>; pub(super) type ItemDetailsFor = ItemDetails<::AccountId, ItemDepositOf, ApprovalsOf>; pub(super) type BalanceOf = @@ -137,12 +139,12 @@ pub struct ItemDeposit { /// 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> { +#[codec(mel_bound(Deposit: MaxEncodedLen))] +pub struct CollectionMetadata> { /// 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> { /// 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> { +pub struct ItemMetadata> { /// 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 { pub(super) amount: DepositBalance, } +/// Information about the reserved item's metadata deposit. +#[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, TypeInfo, MaxEncodedLen)] +pub struct ItemMetadataDeposit { + /// A depositor account, None means the deposit is collection's owner. + pub(super) account: Option, + /// 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 { From ed295ee4170ab42bacf9e7581a1c7ddfdbca3387 Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Wed, 11 Jan 2023 11:34:40 +0200 Subject: [PATCH 3/8] Revert back the access control --- frame/nfts/src/features/metadata.rs | 7 ++----- frame/nfts/src/lib.rs | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/frame/nfts/src/features/metadata.rs b/frame/nfts/src/features/metadata.rs index 923402423eec7..fde5d4abd7bd1 100644 --- a/frame/nfts/src/features/metadata.rs +++ b/frame/nfts/src/features/metadata.rs @@ -99,11 +99,8 @@ impl, I: 'static> Pallet { let depositor_account = metadata.deposit.account.unwrap_or(collection_details.owner.clone()); - if !is_root { - ensure!( - Some(depositor_account.clone()) == maybe_check_owner, - Error::::NoPermission - ); + if let Some(check_owner) = &maybe_check_owner { + ensure!(check_owner == &collection_details.owner, Error::::NoPermission); } // NOTE: if the item was previously burned, the ItemConfigOf record might not exist diff --git a/frame/nfts/src/lib.rs b/frame/nfts/src/lib.rs index c0f3c7bc43c8f..1e56de21c3518 100644 --- a/frame/nfts/src/lib.rs +++ b/frame/nfts/src/lib.rs @@ -1469,7 +1469,7 @@ pub mod pallet { /// Clear the metadata for an item. /// /// Origin must be either `ForceOrigin` or Signed and the sender should be the Owner of the - /// metadata. + /// `collection`. /// /// Any deposit is freed for the collection's owner. /// From 1dbc03a8254972f578450f37b4463c0bcaff7f4a Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Wed, 11 Jan 2023 11:35:39 +0200 Subject: [PATCH 4/8] On collection destroy return the metadata deposit --- frame/nfts/src/features/create_delete_collection.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/frame/nfts/src/features/create_delete_collection.rs b/frame/nfts/src/features/create_delete_collection.rs index 86625bf49efb2..a2caa04bc8dc3 100644 --- a/frame/nfts/src/features/create_delete_collection.rs +++ b/frame/nfts/src/features/create_delete_collection.rs @@ -82,12 +82,13 @@ impl, I: 'static> Pallet { Account::::remove((&details.owner, &collection, &item)); T::Currency::unreserve(&details.deposit.account, details.deposit.amount); } - #[allow(deprecated)] - ItemMetadataOf::::remove_prefix(&collection, None); - #[allow(deprecated)] - ItemPriceOf::::remove_prefix(&collection, None); - #[allow(deprecated)] - PendingSwapOf::::remove_prefix(&collection, None); + for (_, metadata) in ItemMetadataOf::::drain_prefix(&collection) { + if let Some(depositor) = metadata.deposit.account { + T::Currency::unreserve(&depositor, metadata.deposit.amount); + } + } + let _ = ItemPriceOf::::clear_prefix(&collection, witness.items, None); + let _ = PendingSwapOf::::clear_prefix(&collection, witness.items, None); CollectionMetadataOf::::remove(&collection); Self::clear_roles(&collection)?; From 59c80fbdc7d70ef427a523dad5461857d281d276 Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Wed, 11 Jan 2023 11:36:39 +0200 Subject: [PATCH 5/8] Clear the metadata on item burn returning the deposit --- frame/nfts/src/features/create_delete_item.rs | 22 +++++++++++++++++-- frame/nfts/src/tests.rs | 2 +- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/frame/nfts/src/features/create_delete_item.rs b/frame/nfts/src/features/create_delete_item.rs index 8ba012a63b606..26ade25fca416 100644 --- a/frame/nfts/src/features/create_delete_item.rs +++ b/frame/nfts/src/features/create_delete_item.rs @@ -91,6 +91,7 @@ impl, I: 'static> Pallet { with_details: impl FnOnce(&ItemDetailsFor) -> DispatchResult, ) -> DispatchResult { ensure!(!T::Locker::is_locked(collection, item), Error::::ItemLocked); + let item_config = Self::get_item_config(&collection, &item)?; let owner = Collection::::try_mutate( &collection, |maybe_collection_details| -> Result { @@ -103,6 +104,24 @@ impl, I: 'static> Pallet { // 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::::take(&collection, &item) { + let depositor_account = + metadata.deposit.account.unwrap_or(collection_details.owner.clone()); + + T::Currency::unreserve(&depositor_account, metadata.deposit.amount.clone()); + collection_details.item_metadatas.saturating_dec(); + + if depositor_account == collection_details.owner { + collection_details + .owner_deposit + .saturating_reduce(metadata.deposit.amount); + } + } + } + Ok(details.owner) }, )?; @@ -115,8 +134,7 @@ impl, I: 'static> Pallet { // 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::::remove(&collection, &item); } diff --git a/frame/nfts/src/tests.rs b/frame/nfts/src/tests.rs index 2b0c26962e5cd..ebbba33b04fa2 100644 --- a/frame/nfts/src/tests.rs +++ b/frame/nfts/src/tests.rs @@ -1267,7 +1267,7 @@ fn burn_works() { assert_noop!( Nfts::burn(RuntimeOrigin::signed(5), 0, 42, Some(5)), - Error::::UnknownCollection + Error::::UnknownItem ); assert_ok!(Nfts::force_mint(RuntimeOrigin::signed(2), 0, 42, 5, default_item_config())); From 44a289cd96431ed01eff68a901cbc7b092d9d91d Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Wed, 11 Jan 2023 13:03:40 +0200 Subject: [PATCH 6/8] Address comments --- frame/nfts/src/features/attributes.rs | 3 ++- frame/nfts/src/features/create_delete_item.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/frame/nfts/src/features/attributes.rs b/frame/nfts/src/features/attributes.rs index 7f2038488c746..449bcbbc09420 100644 --- a/frame/nfts/src/features/attributes.rs +++ b/frame/nfts/src/features/attributes.rs @@ -159,7 +159,8 @@ impl, I: 'static> Pallet { Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; if let Some(check_owner) = &maybe_check_owner { - // deposit.account.is_none(); maybe_check_owner.is_some() + // validate 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( diff --git a/frame/nfts/src/features/create_delete_item.rs b/frame/nfts/src/features/create_delete_item.rs index 26ade25fca416..f724fe5c63b43 100644 --- a/frame/nfts/src/features/create_delete_item.rs +++ b/frame/nfts/src/features/create_delete_item.rs @@ -111,7 +111,7 @@ impl, I: 'static> Pallet { let depositor_account = metadata.deposit.account.unwrap_or(collection_details.owner.clone()); - T::Currency::unreserve(&depositor_account, metadata.deposit.amount.clone()); + T::Currency::unreserve(&depositor_account, metadata.deposit.amount); collection_details.item_metadatas.saturating_dec(); if depositor_account == collection_details.owner { From 5878096693336c94e0246107af558efe2d0285ee Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Wed, 11 Jan 2023 13:15:32 +0200 Subject: [PATCH 7/8] Fix clippy --- frame/nfts/src/features/metadata.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nfts/src/features/metadata.rs b/frame/nfts/src/features/metadata.rs index fde5d4abd7bd1..272b2247426d6 100644 --- a/frame/nfts/src/features/metadata.rs +++ b/frame/nfts/src/features/metadata.rs @@ -110,7 +110,7 @@ impl, I: 'static> Pallet { ensure!(is_root || !is_locked, Error::::LockedItemMetadata); collection_details.item_metadatas.saturating_dec(); - T::Currency::unreserve(&depositor_account, metadata.deposit.amount.clone()); + T::Currency::unreserve(&depositor_account, metadata.deposit.amount); if depositor_account == collection_details.owner { collection_details.owner_deposit.saturating_reduce(metadata.deposit.amount); From e39130b79e348b6803479f5d4bc4072f3643b55f Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Wed, 11 Jan 2023 14:03:17 +0200 Subject: [PATCH 8/8] Don't return Ok on non-existing attribute removal --- frame/nfts/src/features/attributes.rs | 107 +++++++++++++------------- frame/nfts/src/lib.rs | 2 + 2 files changed, 56 insertions(+), 53 deletions(-) diff --git a/frame/nfts/src/features/attributes.rs b/frame/nfts/src/features/attributes.rs index 449bcbbc09420..b25f2a60cd62d 100644 --- a/frame/nfts/src/features/attributes.rs +++ b/frame/nfts/src/features/attributes.rs @@ -152,67 +152,68 @@ impl, I: 'static> Pallet { namespace: AttributeNamespace, key: BoundedVec, ) -> DispatchResult { - if let Some((_, deposit)) = - Attribute::::take((collection, maybe_item, &namespace, &key)) - { - let mut collection_details = - Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; - - if let Some(check_owner) = &maybe_check_owner { - // validate 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::::NoPermission - ); - } + let (_, deposit) = Attribute::::take((collection, maybe_item, &namespace, &key)) + .ok_or(Error::::AttributeNotFound)?; + let mut collection_details = + Collection::::get(&collection).ok_or(Error::::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::::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::::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::::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::::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::::LockedItemAttributes); + }, }, _ => (), }; - if let Some(deposit_account) = deposit.account { - T::Currency::unreserve(&deposit_account, deposit.amount); - } - Collection::::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::::insert(collection, &collection_details); + Self::deposit_event(Event::AttributeCleared { collection, maybe_item, key, namespace }); + Ok(()) } diff --git a/frame/nfts/src/lib.rs b/frame/nfts/src/lib.rs index 1e56de21c3518..8f24c8dcd6e98 100644 --- a/frame/nfts/src/lib.rs +++ b/frame/nfts/src/lib.rs @@ -561,6 +561,8 @@ pub mod pallet { 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.