[NFTs] Rework permissions model (#13482)

* Disallow admin to transfer or burn items he doesn't own

* lock_collection should be accessible by collection's owner only

* Allow admin to access lock_item_properties()

* Fix do_lock_item_properties

* Move update_mint_settings() to Issuer

* Rename check_owner to check_origin

* Typo

* Make admin to be in charge of managing the metadata

* Make admin the main attributes manager

* offchain mint should be signed by Issuer

* Remove the special case when the Issuer calls the mint() function

* Rework burn and destroy methods

* Return back item_metadatas

* Don't repatriate the deposit on transfer

* A bit more tests

* One more test

* Add migration

* Chore

* Clippy

* Rename to owned_item

* Address comments

* Replace .filter_map with .find_map

* Improve version validation in pre_upgrade()

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

---------

Co-authored-by: parity-processbot <>

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

@@ -40,9 +40,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		);
 
 		if let Some(check_origin) = maybe_check_origin {
-			let is_admin = Self::has_role(&collection, &check_origin, CollectionRole::Admin);
-			let permitted = is_admin || check_origin == details.owner;
-			ensure!(permitted, Error::<T, I>::NoPermission);
+			ensure!(check_origin == details.owner, Error::<T, I>::NoPermission);
 		}
 
 		let now = frame_system::Pallet::<T>::block_number();
@@ -85,9 +83,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		if !is_past_deadline {
 			if let Some(check_origin) = maybe_check_origin {
-				let is_admin = Self::has_role(&collection, &check_origin, CollectionRole::Admin);
-				let permitted = is_admin || check_origin == details.owner;
-				ensure!(permitted, Error::<T, I>::NoPermission);
+				ensure!(check_origin == details.owner, Error::<T, I>::NoPermission);
 			}
 		}
 
@@ -113,9 +109,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			Item::<T, I>::get(&collection, &item).ok_or(Error::<T, I>::UnknownCollection)?;
 
 		if let Some(check_origin) = maybe_check_origin {
-			let is_admin = Self::has_role(&collection, &check_origin, CollectionRole::Admin);
-			let permitted = is_admin || check_origin == details.owner;
-			ensure!(permitted, Error::<T, I>::NoPermission);
+			ensure!(check_origin == details.owner, Error::<T, I>::NoPermission);
 		}
 
 		details.approvals.clear();

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

@@ -33,17 +33,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			Error::<T, I>::MethodDisabled
 		);
 
-		let mut collection_details =
-			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
-
 		ensure!(
-			Self::is_valid_namespace(
-				&origin,
-				&namespace,
-				&collection,
-				&collection_details.owner,
-				&maybe_item,
-			)?,
+			Self::is_valid_namespace(&origin, &namespace, &collection, &maybe_item)?,
 			Error::<T, I>::NoPermission
 		);
 
@@ -66,6 +57,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			_ => (),
 		}
 
+		let mut collection_details =
+			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
+
 		let attribute = Attribute::<T, I>::get((collection, maybe_item, &namespace, &key));
 		let attribute_exists = attribute.is_some();
 		if !attribute_exists {
@@ -219,7 +213,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	pub(crate) fn do_clear_attribute(
-		maybe_check_owner: Option<T::AccountId>,
+		maybe_check_origin: Option<T::AccountId>,
 		collection: T::CollectionId,
 		maybe_item: Option<T::ItemId>,
 		namespace: AttributeNamespace<T::AccountId>,
@@ -227,21 +221,13 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	) -> DispatchResult {
 		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 let Some(check_origin) = &maybe_check_origin {
 			// 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 {
+			if deposit.account != maybe_check_origin {
 				ensure!(
-					Self::is_valid_namespace(
-						&check_owner,
-						&namespace,
-						&collection,
-						&collection_details.owner,
-						&maybe_item,
-					)?,
+					Self::is_valid_namespace(&check_origin, &namespace, &collection, &maybe_item)?,
 					Error::<T, I>::NoPermission
 				);
 			}
@@ -264,17 +250,15 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 							.map_or(None, |c| {
 								Some(c.has_disabled_setting(ItemSetting::UnlockedAttributes))
 							});
-						match maybe_is_locked {
-							Some(is_locked) => {
-								// when item exists, then only the collection's owner can clear that
-								// attribute
-								ensure!(
-									check_owner == &collection_details.owner,
-									Error::<T, I>::NoPermission
-								);
-								ensure!(!is_locked, Error::<T, I>::LockedItemAttributes);
-							},
-							None => (),
+						if let Some(is_locked) = maybe_is_locked {
+							ensure!(!is_locked, Error::<T, I>::LockedItemAttributes);
+							// Only the collection's admin can clear attributes in that namespace.
+							// e.g. in off-chain mints, the attribute's depositor will be the item's
+							// owner, that's why we need to do this extra check.
+							ensure!(
+								Self::has_role(&collection, &check_origin, CollectionRole::Admin),
+								Error::<T, I>::NoPermission
+							);
 						}
 					},
 				},
@@ -282,6 +266,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			};
 		}
 
+		let mut collection_details =
+			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
+
 		collection_details.attributes.saturating_dec();
 
 		match deposit.account {
@@ -372,12 +359,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		origin: &T::AccountId,
 		namespace: &AttributeNamespace<T::AccountId>,
 		collection: &T::CollectionId,
-		collection_owner: &T::AccountId,
 		maybe_item: &Option<T::ItemId>,
 	) -> Result<bool, DispatchError> {
 		let mut result = false;
 		match namespace {
-			AttributeNamespace::CollectionOwner => result = origin == collection_owner,
+			AttributeNamespace::CollectionOwner =>
+				result = Self::has_role(&collection, &origin, CollectionRole::Admin),
 			AttributeNamespace::ItemOwner =>
 				if let Some(item) = maybe_item {
 					let item_details =

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

@@ -38,6 +38,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				owner_deposit: deposit,
 				items: 0,
 				item_metadatas: 0,
+				item_configs: 0,
 				attributes: 0,
 			},
 		);
@@ -71,24 +72,23 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			if let Some(check_owner) = maybe_check_owner {
 				ensure!(collection_details.owner == check_owner, Error::<T, I>::NoPermission);
 			}
-			ensure!(collection_details.items == witness.items, Error::<T, I>::BadWitness);
+			ensure!(collection_details.items == 0, Error::<T, I>::CollectionNotEmpty);
+			ensure!(collection_details.attributes == witness.attributes, Error::<T, I>::BadWitness);
 			ensure!(
 				collection_details.item_metadatas == witness.item_metadatas,
 				Error::<T, I>::BadWitness
 			);
-			ensure!(collection_details.attributes == witness.attributes, Error::<T, I>::BadWitness);
+			ensure!(
+				collection_details.item_configs == witness.item_configs,
+				Error::<T, I>::BadWitness
+			);
 
-			for (item, details) in Item::<T, I>::drain_prefix(&collection) {
-				Account::<T, I>::remove((&details.owner, &collection, &item));
-				T::Currency::unreserve(&details.deposit.account, details.deposit.amount);
-			}
 			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)?;
 
@@ -103,15 +103,13 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			CollectionAccount::<T, I>::remove(&collection_details.owner, &collection);
 			T::Currency::unreserve(&collection_details.owner, collection_details.owner_deposit);
 			CollectionConfigOf::<T, I>::remove(&collection);
-			let _ = ItemConfigOf::<T, I>::clear_prefix(&collection, witness.items, None);
-			let _ =
-				ItemAttributesApprovalsOf::<T, I>::clear_prefix(&collection, witness.items, None);
+			let _ = ItemConfigOf::<T, I>::clear_prefix(&collection, witness.item_configs, None);
 
 			Self::deposit_event(Event::Destroyed { collection });
 
 			Ok(DestroyWitness {
-				items: collection_details.items,
 				item_metadatas: collection_details.item_metadatas,
+				item_configs: collection_details.item_configs,
 				attributes: collection_details.attributes,
 			})
 		})

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

@@ -66,6 +66,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 					ensure!(existing_config == item_config, Error::<T, I>::InconsistentItemConfig);
 				} else {
 					ItemConfigOf::<T, I>::insert(&collection, &item, item_config);
+					collection_details.item_configs.saturating_inc();
 				}
 
 				T::Currency::reserve(&deposit_account, deposit_amount)?;
@@ -107,7 +108,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		let collection_details =
 			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
-		ensure!(collection_details.owner == signer, Error::<T, I>::NoPermission);
+
+		ensure!(
+			Self::has_role(&collection, &signer, CollectionRole::Issuer),
+			Error::<T, I>::NoPermission
+		);
 
 		let item_config = ItemConfig { settings: Self::get_default_item_settings(&collection)? };
 		Self::do_mint(
@@ -118,9 +123,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			item_config,
 			|_, _| Ok(()),
 		)?;
+		let origin = Self::find_account_by_role(&collection, CollectionRole::Admin)
+			.unwrap_or(collection_details.owner.clone());
 		for (key, value) in attributes {
 			Self::do_set_attribute(
-				collection_details.owner.clone(),
+				origin.clone(),
 				collection,
 				Some(item),
 				AttributeNamespace::CollectionOwner,
@@ -131,7 +138,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		}
 		if !metadata.len().is_zero() {
 			Self::do_set_item_metadata(
-				Some(collection_details.owner.clone()),
+				Some(origin.clone()),
 				collection,
 				item,
 				metadata,
@@ -148,6 +155,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	) -> DispatchResult {
 		ensure!(!T::Locker::is_locked(collection, item), Error::<T, I>::ItemLocked);
 		let item_config = Self::get_item_config(&collection, &item)?;
+		// NOTE: if item's settings are not empty (e.g. item's metadata is locked)
+		// then we keep the config record and don't remove it
+		let remove_config = !item_config.has_disabled_settings();
 		let owner = Collection::<T, I>::try_mutate(
 			&collection,
 			|maybe_collection_details| -> Result<T::AccountId, DispatchError> {
@@ -161,6 +171,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				T::Currency::unreserve(&details.deposit.account, details.deposit.amount);
 				collection_details.items.saturating_dec();
 
+				if remove_config {
+					collection_details.item_configs.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) {
@@ -188,9 +202,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		PendingSwapOf::<T, I>::remove(&collection, &item);
 		ItemAttributesApprovalsOf::<T, I>::remove(&collection, &item);
 
-		// 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
-		if !item_config.has_disabled_settings() {
+		if remove_config {
 			ItemConfigOf::<T, I>::remove(&collection, &item);
 		}
 

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

@@ -24,10 +24,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		collection: T::CollectionId,
 		lock_settings: CollectionSettings,
 	) -> DispatchResult {
-		ensure!(
-			Self::has_role(&collection, &origin, CollectionRole::Freezer),
-			Error::<T, I>::NoPermission
-		);
+		ensure!(Self::collection_owner(collection) == Some(origin), Error::<T, I>::NoPermission);
 		ensure!(
 			!lock_settings.is_disabled(CollectionSetting::DepositRequired),
 			Error::<T, I>::WrongSetting
@@ -85,17 +82,17 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	pub(crate) fn do_lock_item_properties(
-		maybe_check_owner: Option<T::AccountId>,
+		maybe_check_origin: Option<T::AccountId>,
 		collection: T::CollectionId,
 		item: T::ItemId,
 		lock_metadata: bool,
 		lock_attributes: bool,
 	) -> DispatchResult {
-		let collection_details =
-			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
-
-		if let Some(check_owner) = &maybe_check_owner {
-			ensure!(check_owner == &collection_details.owner, Error::<T, I>::NoPermission);
+		if let Some(check_origin) = &maybe_check_origin {
+			ensure!(
+				Self::has_role(&collection, &check_origin, CollectionRole::Admin),
+				Error::<T, I>::NoPermission
+			);
 		}
 
 		ItemConfigOf::<T, I>::try_mutate(collection, item, |maybe_config| {

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

@@ -21,13 +21,20 @@ 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>,
+		maybe_check_origin: 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();
+		if let Some(check_origin) = &maybe_check_origin {
+			ensure!(
+				Self::has_role(&collection, &check_origin, CollectionRole::Admin),
+				Error::<T, I>::NoPermission
+			);
+		}
+
+		let is_root = maybe_check_origin.is_none();
 		let mut collection_details =
 			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
 
@@ -37,10 +44,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			Error::<T, I>::LockedItemMetadata
 		);
 
-		if let Some(check_owner) = &maybe_check_owner {
-			ensure!(check_owner == &collection_details.owner, Error::<T, I>::NoPermission);
-		}
-
 		let collection_config = Self::get_collection_config(&collection)?;
 
 		ItemMetadataOf::<T, I>::try_mutate_exists(collection, item, |metadata| {
@@ -89,22 +92,26 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	pub(crate) fn do_clear_item_metadata(
-		maybe_check_owner: Option<T::AccountId>,
+		maybe_check_origin: Option<T::AccountId>,
 		collection: T::CollectionId,
 		item: T::ItemId,
 	) -> DispatchResult {
-		let is_root = maybe_check_owner.is_none();
+		if let Some(check_origin) = &maybe_check_origin {
+			ensure!(
+				Self::has_role(&collection, &check_origin, CollectionRole::Admin),
+				Error::<T, I>::NoPermission
+			);
+		}
+
+		let is_root = maybe_check_origin.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);
-		}
-
 		// 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));
@@ -125,29 +132,32 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	pub(crate) fn do_set_collection_metadata(
-		maybe_check_owner: Option<T::AccountId>,
+		maybe_check_origin: Option<T::AccountId>,
 		collection: T::CollectionId,
 		data: BoundedVec<u8, T::StringLimit>,
 	) -> DispatchResult {
+		if let Some(check_origin) = &maybe_check_origin {
+			ensure!(
+				Self::has_role(&collection, &check_origin, CollectionRole::Admin),
+				Error::<T, I>::NoPermission
+			);
+		}
+
+		let is_root = maybe_check_origin.is_none();
 		let collection_config = Self::get_collection_config(&collection)?;
 		ensure!(
-			maybe_check_owner.is_none() ||
-				collection_config.is_setting_enabled(CollectionSetting::UnlockedMetadata),
+			is_root || collection_config.is_setting_enabled(CollectionSetting::UnlockedMetadata),
 			Error::<T, I>::LockedCollectionMetadata
 		);
 
 		let mut details =
 			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
-		if let Some(check_owner) = &maybe_check_owner {
-			ensure!(check_owner == &details.owner, Error::<T, I>::NoPermission);
-		}
 
 		CollectionMetadataOf::<T, I>::try_mutate_exists(collection, |metadata| {
 			let old_deposit = metadata.take().map_or(Zero::zero(), |m| m.deposit);
 			details.owner_deposit.saturating_reduce(old_deposit);
 			let mut deposit = Zero::zero();
-			if maybe_check_owner.is_some() &&
-				collection_config.is_setting_enabled(CollectionSetting::DepositRequired)
+			if !is_root && collection_config.is_setting_enabled(CollectionSetting::DepositRequired)
 			{
 				deposit = T::DepositPerByte::get()
 					.saturating_mul(((data.len()) as u32).into())
@@ -170,18 +180,22 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	pub(crate) fn do_clear_collection_metadata(
-		maybe_check_owner: Option<T::AccountId>,
+		maybe_check_origin: Option<T::AccountId>,
 		collection: T::CollectionId,
 	) -> DispatchResult {
-		let details =
-			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
-		if let Some(check_owner) = &maybe_check_owner {
-			ensure!(check_owner == &details.owner, Error::<T, I>::NoPermission);
+		if let Some(check_origin) = &maybe_check_origin {
+			ensure!(
+				Self::has_role(&collection, &check_origin, CollectionRole::Admin),
+				Error::<T, I>::NoPermission
+			);
 		}
 
+		let details =
+			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
 		let collection_config = Self::get_collection_config(&collection)?;
+
 		ensure!(
-			maybe_check_owner.is_none() ||
+			maybe_check_origin.is_none() ||
 				collection_config.is_setting_enabled(CollectionSetting::UnlockedMetadata),
 			Error::<T, I>::LockedCollectionMetadata
 		);

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

@@ -82,6 +82,21 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			.map_or(false, |roles| roles.has_role(role))
 	}
 
+	/// Finds the account by a provided role within a collection.
+	///
+	/// - `collection_id`: A collection to check the role in.
+	/// - `role`: A role to find the account for.
+	///
+	/// Returns `Some(T::AccountId)` if the record was found, `None` otherwise.
+	pub(crate) fn find_account_by_role(
+		collection_id: &T::CollectionId,
+		role: CollectionRole,
+	) -> Option<T::AccountId> {
+		CollectionRoleOf::<T, I>::iter_prefix(&collection_id).into_iter().find_map(
+			|(account, roles)| if roles.has_role(role) { Some(account.clone()) } else { None },
+		)
+	}
+
 	/// Groups provided roles by account, given one account could have multiple roles.
 	///
 	/// - `input`: A vector of (Account, Role) tuples.

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

@@ -57,7 +57,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	pub(crate) fn do_update_mint_settings(
-		maybe_check_owner: Option<T::AccountId>,
+		maybe_check_origin: Option<T::AccountId>,
 		collection: T::CollectionId,
 		mint_settings: MintSettings<
 			BalanceOf<T, I>,
@@ -65,10 +65,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			T::CollectionId,
 		>,
 	) -> DispatchResult {
-		let details =
-			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
-		if let Some(check_owner) = &maybe_check_owner {
-			ensure!(check_owner == &details.owner, Error::<T, I>::NoPermission);
+		if let Some(check_origin) = &maybe_check_origin {
+			ensure!(
+				Self::has_role(&collection, &check_origin, CollectionRole::Issuer),
+				Error::<T, I>::NoPermission
+			);
 		}
 
 		CollectionConfigOf::<T, I>::try_mutate(collection, |maybe_config| {

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

@@ -48,16 +48,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			Item::<T, I>::get(&collection, &item).ok_or(Error::<T, I>::UnknownItem)?;
 		with_details(&collection_details, &mut details)?;
 
-		if details.deposit.account == details.owner {
-			// Move the deposit to the new owner.
-			T::Currency::repatriate_reserved(
-				&details.owner,
-				&dest,
-				details.deposit.amount,
-				Reserved,
-			)?;
-		}
-
 		Account::<T, I>::remove((&details.owner, &collection, &item));
 		Account::<T, I>::insert((&dest, &collection, &item), ());
 		let origin = details.owner;