From 99849c14839b9dd822547b3c74d52d874942721d Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Mon, 13 Mar 2023 12:54:37 +0200 Subject: [PATCH 1/3] Allow to unset the role --- frame/nfts/src/benchmarking.rs | 12 +-- frame/nfts/src/features/create_delete_item.rs | 46 +++++----- frame/nfts/src/features/roles.rs | 39 +++++++-- frame/nfts/src/lib.rs | 21 +++-- frame/nfts/src/tests.rs | 86 ++++++++++++++++--- 5 files changed, 143 insertions(+), 61 deletions(-) diff --git a/frame/nfts/src/benchmarking.rs b/frame/nfts/src/benchmarking.rs index d4bbd809ce8be..68252ebfc9cac 100644 --- a/frame/nfts/src/benchmarking.rs +++ b/frame/nfts/src/benchmarking.rs @@ -383,16 +383,16 @@ benchmarks_instance_pallet! { set_team { let (collection, caller, _) = create_collection::(); - 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::(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()); } diff --git a/frame/nfts/src/features/create_delete_item.rs b/frame/nfts/src/features/create_delete_item.rs index cd96d28448055..2aa27dc066619 100644 --- a/frame/nfts/src/features/create_delete_item.rs +++ b/frame/nfts/src/features/create_delete_item.rs @@ -106,9 +106,6 @@ impl, I: 'static> Pallet { let now = frame_system::Pallet::::block_number(); ensure!(deadline >= now, Error::::DeadlineExpired); - let collection_details = - Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; - ensure!( Self::has_role(&collection, &signer, CollectionRole::Issuer), Error::::NoPermission @@ -123,27 +120,28 @@ impl, I: 'static> Pallet { 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(()) } diff --git a/frame/nfts/src/features/roles.rs b/frame/nfts/src/features/roles.rs index f91f3ecb6f348..f977738dcd788 100644 --- a/frame/nfts/src/features/roles.rs +++ b/frame/nfts/src/features/roles.rs @@ -23,24 +23,47 @@ impl, I: 'static> Pallet { pub(crate) fn do_set_team( maybe_check_owner: Option, collection: T::CollectionId, - issuer: T::AccountId, - admin: T::AccountId, - freezer: T::AccountId, + issuer: Option, + admin: Option, + freezer: Option, ) -> DispatchResult { Collection::::try_mutate(collection, |maybe_details| { let details = maybe_details.as_mut().ok_or(Error::::UnknownCollection)?; + let is_root = maybe_check_owner.is_none(); if let Some(check_origin) = maybe_check_owner { ensure!(check_origin == details.owner, Error::::NoPermission); } - // delete previous values - Self::clear_roles(&collection)?; - - let account_to_role = Self::group_roles_by_account(vec![ + let roles_map = vec![ (issuer.clone(), CollectionRole::Issuer), (admin.clone(), CollectionRole::Admin), (freezer.clone(), CollectionRole::Freezer), - ]); + ]; + + // validate roles + if !is_root { + for (account, role) in roles_map.iter() { + if account.is_some() { + // only root can change the role from `None` to `Some(account)` + ensure!( + Self::find_account_by_role(&collection, *role).is_some(), + Error::::NoPermission + ); + } + } + } + + let valid_roles = roles_map + .into_iter() + .filter_map(|(account, role)| account.map(|account| (account, role))) + .collect(); + + let account_to_role = Self::group_roles_by_account(valid_roles); + + // delete previous records + Self::clear_roles(&collection)?; + + // insert new records for (account, roles) in account_to_role { CollectionRoleOf::::insert(&collection, &account, roles); } diff --git a/frame/nfts/src/lib.rs b/frame/nfts/src/lib.rs index 4d9547659a2a7..c7b6251cec67a 100644 --- a/frame/nfts/src/lib.rs +++ b/frame/nfts/src/lib.rs @@ -404,9 +404,9 @@ pub mod pallet { /// The management team changed. TeamChanged { collection: T::CollectionId, - issuer: T::AccountId, - admin: T::AccountId, - freezer: T::AccountId, + issuer: Option, + admin: Option, + freezer: Option, }, /// An `item` of a `collection` has been approved by the `owner` for transfer by /// a `delegate`. @@ -1137,6 +1137,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. @@ -1150,16 +1153,16 @@ pub mod pallet { pub fn set_team( origin: OriginFor, collection: T::CollectionId, - issuer: AccountIdLookupOf, - admin: AccountIdLookupOf, - freezer: AccountIdLookupOf, + issuer: Option>, + admin: Option>, + freezer: Option>, ) -> 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) } diff --git a/frame/nfts/src/tests.rs b/frame/nfts/src/tests.rs index 694468aceebe5..07c4c9f0a38db 100644 --- a/frame/nfts/src/tests.rs +++ b/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::::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::::NoPermission @@ -655,6 +655,46 @@ fn set_team_should_work() { Nfts::burn(RuntimeOrigin::signed(account(3)), 0, 42), Error::::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::::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::::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::::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::::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!( From 40712c7235e92f2be814dee66b5f1276bdb1fb1e Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Mon, 13 Mar 2023 13:09:45 +0200 Subject: [PATCH 2/3] Chore --- frame/nfts/src/features/roles.rs | 9 ++++----- frame/nfts/src/tests.rs | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/frame/nfts/src/features/roles.rs b/frame/nfts/src/features/roles.rs index f977738dcd788..315d3d4fa0461 100644 --- a/frame/nfts/src/features/roles.rs +++ b/frame/nfts/src/features/roles.rs @@ -40,11 +40,10 @@ impl, I: 'static> Pallet { (freezer.clone(), CollectionRole::Freezer), ]; - // validate roles + // 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() { - // only root can change the role from `None` to `Some(account)` ensure!( Self::find_account_by_role(&collection, *role).is_some(), Error::::NoPermission @@ -53,14 +52,14 @@ impl, I: 'static> Pallet { } } - let valid_roles = roles_map + 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(valid_roles); + let account_to_role = Self::group_roles_by_account(roles); - // delete previous records + // delete the previous records Self::clear_roles(&collection)?; // insert new records diff --git a/frame/nfts/src/tests.rs b/frame/nfts/src/tests.rs index 07c4c9f0a38db..0fc6fcd15c45f 100644 --- a/frame/nfts/src/tests.rs +++ b/frame/nfts/src/tests.rs @@ -3278,7 +3278,7 @@ fn pre_signed_mints_should_work() { signature, user_1.clone(), ), - Error::::UnknownCollection + Error::::NoPermission ); // validate max attributes limit From 889620dbf1ad58c45066399f4af6476c6b5cb1eb Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Mon, 13 Mar 2023 13:35:08 +0200 Subject: [PATCH 3/3] Array instead of vec --- frame/nfts/src/features/roles.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nfts/src/features/roles.rs b/frame/nfts/src/features/roles.rs index 315d3d4fa0461..3bac002069cf3 100644 --- a/frame/nfts/src/features/roles.rs +++ b/frame/nfts/src/features/roles.rs @@ -34,7 +34,7 @@ impl, I: 'static> Pallet { ensure!(check_origin == details.owner, Error::::NoPermission); } - let roles_map = vec![ + let roles_map = [ (issuer.clone(), CollectionRole::Issuer), (admin.clone(), CollectionRole::Admin), (freezer.clone(), CollectionRole::Freezer),