[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 <>

frame/nfts/src/benchmarking.rs

@@ -72,19 +72,68 @@ fn add_collection_metadata<T: Config<I>, I: 'static>() -> (T::AccountId, Account
 
 fn mint_item<T: Config<I>, I: 'static>(
 	index: u16,
+) -> (T::ItemId, T::AccountId, AccountIdLookupOf<T>) {
+	let item = T::Helper::item(index);
+	let collection = T::Helper::collection(0);
+	let caller = Collection::<T, I>::get(collection).unwrap().owner;
+	if caller != whitelisted_caller() {
+		whitelist_account!(caller);
+	}
+	let caller_lookup = T::Lookup::unlookup(caller.clone());
+	let item_exists = Item::<T, I>::contains_key(&collection, &item);
+	let item_config = ItemConfigOf::<T, I>::get(&collection, &item);
+	if item_exists {
+		return (item, caller, caller_lookup)
+	} else if let Some(item_config) = item_config {
+		assert_ok!(Nfts::<T, I>::force_mint(
+			SystemOrigin::Signed(caller.clone()).into(),
+			collection,
+			item,
+			caller_lookup.clone(),
+			item_config,
+		));
+	} else {
+		assert_ok!(Nfts::<T, I>::mint(
+			SystemOrigin::Signed(caller.clone()).into(),
+			collection,
+			item,
+			caller_lookup.clone(),
+			None,
+		));
+	}
+	(item, caller, caller_lookup)
+}
+
+fn lock_item<T: Config<I>, I: 'static>(
+	index: u16,
 ) -> (T::ItemId, T::AccountId, AccountIdLookupOf<T>) {
 	let caller = Collection::<T, I>::get(T::Helper::collection(0)).unwrap().owner;
 	if caller != whitelisted_caller() {
 		whitelist_account!(caller);
 	}
 	let caller_lookup = T::Lookup::unlookup(caller.clone());
 	let item = T::Helper::item(index);
-	assert_ok!(Nfts::<T, I>::mint(
+	assert_ok!(Nfts::<T, I>::lock_item_transfer(
+		SystemOrigin::Signed(caller.clone()).into(),
+		T::Helper::collection(0),
+		item,
+	));
+	(item, caller, caller_lookup)
+}
+
+fn burn_item<T: Config<I>, I: 'static>(
+	index: u16,
+) -> (T::ItemId, T::AccountId, AccountIdLookupOf<T>) {
+	let caller = Collection::<T, I>::get(T::Helper::collection(0)).unwrap().owner;
+	if caller != whitelisted_caller() {
+		whitelist_account!(caller);
+	}
+	let caller_lookup = T::Lookup::unlookup(caller.clone());
+	let item = T::Helper::item(index);
+	assert_ok!(Nfts::<T, I>::burn(
 		SystemOrigin::Signed(caller.clone()).into(),
 		T::Helper::collection(0),
 		item,
-		caller_lookup.clone(),
-		None,
 	));
 	(item, caller, caller_lookup)
 }
@@ -126,6 +175,26 @@ fn add_item_attribute<T: Config<I>, I: 'static>(
 	(key, caller, caller_lookup)
 }
 
+fn add_collection_attribute<T: Config<I>, I: 'static>(
+	i: u16,
+) -> (BoundedVec<u8, T::KeyLimit>, T::AccountId, AccountIdLookupOf<T>) {
+	let caller = Collection::<T, I>::get(T::Helper::collection(0)).unwrap().owner;
+	if caller != whitelisted_caller() {
+		whitelist_account!(caller);
+	}
+	let caller_lookup = T::Lookup::unlookup(caller.clone());
+	let key: BoundedVec<_, _> = make_filled_vec(i, T::KeyLimit::get() as usize).try_into().unwrap();
+	assert_ok!(Nfts::<T, I>::set_attribute(
+		SystemOrigin::Signed(caller.clone()).into(),
+		T::Helper::collection(0),
+		None,
+		AttributeNamespace::CollectionOwner,
+		key.clone(),
+		vec![0; T::ValueLimit::get() as usize].try_into().unwrap(),
+	));
+	(key, caller, caller_lookup)
+}
+
 fn assert_last_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::RuntimeEvent) {
 	let events = frame_system::Pallet::<T>::events();
 	let system_event: <T as frame_system::Config>::RuntimeEvent = generic_event.into();
@@ -190,26 +259,25 @@ benchmarks_instance_pallet! {
 	}
 
 	destroy {
-		let n in 0 .. 1_000;
 		let m in 0 .. 1_000;
+		let c in 0 .. 1_000;
 		let a in 0 .. 1_000;
 
 		let (collection, caller, _) = create_collection::<T, I>();
 		add_collection_metadata::<T, I>();
-		for i in 0..n {
-			mint_item::<T, I>(i as u16);
-		}
 		for i in 0..m {
-			if !Item::<T, I>::contains_key(collection, T::Helper::item(i as u16)) {
-				mint_item::<T, I>(i as u16);
-			}
+			mint_item::<T, I>(i as u16);
 			add_item_metadata::<T, I>(T::Helper::item(i as u16));
+			lock_item::<T, I>(i as u16);
+			burn_item::<T, I>(i as u16);
+		}
+		for i in 0..c {
+			mint_item::<T, I>(i as u16);
+			lock_item::<T, I>(i as u16);
+			burn_item::<T, I>(i as u16);
 		}
 		for i in 0..a {
-			if !Item::<T, I>::contains_key(collection, T::Helper::item(i as u16)) {
-				mint_item::<T, I>(i as u16);
-			}
-			add_item_attribute::<T, I>(T::Helper::item(i as u16));
+			add_collection_attribute::<T, I>(i as u16);
 		}
 		let witness = Collection::<T, I>::get(collection).unwrap().destroy_witness();
 	}: _(SystemOrigin::Signed(caller), collection, witness)
@@ -234,9 +302,9 @@ benchmarks_instance_pallet! {
 	}
 
 	burn {
-		let (collection, caller, caller_lookup) = create_collection::<T, I>();
+		let (collection, caller, _) = create_collection::<T, I>();
 		let (item, ..) = mint_item::<T, I>(0);
-	}: _(SystemOrigin::Signed(caller.clone()), collection, item, Some(caller_lookup))
+	}: _(SystemOrigin::Signed(caller.clone()), collection, item)
 	verify {
 		assert_last_event::<T, I>(Event::Burned { collection, item, owner: caller }.into());
 	}

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

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,29 +213,21 @@ 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>,
 		key: BoundedVec<u8, T::KeyLimit>,
 	) -> 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,24 +250,25 @@ 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
+							);
 						}
 					},
 				},
 				_ => (),
 			};
 		}
 
+		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 =

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