From 6763dd6124882843e488c905dc0ab6b030670c4d Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> Date: Thu, 20 Oct 2022 11:32:52 +0300 Subject: [PATCH] [Uniques V2] Refactor roles (#12437) * Basics * WIP: change the data format * Refactor * Remove redundant new() method * Rename settings * Enable tests * Chore * Change params order * Delete the config on collection removal * Chore * Remove redundant system features * Rename force_item_status to force_collection_status * Update node runtime * Chore * Remove thaw_collection * Chore * Connect collection.is_frozen to config * Allow to lock the collection in a new way * Move free_holding into settings * Connect collection's metadata locker to feature flags * DRY * Chore * Connect pallet level feature flags * Prepare tests for the new changes * Implement Item settings * Allow to lock the metadata or attributes of an item * Common -> Settings * Extract settings related code to a separate file * Move feature flag checks inside the do_* methods * Split settings.rs into parts * Extract repeated code into macro * Extract macros into their own file * Chore * Fix traits * Fix traits * Test SystemFeatures * Fix benchmarks * Add missing benchmark * Fix node/runtime/lib.rs * ".git/.scripts/bench-bot.sh" pallet dev pallet_nfts * Keep item's config on burn if it's not empty * Fix the merge artifacts * Fmt * Add SystemFeature::NoSwaps check * Refactor roles structure * Rename SystemFeatures to PalletFeatures * Rename errors * Add docs * Change error message * Rework pallet features * Move macros * Change comments * Fmt * Refactor Incrementable * Use pub(crate) for do_* functions * Update comments * Refactor freeze and lock functions * Rework Collection config and Item confg api * Chore * Make clippy happy * Chore * Fix artifacts * Address comments * Further refactoring * Add comments * Add tests for group_roles_by_account() * Update frame/nfts/src/impl_nonfungibles.rs * Add test * Replace Itertools group_by with a custom implementation * ItemsNotTransferable => ItemsNonTransferable * Update frame/nfts/src/features/roles.rs Co-authored-by: Muharem Ismailov * Address PR comments * Add missed comment Co-authored-by: command-bot <> Co-authored-by: Muharem Ismailov --- frame/nfts/src/benchmarking.rs | 2 +- frame/nfts/src/features/lock.rs | 22 +++--- frame/nfts/src/features/mod.rs | 1 + frame/nfts/src/features/roles.rs | 69 ++++++++++++++++++ frame/nfts/src/functions.rs | 22 +++--- frame/nfts/src/impl_nonfungibles.rs | 4 +- frame/nfts/src/lib.rs | 97 ++++++++++++++++++-------- frame/nfts/src/tests.rs | 104 +++++++++++++++++++++++----- frame/nfts/src/types.rs | 44 +++++++++--- 9 files changed, 291 insertions(+), 74 deletions(-) create mode 100644 frame/nfts/src/features/roles.rs diff --git a/frame/nfts/src/benchmarking.rs b/frame/nfts/src/benchmarking.rs index c65430fd35108..a5a264c40a715 100644 --- a/frame/nfts/src/benchmarking.rs +++ b/frame/nfts/src/benchmarking.rs @@ -68,7 +68,7 @@ fn add_collection_metadata, I: 'static>() -> (T::AccountId, Account fn mint_item, I: 'static>( index: u16, ) -> (T::ItemId, T::AccountId, AccountIdLookupOf) { - let caller = Collection::::get(T::Helper::collection(0)).unwrap().admin; + let caller = Collection::::get(T::Helper::collection(0)).unwrap().owner; if caller != whitelisted_caller() { whitelist_account!(caller); } diff --git a/frame/nfts/src/features/lock.rs b/frame/nfts/src/features/lock.rs index 0a5fecc1d6224..50420d8e3de87 100644 --- a/frame/nfts/src/features/lock.rs +++ b/frame/nfts/src/features/lock.rs @@ -24,10 +24,10 @@ impl, I: 'static> Pallet { collection: T::CollectionId, lock_config: CollectionConfig, ) -> DispatchResult { - let details = - Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; - ensure!(origin == details.freezer, Error::::NoPermission); - + ensure!( + Self::has_role(&collection, &origin, CollectionRole::Freezer), + Error::::NoPermission + ); CollectionConfigOf::::try_mutate(collection, |maybe_config| { let config = maybe_config.as_mut().ok_or(Error::::NoConfig)?; @@ -51,9 +51,10 @@ impl, I: 'static> Pallet { collection: T::CollectionId, item: T::ItemId, ) -> DispatchResult { - let collection_details = - Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; - ensure!(collection_details.freezer == origin, Error::::NoPermission); + ensure!( + Self::has_role(&collection, &origin, CollectionRole::Freezer), + Error::::NoPermission + ); let mut config = Self::get_item_config(&collection, &item)?; if !config.has_disabled_setting(ItemSetting::Transferable) { @@ -70,9 +71,10 @@ impl, I: 'static> Pallet { collection: T::CollectionId, item: T::ItemId, ) -> DispatchResult { - let collection_details = - Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; - ensure!(collection_details.freezer == origin, Error::::NoPermission); + ensure!( + Self::has_role(&collection, &origin, CollectionRole::Freezer), + Error::::NoPermission + ); let mut config = Self::get_item_config(&collection, &item)?; if config.has_disabled_setting(ItemSetting::Transferable) { diff --git a/frame/nfts/src/features/mod.rs b/frame/nfts/src/features/mod.rs index 47e5816bc953c..f814d696d774b 100644 --- a/frame/nfts/src/features/mod.rs +++ b/frame/nfts/src/features/mod.rs @@ -18,4 +18,5 @@ pub mod atomic_swap; pub mod buy_sell; pub mod lock; +pub mod roles; pub mod settings; diff --git a/frame/nfts/src/features/roles.rs b/frame/nfts/src/features/roles.rs new file mode 100644 index 0000000000000..e961779725b6e --- /dev/null +++ b/frame/nfts/src/features/roles.rs @@ -0,0 +1,69 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use crate::*; +use frame_support::pallet_prelude::*; +use sp_std::collections::btree_map::BTreeMap; + +impl, I: 'static> Pallet { + /// Clears all the roles in a specified collection. + /// + /// - `collection_id`: A collection to clear the roles in. + /// + /// Throws an error if some of the roles were left in storage. + /// This means the `CollectionRoles::max_roles()` needs to be adjusted. + pub(crate) fn clear_roles(collection_id: &T::CollectionId) -> Result<(), DispatchError> { + let res = CollectionRoleOf::::clear_prefix( + &collection_id, + CollectionRoles::max_roles() as u32, + None, + ); + ensure!(res.maybe_cursor.is_none(), Error::::RolesNotCleared); + Ok(()) + } + + /// Returns true if a specified account has a provided role within that collection. + /// + /// - `collection_id`: A collection to check the role in. + /// - `account_id`: An account to check the role for. + /// - `role`: A role to validate. + /// + /// Returns boolean. + pub(crate) fn has_role( + collection_id: &T::CollectionId, + account_id: &T::AccountId, + role: CollectionRole, + ) -> bool { + CollectionRoleOf::::get(&collection_id, &account_id) + .map_or(false, |roles| roles.has_role(role)) + } + + /// Groups provided roles by account, given one account could have multiple roles. + /// + /// - `input`: A vector of (Account, Role) tuples. + /// + /// Returns a grouped vector. + pub fn group_roles_by_account( + input: Vec<(T::AccountId, CollectionRole)>, + ) -> Vec<(T::AccountId, CollectionRoles)> { + let mut result = BTreeMap::new(); + for (account, role) in input.into_iter() { + result.entry(account).or_insert(CollectionRoles::none()).add_role(role); + } + result.into_iter().collect() + } +} diff --git a/frame/nfts/src/functions.rs b/frame/nfts/src/functions.rs index 275e3668d7a20..90a701bc9eaa0 100644 --- a/frame/nfts/src/functions.rs +++ b/frame/nfts/src/functions.rs @@ -41,7 +41,7 @@ impl, I: 'static> Pallet { let collection_config = Self::get_collection_config(&collection)?; ensure!( collection_config.is_setting_enabled(CollectionSetting::TransferableItems), - Error::::ItemsNotTransferable + Error::::ItemsNonTransferable ); let item_config = Self::get_item_config(&collection, &item)?; @@ -93,15 +93,19 @@ impl, I: 'static> Pallet { collection, CollectionDetails { owner: owner.clone(), - issuer: admin.clone(), - admin: admin.clone(), - freezer: admin, total_deposit: deposit, items: 0, item_metadatas: 0, attributes: 0, }, ); + CollectionRoleOf::::insert( + collection, + admin, + CollectionRoles( + CollectionRole::Admin | CollectionRole::Freezer | CollectionRole::Issuer, + ), + ); let next_id = collection.increment(); @@ -142,6 +146,7 @@ impl, I: 'static> Pallet { #[allow(deprecated)] PendingSwapOf::::remove_prefix(&collection, None); CollectionMetadataOf::::remove(&collection); + Self::clear_roles(&collection)?; #[allow(deprecated)] Attribute::::remove_prefix((&collection,), None); CollectionAccount::::remove(&collection_details.owner, &collection); @@ -165,7 +170,6 @@ impl, I: 'static> Pallet { item: T::ItemId, owner: T::AccountId, config: ItemConfig, - with_details: impl FnOnce(&CollectionDetailsFor) -> DispatchResult, ) -> DispatchResult { ensure!(!Item::::contains_key(collection, item), Error::::AlreadyExists); @@ -175,8 +179,6 @@ impl, I: 'static> Pallet { let collection_details = maybe_collection_details.as_mut().ok_or(Error::::UnknownCollection)?; - with_details(collection_details)?; - if let Ok(max_supply) = CollectionMaxSupply::::try_get(&collection) { ensure!(collection_details.items < max_supply, Error::::MaxSupplyReached); } @@ -218,7 +220,7 @@ impl, I: 'static> Pallet { pub fn do_burn( collection: T::CollectionId, item: T::ItemId, - with_details: impl FnOnce(&CollectionDetailsFor, &ItemDetailsFor) -> DispatchResult, + with_details: impl FnOnce(&ItemDetailsFor) -> DispatchResult, ) -> DispatchResult { let owner = Collection::::try_mutate( &collection, @@ -227,7 +229,7 @@ impl, I: 'static> Pallet { maybe_collection_details.as_mut().ok_or(Error::::UnknownCollection)?; let details = Item::::get(&collection, &item) .ok_or(Error::::UnknownCollection)?; - with_details(collection_details, &details)?; + with_details(&details)?; // Return the deposit. T::Currency::unreserve(&collection_details.owner, details.deposit); @@ -271,7 +273,7 @@ impl, I: 'static> Pallet { let collection_config = Self::get_collection_config(&collection)?; ensure!( collection_config.is_setting_enabled(CollectionSetting::TransferableItems), - Error::::ItemsNotTransferable + Error::::ItemsNonTransferable ); let item_config = Self::get_item_config(&collection, &item)?; diff --git a/frame/nfts/src/impl_nonfungibles.rs b/frame/nfts/src/impl_nonfungibles.rs index 8a7c79fc0c14f..210fe4831991d 100644 --- a/frame/nfts/src/impl_nonfungibles.rs +++ b/frame/nfts/src/impl_nonfungibles.rs @@ -143,7 +143,7 @@ impl, I: 'static> Mutate<::AccountId, ItemSettin who: &T::AccountId, settings: &ItemSettings, ) -> DispatchResult { - Self::do_mint(*collection, *item, who.clone(), ItemConfig(*settings), |_| Ok(())) + Self::do_mint(*collection, *item, who.clone(), ItemConfig(*settings)) } fn burn( @@ -151,7 +151,7 @@ impl, I: 'static> Mutate<::AccountId, ItemSettin item: &Self::ItemId, maybe_check_owner: Option<&T::AccountId>, ) -> DispatchResult { - Self::do_burn(*collection, *item, |_, d| { + Self::do_burn(*collection, *item, |d| { if let Some(check_owner) = maybe_check_owner { if &d.owner != check_owner { return Err(Error::::NoPermission.into()) diff --git a/frame/nfts/src/lib.rs b/frame/nfts/src/lib.rs index bfba0c1ea3330..8b8b21f944f3c 100644 --- a/frame/nfts/src/lib.rs +++ b/frame/nfts/src/lib.rs @@ -219,6 +219,19 @@ pub mod pallet { OptionQuery, >; + /// The items in existence and their ownership details. + #[pallet::storage] + /// Stores collection roles as per account. + pub(super) type CollectionRoleOf, I: 'static = ()> = StorageDoubleMap< + _, + Blake2_128Concat, + T::CollectionId, + Blake2_128Concat, + T::AccountId, + CollectionRoles, + OptionQuery, + >; + /// The items in existence and their ownership details. #[pallet::storage] #[pallet::storage_prefix = "Asset"] @@ -497,7 +510,7 @@ pub mod pallet { /// Collection ID is already taken. CollectionIdInUse, /// Items within that collection are non-transferable. - ItemsNotTransferable, + ItemsNonTransferable, /// The provided account is not a delegate. NotDelegate, /// The delegate turned out to be different to what was expected. @@ -544,6 +557,8 @@ pub mod pallet { InconsistentItemConfig, /// Config for a collection or an item can't be found. NoConfig, + /// Some roles were not cleared. + RolesNotCleared, } impl, I: 'static> Pallet { @@ -702,10 +717,11 @@ pub mod pallet { let origin = ensure_signed(origin)?; let owner = T::Lookup::lookup(owner)?; - Self::do_mint(collection, item, owner, config, |collection_details| { - ensure!(collection_details.issuer == origin, Error::::NoPermission); - Ok(()) - }) + ensure!( + Self::has_role(&collection, &origin, CollectionRole::Issuer), + Error::::NoPermission + ); + Self::do_mint(collection, item, owner, config) } /// Destroy a single item. @@ -731,8 +747,9 @@ pub mod pallet { let origin = ensure_signed(origin)?; let check_owner = check_owner.map(T::Lookup::lookup).transpose()?; - Self::do_burn(collection, item, |collection_details, details| { - let is_permitted = collection_details.admin == origin || details.owner == origin; + Self::do_burn(collection, item, |details| { + let is_admin = Self::has_role(&collection, &origin, CollectionRole::Admin); + let is_permitted = is_admin || details.owner == origin; ensure!(is_permitted, Error::::NoPermission); ensure!( check_owner.map_or(true, |o| o == details.owner), @@ -767,8 +784,9 @@ pub mod pallet { let origin = ensure_signed(origin)?; let dest = T::Lookup::lookup(dest)?; - Self::do_transfer(collection, item, dest, |collection_details, details| { - if details.owner != origin && collection_details.admin != origin { + Self::do_transfer(collection, item, dest, |_, details| { + let is_admin = Self::has_role(&collection, &origin, CollectionRole::Admin); + if details.owner != origin && !is_admin { let deadline = details.approvals.get(&origin).ok_or(Error::::NoPermission)?; if let Some(d) = deadline { @@ -986,9 +1004,17 @@ pub mod pallet { let details = maybe_details.as_mut().ok_or(Error::::UnknownCollection)?; ensure!(origin == details.owner, Error::::NoPermission); - details.issuer = issuer.clone(); - details.admin = admin.clone(); - details.freezer = freezer.clone(); + // delete previous values + Self::clear_roles(&collection)?; + + let account_to_role = Self::group_roles_by_account(vec![ + (issuer.clone(), CollectionRole::Issuer), + (admin.clone(), CollectionRole::Admin), + (freezer.clone(), CollectionRole::Freezer), + ]); + for (account, roles) in account_to_role { + CollectionRoleOf::::insert(&collection, &account, roles); + } Self::deposit_event(Event::TeamChanged { collection, issuer, admin, freezer }); Ok(()) @@ -1026,19 +1052,24 @@ pub mod pallet { let delegate = T::Lookup::lookup(delegate)?; - let collection_details = - Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; let mut details = - Item::::get(&collection, &item).ok_or(Error::::UnknownCollection)?; + Item::::get(&collection, &item).ok_or(Error::::UnknownItem)?; + + let collection_config = Self::get_collection_config(&collection)?; + ensure!( + collection_config.is_setting_enabled(CollectionSetting::TransferableItems), + Error::::ItemsNonTransferable + ); let collection_config = Self::get_collection_config(&collection)?; ensure!( collection_config.is_setting_enabled(CollectionSetting::TransferableItems), - Error::::ItemsNotTransferable + Error::::ItemsNonTransferable ); if let Some(check) = maybe_check { - let permitted = check == collection_details.admin || check == details.owner; + let is_admin = Self::has_role(&collection, &check, CollectionRole::Admin); + let permitted = is_admin || check == details.owner; ensure!(permitted, Error::::NoPermission); } @@ -1090,10 +1121,8 @@ pub mod pallet { let delegate = T::Lookup::lookup(delegate)?; - let collection_details = - Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; let mut details = - Item::::get(&collection, &item).ok_or(Error::::UnknownCollection)?; + Item::::get(&collection, &item).ok_or(Error::::UnknownItem)?; let maybe_deadline = details.approvals.get(&delegate).ok_or(Error::::NotDelegate)?; @@ -1107,7 +1136,8 @@ pub mod pallet { if !is_past_deadline { if let Some(check) = maybe_check { - let permitted = check == collection_details.admin || check == details.owner; + let is_admin = Self::has_role(&collection, &check, CollectionRole::Admin); + let permitted = is_admin || check == details.owner; ensure!(permitted, Error::::NoPermission); } } @@ -1148,12 +1178,12 @@ pub mod pallet { .map(|_| None) .or_else(|origin| ensure_signed(origin).map(Some).map_err(DispatchError::from))?; - let collection_details = - Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; let mut details = Item::::get(&collection, &item).ok_or(Error::::UnknownCollection)?; + if let Some(check) = maybe_check { - let permitted = check == collection_details.admin || check == details.owner; + let is_admin = Self::has_role(&collection, &check, CollectionRole::Admin); + let permitted = is_admin || check == details.owner; ensure!(permitted, Error::::NoPermission); } @@ -1200,14 +1230,27 @@ pub mod pallet { let old_owner = collection_info.owner; let new_owner = T::Lookup::lookup(owner)?; collection_info.owner = new_owner.clone(); - collection_info.issuer = T::Lookup::lookup(issuer)?; - collection_info.admin = T::Lookup::lookup(admin)?; - collection_info.freezer = T::Lookup::lookup(freezer)?; *maybe_collection = Some(collection_info); CollectionAccount::::remove(&old_owner, &collection); CollectionAccount::::insert(&new_owner, &collection, ()); CollectionConfigOf::::insert(&collection, config); + let issuer = T::Lookup::lookup(issuer)?; + let admin = T::Lookup::lookup(admin)?; + let freezer = T::Lookup::lookup(freezer)?; + + // delete previous values + Self::clear_roles(&collection)?; + + let account_to_role = Self::group_roles_by_account(vec![ + (issuer, CollectionRole::Issuer), + (admin, CollectionRole::Admin), + (freezer, CollectionRole::Freezer), + ]); + for (account, roles) in account_to_role { + CollectionRoleOf::::insert(&collection, &account, roles); + } + Self::deposit_event(Event::CollectionStatusChanged { collection }); Ok(()) }) diff --git a/frame/nfts/src/tests.rs b/frame/nfts/src/tests.rs index 1b60fd6431b19..d0841ebc1f238 100644 --- a/frame/nfts/src/tests.rs +++ b/frame/nfts/src/tests.rs @@ -21,7 +21,7 @@ use crate::{mock::*, Event, *}; use frame_support::{ assert_noop, assert_ok, dispatch::Dispatchable, - traits::{Currency, Get}, + traits::{tokens::nonfungibles_v2::Destroy, Currency, Get}, }; use pallet_balances::Error as BalancesError; use sp_std::prelude::*; @@ -152,7 +152,7 @@ fn lifecycle_should_work() { assert_eq!(Balances::reserved_balance(&1), 13); assert!(ItemMetadataOf::::contains_key(0, 69)); - let w = Collection::::get(0).unwrap().destroy_witness(); + let w = Nfts::get_destroy_witness(&0).unwrap(); assert_eq!(w.items, 2); assert_eq!(w.item_metadatas, 2); assert_ok!(Nfts::destroy(RuntimeOrigin::signed(1), 0, w)); @@ -227,7 +227,7 @@ fn transfer_should_work() { assert_noop!( Nfts::transfer(RuntimeOrigin::signed(1), collection_id, 42, 3,), - Error::::ItemsNotTransferable + Error::::ItemsNonTransferable ); }); } @@ -248,7 +248,7 @@ fn locking_transfer_should_work() { )); assert_noop!( Nfts::transfer(RuntimeOrigin::signed(1), 0, 42, 2), - Error::::ItemsNotTransferable + Error::::ItemsNonTransferable ); assert_ok!(Nfts::force_collection_status( @@ -296,7 +296,7 @@ fn origin_guards_should_work() { Nfts::burn(RuntimeOrigin::signed(2), 0, 42, None), Error::::NoPermission ); - let w = Collection::::get(0).unwrap().destroy_witness(); + let w = Nfts::get_destroy_witness(&0).unwrap(); assert_noop!(Nfts::destroy(RuntimeOrigin::signed(2), 0, w), Error::::NoPermission); }); } @@ -550,7 +550,7 @@ fn set_attribute_should_work() { ); assert_eq!(Balances::reserved_balance(1), 16); - let w = Collection::::get(0).unwrap().destroy_witness(); + let w = Nfts::get_destroy_witness(&0).unwrap(); assert_ok!(Nfts::destroy(RuntimeOrigin::signed(1), 0, w)); assert_eq!(attributes(0), vec![]); assert_eq!(Balances::reserved_balance(1), 0); @@ -687,6 +687,48 @@ fn force_collection_status_should_work() { assert_ok!(Nfts::set_collection_metadata(RuntimeOrigin::signed(1), 0, bvec![0; 20])); assert_eq!(Balances::reserved_balance(1), 0); + + // validate new roles + assert_ok!(Nfts::force_collection_status( + RuntimeOrigin::root(), + 0, + 1, + 2, + 3, + 4, + CollectionConfig::all_settings_enabled(), + )); + assert_eq!( + CollectionRoleOf::::get(0, 2).unwrap(), + CollectionRoles(CollectionRole::Issuer.into()) + ); + assert_eq!( + CollectionRoleOf::::get(0, 3).unwrap(), + CollectionRoles(CollectionRole::Admin.into()) + ); + assert_eq!( + CollectionRoleOf::::get(0, 4).unwrap(), + CollectionRoles(CollectionRole::Freezer.into()) + ); + + assert_ok!(Nfts::force_collection_status( + RuntimeOrigin::root(), + 0, + 1, + 3, + 2, + 3, + CollectionConfig::all_settings_enabled(), + )); + + assert_eq!( + CollectionRoleOf::::get(0, 2).unwrap(), + CollectionRoles(CollectionRole::Admin.into()) + ); + assert_eq!( + CollectionRoleOf::::get(0, 3).unwrap(), + CollectionRoles(CollectionRole::Issuer | CollectionRole::Freezer) + ); }); } @@ -761,7 +803,7 @@ fn approval_lifecycle_works() { assert_noop!( Nfts::approve_transfer(RuntimeOrigin::signed(1), collection_id, 1, 2, None), - Error::::ItemsNotTransferable + Error::::ItemsNonTransferable ); }); } @@ -775,11 +817,11 @@ fn cancel_approval_works() { assert_ok!(Nfts::approve_transfer(RuntimeOrigin::signed(2), 0, 42, 3, None)); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::signed(2), 1, 42, 3), - Error::::UnknownCollection + Error::::UnknownItem ); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::signed(2), 0, 43, 3), - Error::::UnknownCollection + Error::::UnknownItem ); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::signed(3), 0, 42, 3), @@ -898,11 +940,11 @@ fn cancel_approval_works_with_admin() { assert_ok!(Nfts::approve_transfer(RuntimeOrigin::signed(2), 0, 42, 3, None)); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::signed(1), 1, 42, 1), - Error::::UnknownCollection + Error::::UnknownItem ); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::signed(1), 0, 43, 1), - Error::::UnknownCollection + Error::::UnknownItem ); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::signed(1), 0, 42, 4), @@ -926,11 +968,11 @@ fn cancel_approval_works_with_force() { assert_ok!(Nfts::approve_transfer(RuntimeOrigin::signed(2), 0, 42, 3, None)); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::root(), 1, 42, 1), - Error::::UnknownCollection + Error::::UnknownItem ); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::root(), 0, 43, 1), - Error::::UnknownCollection + Error::::UnknownItem ); assert_noop!( Nfts::cancel_approval(RuntimeOrigin::root(), 0, 42, 4), @@ -1041,7 +1083,7 @@ fn max_supply_should_work() { assert_ok!(Nfts::destroy( RuntimeOrigin::signed(user_id), collection_id, - Collection::::get(collection_id).unwrap().destroy_witness() + Nfts::get_destroy_witness(&collection_id).unwrap() )); assert!(!CollectionMaxSupply::::contains_key(collection_id)); }); @@ -1137,7 +1179,7 @@ fn set_price_should_work() { assert_noop!( Nfts::set_price(RuntimeOrigin::signed(user_id), collection_id, item_1, Some(2), None), - Error::::ItemsNotTransferable + Error::::ItemsNonTransferable ); }); } @@ -1276,7 +1318,7 @@ fn buy_item_should_work() { }); assert_noop!( buy_item_call.dispatch(RuntimeOrigin::signed(user_2)), - Error::::ItemsNotTransferable + Error::::ItemsNonTransferable ); // unlock the collection @@ -1824,3 +1866,33 @@ fn pallet_level_feature_flags_should_work() { ); }) } + +#[test] +fn group_roles_by_account_should_work() { + new_test_ext().execute_with(|| { + assert_eq!(Nfts::group_roles_by_account(vec![]), vec![]); + + let account_to_role = Nfts::group_roles_by_account(vec![ + (3, CollectionRole::Freezer), + (1, CollectionRole::Issuer), + (2, CollectionRole::Admin), + ]); + let expect = vec![ + (1, CollectionRoles(CollectionRole::Issuer.into())), + (2, CollectionRoles(CollectionRole::Admin.into())), + (3, CollectionRoles(CollectionRole::Freezer.into())), + ]; + assert_eq!(account_to_role, expect); + + let account_to_role = Nfts::group_roles_by_account(vec![ + (3, CollectionRole::Freezer), + (2, CollectionRole::Issuer), + (2, CollectionRole::Admin), + ]); + let expect = vec![ + (2, CollectionRoles(CollectionRole::Issuer | CollectionRole::Admin)), + (3, CollectionRoles(CollectionRole::Freezer.into())), + ]; + assert_eq!(account_to_role, expect); + }) +} diff --git a/frame/nfts/src/types.rs b/frame/nfts/src/types.rs index 6ed57e4da25e5..0122a817229ac 100644 --- a/frame/nfts/src/types.rs +++ b/frame/nfts/src/types.rs @@ -56,14 +56,8 @@ impl_incrementable!(u8, u16, u32, u64, u128, i8, i16, i32, i64, i128); #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, TypeInfo, MaxEncodedLen)] pub struct CollectionDetails { - /// Can change `owner`, `issuer`, `freezer` and `admin` accounts. + /// Collection's owner. pub(super) owner: AccountId, - /// Can mint tokens. - pub(super) issuer: AccountId, - /// Can thaw tokens, force transfers and burn tokens from any account. - pub(super) admin: AccountId, - /// Can freeze tokens. - pub(super) freezer: AccountId, /// The total balance deposited for the all storage associated with this collection. /// Used by `destroy`. pub(super) total_deposit: DepositBalance, @@ -84,8 +78,8 @@ pub struct DestroyWitness { /// The total number of items in this collection that have outstanding item metadata. #[codec(compact)] pub item_metadatas: u32, - #[codec(compact)] /// The total number of attributes for this collection. + #[codec(compact)] pub attributes: u32, } @@ -301,3 +295,37 @@ impl PalletFeatures { } } impl_codec_bitflags!(PalletFeatures, u64, PalletFeature); + +/// Support for up to 8 different roles for collections. +#[bitflags] +#[repr(u8)] +#[derive(Copy, Clone, RuntimeDebug, PartialEq, Eq, Encode, Decode, MaxEncodedLen, TypeInfo)] +pub enum CollectionRole { + /// Can mint items. + Issuer, + /// Can freeze items. + Freezer, + /// Can thaw items, force transfers and burn items from any account. + Admin, +} + +/// A wrapper type that implements `Codec`. +#[derive(Clone, Copy, PartialEq, Eq, Default, RuntimeDebug)] +pub struct CollectionRoles(pub BitFlags); + +impl CollectionRoles { + pub fn none() -> Self { + Self(BitFlags::EMPTY) + } + pub fn has_role(&self, role: CollectionRole) -> bool { + self.0.contains(role) + } + pub fn add_role(&mut self, role: CollectionRole) { + self.0.insert(role); + } + pub fn max_roles() -> u8 { + let all: BitFlags = BitFlags::all(); + all.len() as u8 + } +} +impl_codec_bitflags!(CollectionRoles, u8, CollectionRole);