[NFTs] Allow to set the role to None (#13591)

* Allow to unset the role

* Chore

* Array instead of vec

---------

Co-authored-by: parity-processbot <>

substrate/frame/nfts/src/benchmarking.rs

@@ -383,16 +383,16 @@ benchmarks_instance_pallet! {
 
 	set_team {
 		let (collection, caller, _) = create_collection::<T, I>();
-		let target0 = T::Lookup::unlookup(account("target", 0, SEED));
-		let target1 = T::Lookup::unlookup(account("target", 1, SEED));
-		let target2 = T::Lookup::unlookup(account("target", 2, SEED));
+		let target0 = Some(T::Lookup::unlookup(account("target", 0, SEED)));
+		let target1 = Some(T::Lookup::unlookup(account("target", 1, SEED)));
+		let target2 = Some(T::Lookup::unlookup(account("target", 2, SEED)));
 	}: _(SystemOrigin::Signed(caller), collection, target0, target1, target2)
 	verify {
 		assert_last_event::<T, I>(Event::TeamChanged{
 			collection,
-			issuer: account("target", 0, SEED),
-			admin: account("target", 1, SEED),
-			freezer: account("target", 2, SEED),
+			issuer: Some(account("target", 0, SEED)),
+			admin: Some(account("target", 1, SEED)),
+			freezer: Some(account("target", 2, SEED)),
 		}.into());
 	}
 

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

@@ -106,9 +106,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		let now = frame_system::Pallet::<T>::block_number();
 		ensure!(deadline >= now, Error::<T, I>::DeadlineExpired);
 
-		let collection_details =
-			Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
-
 		ensure!(
 			Self::has_role(&collection, &signer, CollectionRole::Issuer),
 			Error::<T, I>::NoPermission
@@ -123,27 +120,28 @@ 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(
-				origin.clone(),
-				collection,
-				Some(item),
-				AttributeNamespace::CollectionOwner,
-				Self::construct_attribute_key(key)?,
-				Self::construct_attribute_value(value)?,
-				mint_to.clone(),
-			)?;
-		}
-		if !metadata.len().is_zero() {
-			Self::do_set_item_metadata(
-				Some(origin.clone()),
-				collection,
-				item,
-				metadata,
-				Some(mint_to.clone()),
-			)?;
+		let admin_account = Self::find_account_by_role(&collection, CollectionRole::Admin);
+		if let Some(admin_account) = admin_account {
+			for (key, value) in attributes {
+				Self::do_set_attribute(
+					admin_account.clone(),
+					collection,
+					Some(item),
+					AttributeNamespace::CollectionOwner,
+					Self::construct_attribute_key(key)?,
+					Self::construct_attribute_value(value)?,
+					mint_to.clone(),
+				)?;
+			}
+			if !metadata.len().is_zero() {
+				Self::do_set_item_metadata(
+					Some(admin_account.clone()),
+					collection,
+					item,
+					metadata,
+					Some(mint_to.clone()),
+				)?;
+			}
 		}
 		Ok(())
 	}

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

@@ -23,24 +23,46 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	pub(crate) fn do_set_team(
 		maybe_check_owner: Option<T::AccountId>,
 		collection: T::CollectionId,
-		issuer: T::AccountId,
-		admin: T::AccountId,
-		freezer: T::AccountId,
+		issuer: Option<T::AccountId>,
+		admin: Option<T::AccountId>,
+		freezer: Option<T::AccountId>,
 	) -> DispatchResult {
 		Collection::<T, I>::try_mutate(collection, |maybe_details| {
 			let details = maybe_details.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;
+			let is_root = maybe_check_owner.is_none();
 			if let Some(check_origin) = maybe_check_owner {
 				ensure!(check_origin == details.owner, Error::<T, I>::NoPermission);
 			}
 
-			// delete previous values
-			Self::clear_roles(&collection)?;
-
-			let account_to_role = Self::group_roles_by_account(vec![
+			let roles_map = [
 				(issuer.clone(), CollectionRole::Issuer),
 				(admin.clone(), CollectionRole::Admin),
 				(freezer.clone(), CollectionRole::Freezer),
-			]);
+			];
+
+			// only root can change the role from `None` to `Some(account)`
+			if !is_root {
+				for (account, role) in roles_map.iter() {
+					if account.is_some() {
+						ensure!(
+							Self::find_account_by_role(&collection, *role).is_some(),
+							Error::<T, I>::NoPermission
+						);
+					}
+				}
+			}
+
+			let roles = roles_map
+				.into_iter()
+				.filter_map(|(account, role)| account.map(|account| (account, role)))
+				.collect();
+
+			let account_to_role = Self::group_roles_by_account(roles);
+
+			// delete the previous records
+			Self::clear_roles(&collection)?;
+
+			// insert new records
 			for (account, roles) in account_to_role {
 				CollectionRoleOf::<T, I>::insert(&collection, &account, roles);
 			}

substrate/frame/nfts/src/lib.rs

@@ -403,9 +403,9 @@ pub mod pallet {
 		/// The management team changed.
 		TeamChanged {
 			collection: T::CollectionId,
-			issuer: T::AccountId,
-			admin: T::AccountId,
-			freezer: T::AccountId,
+			issuer: Option<T::AccountId>,
+			admin: Option<T::AccountId>,
+			freezer: Option<T::AccountId>,
 		},
 		/// An `item` of a `collection` has been approved by the `owner` for transfer by
 		/// a `delegate`.
@@ -1136,6 +1136,9 @@ pub mod pallet {
 		/// Origin must be either `ForceOrigin` or Signed and the sender should be the Owner of the
 		/// `collection`.
 		///
+		/// Note: by setting the role to `None` only the `ForceOrigin` will be able to change it
+		/// after to `Some(account)`.
+		///
 		/// - `collection`: The collection whose team should be changed.
 		/// - `issuer`: The new Issuer of this collection.
 		/// - `admin`: The new Admin of this collection.
@@ -1149,16 +1152,16 @@ pub mod pallet {
 		pub fn set_team(
 			origin: OriginFor<T>,
 			collection: T::CollectionId,
-			issuer: AccountIdLookupOf<T>,
-			admin: AccountIdLookupOf<T>,
-			freezer: AccountIdLookupOf<T>,
+			issuer: Option<AccountIdLookupOf<T>>,
+			admin: Option<AccountIdLookupOf<T>>,
+			freezer: Option<AccountIdLookupOf<T>>,
 		) -> DispatchResult {
 			let maybe_check_owner = T::ForceOrigin::try_origin(origin)
 				.map(|_| None)
 				.or_else(|origin| ensure_signed(origin).map(Some).map_err(DispatchError::from))?;
-			let issuer = T::Lookup::lookup(issuer)?;
-			let admin = T::Lookup::lookup(admin)?;
-			let freezer = T::Lookup::lookup(freezer)?;
+			let issuer = issuer.map(T::Lookup::lookup).transpose()?;
+			let admin = admin.map(T::Lookup::lookup).transpose()?;
+			let freezer = freezer.map(T::Lookup::lookup).transpose()?;
 			Self::do_set_team(maybe_check_owner, collection, issuer, admin, freezer)
 		}
 

substrate/frame/nfts/src/tests.rs

@@ -535,9 +535,9 @@ fn origin_guards_should_work() {
 			Nfts::set_team(
 				RuntimeOrigin::signed(account(2)),
 				0,
-				account(2),
-				account(2),
-				account(2),
+				Some(account(2)),
+				Some(account(2)),
+				Some(account(2)),
 			),
 			Error::<Test>::NoPermission
 		);
@@ -639,14 +639,14 @@ fn set_team_should_work() {
 		assert_ok!(Nfts::set_team(
 			RuntimeOrigin::signed(account(1)),
 			0,
-			account(2),
-			account(3),
-			account(4),
+			Some(account(2)),
+			Some(account(3)),
+			Some(account(4)),
 		));
 
 		assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(2)), 0, 42, account(2), None));
-		assert_ok!(Nfts::lock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42));
-		assert_ok!(Nfts::unlock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42));
+
+		// admin can't transfer/burn items he doesn't own
 		assert_noop!(
 			Nfts::transfer(RuntimeOrigin::signed(account(3)), 0, 42, account(3)),
 			Error::<Test>::NoPermission
@@ -655,6 +655,46 @@ fn set_team_should_work() {
 			Nfts::burn(RuntimeOrigin::signed(account(3)), 0, 42),
 			Error::<Test>::NoPermission
 		);
+
+		assert_ok!(Nfts::lock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42));
+		assert_ok!(Nfts::unlock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42));
+
+		// validate we can set any role to None
+		assert_ok!(Nfts::set_team(
+			RuntimeOrigin::signed(account(1)),
+			0,
+			Some(account(2)),
+			Some(account(3)),
+			None,
+		));
+		assert_noop!(
+			Nfts::lock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42),
+			Error::<Test>::NoPermission
+		);
+
+		// set all the roles to None
+		assert_ok!(Nfts::set_team(RuntimeOrigin::signed(account(1)), 0, None, None, None,));
+
+		// validate we can't set the roles back
+		assert_noop!(
+			Nfts::set_team(
+				RuntimeOrigin::signed(account(1)),
+				0,
+				Some(account(2)),
+				Some(account(3)),
+				None,
+			),
+			Error::<Test>::NoPermission
+		);
+
+		// only the root account can change the roles from None to Some()
+		assert_ok!(Nfts::set_team(
+			RuntimeOrigin::root(),
+			0,
+			Some(account(2)),
+			Some(account(3)),
+			None,
+		));
 	});
 }
 
@@ -1476,7 +1516,13 @@ fn force_update_collection_should_work() {
 
 		Balances::make_free_balance_be(&account(5), 100);
 		assert_ok!(Nfts::force_collection_owner(RuntimeOrigin::root(), 0, account(5)));
-		assert_ok!(Nfts::set_team(RuntimeOrigin::root(), 0, account(2), account(5), account(4)));
+		assert_ok!(Nfts::set_team(
+			RuntimeOrigin::root(),
+			0,
+			Some(account(2)),
+			Some(account(5)),
+			Some(account(4)),
+		));
 		assert_eq!(collections(), vec![(account(5), 0)]);
 		assert_eq!(Balances::reserved_balance(account(1)), 2);
 		assert_eq!(Balances::reserved_balance(account(5)), 63);
@@ -1502,7 +1548,13 @@ fn force_update_collection_should_work() {
 		assert_eq!(Balances::reserved_balance(account(5)), 0);
 
 		// validate new roles
-		assert_ok!(Nfts::set_team(RuntimeOrigin::root(), 0, account(2), account(3), account(4)));
+		assert_ok!(Nfts::set_team(
+			RuntimeOrigin::root(),
+			0,
+			Some(account(2)),
+			Some(account(3)),
+			Some(account(4)),
+		));
 		assert_eq!(
 			CollectionRoleOf::<Test>::get(0, account(2)).unwrap(),
 			CollectionRoles(CollectionRole::Issuer.into())
@@ -1516,7 +1568,13 @@ fn force_update_collection_should_work() {
 			CollectionRoles(CollectionRole::Freezer.into())
 		);
 
-		assert_ok!(Nfts::set_team(RuntimeOrigin::root(), 0, account(3), account(2), account(3)));
+		assert_ok!(Nfts::set_team(
+			RuntimeOrigin::root(),
+			0,
+			Some(account(3)),
+			Some(account(2)),
+			Some(account(3)),
+		));
 
 		assert_eq!(
 			CollectionRoleOf::<Test>::get(0, account(2)).unwrap(),
@@ -1541,9 +1599,9 @@ fn burn_works() {
 		assert_ok!(Nfts::set_team(
 			RuntimeOrigin::signed(account(1)),
 			0,
-			account(2),
-			account(3),
-			account(4),
+			Some(account(2)),
+			Some(account(3)),
+			Some(account(4)),
 		));
 
 		assert_noop!(
@@ -3220,7 +3278,7 @@ fn pre_signed_mints_should_work() {
 				signature,
 				user_1.clone(),
 			),
-			Error::<Test>::UnknownCollection
+			Error::<Test>::NoPermission
 		);
 
 		// validate max attributes limit