Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Commit 4f28d29

Browse files
jsidorenkojsidorenko
authored
[NFTs] Allow to set the role to None (#13591)
* Allow to unset the role * Chore * Array instead of vec --------- Co-authored-by: parity-processbot <>
1 parent 3a94009 commit 4f28d29

File tree

5 files changed

+143
-62
lines changed

5 files changed

+143
-62
lines changed

‎frame/nfts/src/benchmarking.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -383,16 +383,16 @@ benchmarks_instance_pallet! {
383383

384384
set_team {
385385
let (collection, caller, _) = create_collection::<T, I>();
386-
let target0 = T::Lookup::unlookup(account("target", 0, SEED));
387-
let target1 = T::Lookup::unlookup(account("target", 1, SEED));
388-
let target2 = T::Lookup::unlookup(account("target", 2, SEED));
386+
let target0 = Some(T::Lookup::unlookup(account("target", 0, SEED)));
387+
let target1 = Some(T::Lookup::unlookup(account("target", 1, SEED)));
388+
let target2 = Some(T::Lookup::unlookup(account("target", 2, SEED)));
389389
}: _(SystemOrigin::Signed(caller), collection, target0, target1, target2)
390390
verify {
391391
assert_last_event::<T, I>(Event::TeamChanged{
392392
collection,
393-
issuer: account("target", 0, SEED),
394-
admin: account("target", 1, SEED),
395-
freezer: account("target", 2, SEED),
393+
issuer: Some(account("target", 0, SEED)),
394+
admin: Some(account("target", 1, SEED)),
395+
freezer: Some(account("target", 2, SEED)),
396396
}.into());
397397
}
398398

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

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
106106
let now = frame_system::Pallet::<T>::block_number();
107107
ensure!(deadline >= now, Error::<T, I>::DeadlineExpired);
108108

109-
let collection_details =
110-
Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
111-
112109
ensure!(
113110
Self::has_role(&collection, &signer, CollectionRole::Issuer),
114111
Error::<T, I>::NoPermission
@@ -123,27 +120,28 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
123120
item_config,
124121
|_, _| Ok(()),
125122
)?;
126-
let origin = Self::find_account_by_role(&collection, CollectionRole::Admin)
127-
.unwrap_or(collection_details.owner.clone());
128-
for (key, value) in attributes {
129-
Self::do_set_attribute(
130-
origin.clone(),
131-
collection,
132-
Some(item),
133-
AttributeNamespace::CollectionOwner,
134-
Self::construct_attribute_key(key)?,
135-
Self::construct_attribute_value(value)?,
136-
mint_to.clone(),
137-
)?;
138-
}
139-
if !metadata.len().is_zero() {
140-
Self::do_set_item_metadata(
141-
Some(origin.clone()),
142-
collection,
143-
item,
144-
metadata,
145-
Some(mint_to.clone()),
146-
)?;
123+
let admin_account = Self::find_account_by_role(&collection, CollectionRole::Admin);
124+
if let Some(admin_account) = admin_account {
125+
for (key, value) in attributes {
126+
Self::do_set_attribute(
127+
admin_account.clone(),
128+
collection,
129+
Some(item),
130+
AttributeNamespace::CollectionOwner,
131+
Self::construct_attribute_key(key)?,
132+
Self::construct_attribute_value(value)?,
133+
mint_to.clone(),
134+
)?;
135+
}
136+
if !metadata.len().is_zero() {
137+
Self::do_set_item_metadata(
138+
Some(admin_account.clone()),
139+
collection,
140+
item,
141+
metadata,
142+
Some(mint_to.clone()),
143+
)?;
144+
}
147145
}
148146
Ok(())
149147
}

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

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,46 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
2323
pub(crate) fn do_set_team(
2424
maybe_check_owner: Option<T::AccountId>,
2525
collection: T::CollectionId,
26-
issuer: T::AccountId,
27-
admin: T::AccountId,
28-
freezer: T::AccountId,
26+
issuer: Option<T::AccountId>,
27+
admin: Option<T::AccountId>,
28+
freezer: Option<T::AccountId>,
2929
) -> DispatchResult {
3030
Collection::<T, I>::try_mutate(collection, |maybe_details| {
3131
let details = maybe_details.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;
32+
let is_root = maybe_check_owner.is_none();
3233
if let Some(check_origin) = maybe_check_owner {
3334
ensure!(check_origin == details.owner, Error::<T, I>::NoPermission);
3435
}
3536

36-
// delete previous values
37-
Self::clear_roles(&collection)?;
38-
39-
let account_to_role = Self::group_roles_by_account(vec![
37+
let roles_map = [
4038
(issuer.clone(), CollectionRole::Issuer),
4139
(admin.clone(), CollectionRole::Admin),
4240
(freezer.clone(), CollectionRole::Freezer),
43-
]);
41+
];
42+
43+
// only root can change the role from `None` to `Some(account)`
44+
if !is_root {
45+
for (account, role) in roles_map.iter() {
46+
if account.is_some() {
47+
ensure!(
48+
Self::find_account_by_role(&collection, *role).is_some(),
49+
Error::<T, I>::NoPermission
50+
);
51+
}
52+
}
53+
}
54+
55+
let roles = roles_map
56+
.into_iter()
57+
.filter_map(|(account, role)| account.map(|account| (account, role)))
58+
.collect();
59+
60+
let account_to_role = Self::group_roles_by_account(roles);
61+
62+
// delete the previous records
63+
Self::clear_roles(&collection)?;
64+
65+
// insert new records
4466
for (account, roles) in account_to_role {
4567
CollectionRoleOf::<T, I>::insert(&collection, &account, roles);
4668
}

‎frame/nfts/src/lib.rs

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -403,9 +403,9 @@ pub mod pallet {
403403
/// The management team changed.
404404
TeamChanged {
405405
collection: T::CollectionId,
406-
issuer: T::AccountId,
407-
admin: T::AccountId,
408-
freezer: T::AccountId,
406+
issuer: Option<T::AccountId>,
407+
admin: Option<T::AccountId>,
408+
freezer: Option<T::AccountId>,
409409
},
410410
/// An `item` of a `collection` has been approved by the `owner` for transfer by
411411
/// a `delegate`.
@@ -1136,6 +1136,9 @@ pub mod pallet {
11361136
/// Origin must be either `ForceOrigin` or Signed and the sender should be the Owner of the
11371137
/// `collection`.
11381138
///
1139+
/// Note: by setting the role to `None` only the `ForceOrigin` will be able to change it
1140+
/// after to `Some(account)`.
1141+
///
11391142
/// - `collection`: The collection whose team should be changed.
11401143
/// - `issuer`: The new Issuer of this collection.
11411144
/// - `admin`: The new Admin of this collection.
@@ -1149,16 +1152,16 @@ pub mod pallet {
11491152
pub fn set_team(
11501153
origin: OriginFor<T>,
11511154
collection: T::CollectionId,
1152-
issuer: AccountIdLookupOf<T>,
1153-
admin: AccountIdLookupOf<T>,
1154-
freezer: AccountIdLookupOf<T>,
1155+
issuer: Option<AccountIdLookupOf<T>>,
1156+
admin: Option<AccountIdLookupOf<T>>,
1157+
freezer: Option<AccountIdLookupOf<T>>,
11551158
) -> DispatchResult {
11561159
let maybe_check_owner = T::ForceOrigin::try_origin(origin)
11571160
.map(|_| None)
11581161
.or_else(|origin| ensure_signed(origin).map(Some).map_err(DispatchError::from))?;
1159-
let issuer = T::Lookup::lookup(issuer)?;
1160-
let admin = T::Lookup::lookup(admin)?;
1161-
let freezer = T::Lookup::lookup(freezer)?;
1162+
let issuer = issuer.map(T::Lookup::lookup).transpose()?;
1163+
let admin = admin.map(T::Lookup::lookup).transpose()?;
1164+
let freezer = freezer.map(T::Lookup::lookup).transpose()?;
11621165
Self::do_set_team(maybe_check_owner, collection, issuer, admin, freezer)
11631166
}
11641167

‎frame/nfts/src/tests.rs

Lines changed: 73 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -535,9 +535,9 @@ fn origin_guards_should_work() {
535535
Nfts::set_team(
536536
RuntimeOrigin::signed(account(2)),
537537
0,
538-
account(2),
539-
account(2),
540-
account(2),
538+
Some(account(2)),
539+
Some(account(2)),
540+
Some(account(2)),
541541
),
542542
Error::<Test>::NoPermission
543543
);
@@ -639,14 +639,14 @@ fn set_team_should_work() {
639639
assert_ok!(Nfts::set_team(
640640
RuntimeOrigin::signed(account(1)),
641641
0,
642-
account(2),
643-
account(3),
644-
account(4),
642+
Some(account(2)),
643+
Some(account(3)),
644+
Some(account(4)),
645645
));
646646

647647
assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(2)), 0, 42, account(2), None));
648-
assert_ok!(Nfts::lock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42));
649-
assert_ok!(Nfts::unlock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42));
648+
649+
// admin can't transfer/burn items he doesn't own
650650
assert_noop!(
651651
Nfts::transfer(RuntimeOrigin::signed(account(3)), 0, 42, account(3)),
652652
Error::<Test>::NoPermission
@@ -655,6 +655,46 @@ fn set_team_should_work() {
655655
Nfts::burn(RuntimeOrigin::signed(account(3)), 0, 42),
656656
Error::<Test>::NoPermission
657657
);
658+
659+
assert_ok!(Nfts::lock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42));
660+
assert_ok!(Nfts::unlock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42));
661+
662+
// validate we can set any role to None
663+
assert_ok!(Nfts::set_team(
664+
RuntimeOrigin::signed(account(1)),
665+
0,
666+
Some(account(2)),
667+
Some(account(3)),
668+
None,
669+
));
670+
assert_noop!(
671+
Nfts::lock_item_transfer(RuntimeOrigin::signed(account(4)), 0, 42),
672+
Error::<Test>::NoPermission
673+
);
674+
675+
// set all the roles to None
676+
assert_ok!(Nfts::set_team(RuntimeOrigin::signed(account(1)), 0, None, None, None,));
677+
678+
// validate we can't set the roles back
679+
assert_noop!(
680+
Nfts::set_team(
681+
RuntimeOrigin::signed(account(1)),
682+
0,
683+
Some(account(2)),
684+
Some(account(3)),
685+
None,
686+
),
687+
Error::<Test>::NoPermission
688+
);
689+
690+
// only the root account can change the roles from None to Some()
691+
assert_ok!(Nfts::set_team(
692+
RuntimeOrigin::root(),
693+
0,
694+
Some(account(2)),
695+
Some(account(3)),
696+
None,
697+
));
658698
});
659699
}
660700

@@ -1476,7 +1516,13 @@ fn force_update_collection_should_work() {
14761516

14771517
Balances::make_free_balance_be(&account(5), 100);
14781518
assert_ok!(Nfts::force_collection_owner(RuntimeOrigin::root(), 0, account(5)));
1479-
assert_ok!(Nfts::set_team(RuntimeOrigin::root(), 0, account(2), account(5), account(4)));
1519+
assert_ok!(Nfts::set_team(
1520+
RuntimeOrigin::root(),
1521+
0,
1522+
Some(account(2)),
1523+
Some(account(5)),
1524+
Some(account(4)),
1525+
));
14801526
assert_eq!(collections(), vec![(account(5), 0)]);
14811527
assert_eq!(Balances::reserved_balance(account(1)), 2);
14821528
assert_eq!(Balances::reserved_balance(account(5)), 63);
@@ -1502,7 +1548,13 @@ fn force_update_collection_should_work() {
15021548
assert_eq!(Balances::reserved_balance(account(5)), 0);
15031549

15041550
// validate new roles
1505-
assert_ok!(Nfts::set_team(RuntimeOrigin::root(), 0, account(2), account(3), account(4)));
1551+
assert_ok!(Nfts::set_team(
1552+
RuntimeOrigin::root(),
1553+
0,
1554+
Some(account(2)),
1555+
Some(account(3)),
1556+
Some(account(4)),
1557+
));
15061558
assert_eq!(
15071559
CollectionRoleOf::<Test>::get(0, account(2)).unwrap(),
15081560
CollectionRoles(CollectionRole::Issuer.into())
@@ -1516,7 +1568,13 @@ fn force_update_collection_should_work() {
15161568
CollectionRoles(CollectionRole::Freezer.into())
15171569
);
15181570

1519-
assert_ok!(Nfts::set_team(RuntimeOrigin::root(), 0, account(3), account(2), account(3)));
1571+
assert_ok!(Nfts::set_team(
1572+
RuntimeOrigin::root(),
1573+
0,
1574+
Some(account(3)),
1575+
Some(account(2)),
1576+
Some(account(3)),
1577+
));
15201578

15211579
assert_eq!(
15221580
CollectionRoleOf::<Test>::get(0, account(2)).unwrap(),
@@ -1541,9 +1599,9 @@ fn burn_works() {
15411599
assert_ok!(Nfts::set_team(
15421600
RuntimeOrigin::signed(account(1)),
15431601
0,
1544-
account(2),
1545-
account(3),
1546-
account(4),
1602+
Some(account(2)),
1603+
Some(account(3)),
1604+
Some(account(4)),
15471605
));
15481606

15491607
assert_noop!(
@@ -3220,7 +3278,7 @@ fn pre_signed_mints_should_work() {
32203278
signature,
32213279
user_1.clone(),
32223280
),
3223-
Error::<Test>::UnknownCollection
3281+
Error::<Test>::NoPermission
32243282
);
32253283

32263284
// validate max attributes limit

0 commit comments

Comments
 (0)