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

[Uniques V2] Refactor roles #12437

Merged
merged 78 commits into from
Oct 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
97a7e30
Basics
jsidorenko Sep 21, 2022
a85608f
WIP: change the data format
jsidorenko Sep 23, 2022
6db840a
Refactor
jsidorenko Sep 24, 2022
fa3a2a7
Remove redundant new() method
jsidorenko Sep 24, 2022
472a753
Rename settings
jsidorenko Sep 24, 2022
2611dd4
Enable tests
jsidorenko Sep 24, 2022
52d7d38
Merge branch 'js/uniques-v2-main-branch' into js/uniques-v2-feature-f…
jsidorenko Sep 24, 2022
46a6b7e
Chore
jsidorenko Sep 24, 2022
5ff7048
Change params order
jsidorenko Sep 24, 2022
f7ddfed
Delete the config on collection removal
jsidorenko Sep 24, 2022
d0d1d60
Chore
jsidorenko Sep 24, 2022
7d4b31f
Remove redundant system features
jsidorenko Sep 24, 2022
dd84637
Rename force_item_status to force_collection_status
jsidorenko Sep 24, 2022
4920a0c
Update node runtime
jsidorenko Sep 26, 2022
7456e73
Chore
jsidorenko Sep 26, 2022
9ff45b8
Remove thaw_collection
jsidorenko Sep 26, 2022
c41a48c
Chore
jsidorenko Sep 26, 2022
fee3ade
Connect collection.is_frozen to config
jsidorenko Sep 26, 2022
a9ab6f8
Allow to lock the collection in a new way
jsidorenko Sep 26, 2022
254816d
Move free_holding into settings
jsidorenko Sep 26, 2022
5878197
Connect collection's metadata locker to feature flags
jsidorenko Sep 26, 2022
95e21fb
DRY
jsidorenko Sep 27, 2022
17b7c9e
Chore
jsidorenko Sep 27, 2022
dc6d2f0
Connect pallet level feature flags
jsidorenko Sep 27, 2022
cc64873
Prepare tests for the new changes
jsidorenko Sep 27, 2022
5153080
Implement Item settings
jsidorenko Sep 27, 2022
08a2048
Allow to lock the metadata or attributes of an item
jsidorenko Sep 27, 2022
1551714
Common -> Settings
jsidorenko Sep 27, 2022
8830a4c
Extract settings related code to a separate file
jsidorenko Sep 27, 2022
f8800df
Move feature flag checks inside the do_* methods
jsidorenko Sep 28, 2022
3ccf04e
Split settings.rs into parts
jsidorenko Sep 28, 2022
f83fb73
Extract repeated code into macro
jsidorenko Sep 28, 2022
df97cce
Extract macros into their own file
jsidorenko Sep 28, 2022
3bb6eb5
Chore
jsidorenko Sep 28, 2022
8563a31
Fix traits
jsidorenko Sep 29, 2022
09aca13
Fix traits
jsidorenko Sep 29, 2022
114e519
Test SystemFeatures
jsidorenko Sep 29, 2022
d6536fc
Fix benchmarks
jsidorenko Sep 29, 2022
09a4ed6
Add missing benchmark
jsidorenko Sep 29, 2022
d408b32
Fix node/runtime/lib.rs
jsidorenko Sep 29, 2022
fd86e41
".git/.scripts/bench-bot.sh" pallet dev pallet_nfts
Oct 3, 2022
c002f8b
Keep item's config on burn if it's not empty
jsidorenko Oct 5, 2022
b16435a
Merge branch 'js/uniques-v2-main-branch' into js/uniques-v2-feature-f…
jsidorenko Oct 5, 2022
d0ee824
Fix the merge artifacts
jsidorenko Oct 5, 2022
0eb568e
Fmt
jsidorenko Oct 5, 2022
998691e
Add SystemFeature::NoSwaps check
jsidorenko Oct 6, 2022
fbc7906
Refactor roles structure
jsidorenko Oct 6, 2022
b8c6f5e
Rename SystemFeatures to PalletFeatures
jsidorenko Oct 7, 2022
8bef776
Rename errors
jsidorenko Oct 7, 2022
ddaa884
Add docs
jsidorenko Oct 7, 2022
4c01fc5
Change error message
jsidorenko Oct 7, 2022
fa9d471
Rework pallet features
jsidorenko Oct 14, 2022
ad980de
Move macros
jsidorenko Oct 14, 2022
8532179
Change comments
jsidorenko Oct 14, 2022
fc09bbd
Fmt
jsidorenko Oct 14, 2022
e3f67ad
Refactor Incrementable
jsidorenko Oct 14, 2022
1966a58
Use pub(crate) for do_* functions
jsidorenko Oct 14, 2022
39c84b4
Update comments
jsidorenko Oct 14, 2022
71ff09e
Refactor freeze and lock functions
jsidorenko Oct 14, 2022
e3778ad
Rework Collection config and Item confg api
jsidorenko Oct 14, 2022
f84563d
Chore
jsidorenko Oct 14, 2022
5d5cc1c
Merge branch 'js/uniques-v2-main-branch' into js/uniques-v2-feature-f…
jsidorenko Oct 14, 2022
05b6d11
Make clippy happy
jsidorenko Oct 14, 2022
08ca688
Chore
jsidorenko Oct 15, 2022
1760e7e
Merge branch 'js/uniques-v2-feature-flags' into js/uniques-v2-roles-r…
jsidorenko Oct 17, 2022
26adcc9
Fix artifacts
jsidorenko Oct 17, 2022
0225479
Address comments
jsidorenko Oct 17, 2022
c944afc
Further refactoring
jsidorenko Oct 17, 2022
a665764
Add comments
jsidorenko Oct 17, 2022
2461bc0
Add tests for group_roles_by_account()
jsidorenko Oct 17, 2022
6608f87
Update frame/nfts/src/impl_nonfungibles.rs
jsidorenko Oct 18, 2022
e6c56d6
Add test
jsidorenko Oct 18, 2022
414bd8b
Replace Itertools group_by with a custom implementation
jsidorenko Oct 18, 2022
7ee0d2e
Merge branch 'js/uniques-v2-main-branch' into js/uniques-v2-roles-ref…
jsidorenko Oct 18, 2022
e688ad5
ItemsNotTransferable => ItemsNonTransferable
jsidorenko Oct 18, 2022
fc8d341
Update frame/nfts/src/features/roles.rs
jsidorenko Oct 19, 2022
2a3d11e
Address PR comments
jsidorenko Oct 19, 2022
e3ba2b2
Add missed comment
jsidorenko Oct 20, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frame/nfts/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ 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 caller = Collection::<T, I>::get(T::Helper::collection(0)).unwrap().admin;
let caller = Collection::<T, I>::get(T::Helper::collection(0)).unwrap().owner;
if caller != whitelisted_caller() {
whitelist_account!(caller);
}
Expand Down
22 changes: 12 additions & 10 deletions frame/nfts/src/features/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
collection: T::CollectionId,
lock_config: CollectionConfig,
) -> DispatchResult {
let details =
Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
ensure!(origin == details.freezer, Error::<T, I>::NoPermission);

ensure!(
Self::has_role(&collection, &origin, CollectionRole::Freezer),
Error::<T, I>::NoPermission
);
CollectionConfigOf::<T, I>::try_mutate(collection, |maybe_config| {
let config = maybe_config.as_mut().ok_or(Error::<T, I>::NoConfig)?;

Expand All @@ -51,9 +51,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
collection: T::CollectionId,
item: T::ItemId,
) -> DispatchResult {
let collection_details =
Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
ensure!(collection_details.freezer == origin, Error::<T, I>::NoPermission);
ensure!(
Self::has_role(&collection, &origin, CollectionRole::Freezer),
Error::<T, I>::NoPermission
);

let mut config = Self::get_item_config(&collection, &item)?;
if !config.has_disabled_setting(ItemSetting::Transferable) {
Expand All @@ -70,9 +71,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
collection: T::CollectionId,
item: T::ItemId,
) -> DispatchResult {
let collection_details =
Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
ensure!(collection_details.freezer == origin, Error::<T, I>::NoPermission);
ensure!(
Self::has_role(&collection, &origin, CollectionRole::Freezer),
Error::<T, I>::NoPermission
);

let mut config = Self::get_item_config(&collection, &item)?;
if config.has_disabled_setting(ItemSetting::Transferable) {
Expand Down
1 change: 1 addition & 0 deletions frame/nfts/src/features/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@
pub mod atomic_swap;
pub mod buy_sell;
pub mod lock;
pub mod roles;
pub mod settings;
69 changes: 69 additions & 0 deletions frame/nfts/src/features/roles.rs
Original file line number Diff line number Diff line change
@@ -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<T: Config<I>, I: 'static> Pallet<T, I> {
Copy link
Contributor

@muharem muharem Oct 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With multiple files the pallet structure looks unfamiliar to me.
The way we structure the code is a matter of taste, but keeping the structure similar within one project helps a lot.

I would rather keep the structure similar to the other pallets or have a well defined proposal with the new structure.

When I see this new structure, I have many questions, like why i see see impl of one type Pallet in different modules, why we dont have roles types in roles module, whats difference between functions.rs and features/*?

This is not a blocker.

/// 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::<T, I>::clear_prefix(
&collection_id,
CollectionRoles::max_roles() as u32,
None,
);
ensure!(res.maybe_cursor.is_none(), Error::<T, I>::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(
jsidorenko marked this conversation as resolved.
Show resolved Hide resolved
collection_id: &T::CollectionId,
account_id: &T::AccountId,
role: CollectionRole,
) -> bool {
CollectionRoleOf::<T, I>::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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to have a doc and tests for this functions

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()
}
}
22 changes: 12 additions & 10 deletions frame/nfts/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
let collection_config = Self::get_collection_config(&collection)?;
ensure!(
collection_config.is_setting_enabled(CollectionSetting::TransferableItems),
Error::<T, I>::ItemsNotTransferable
Error::<T, I>::ItemsNonTransferable
);

let item_config = Self::get_item_config(&collection, &item)?;
Expand Down Expand Up @@ -93,15 +93,19 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
collection,
CollectionDetails {
owner: owner.clone(),
issuer: admin.clone(),
admin: admin.clone(),
freezer: admin,
total_deposit: deposit,
items: 0,
item_metadatas: 0,
attributes: 0,
},
);
CollectionRoleOf::<T, I>::insert(
collection,
admin,
CollectionRoles(
CollectionRole::Admin | CollectionRole::Freezer | CollectionRole::Issuer,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the admin be able to perform all the operations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the prev logic - no :) I only reworked the existing data structure into a new one without changing the business rules

),
);

let next_id = collection.increment();

Expand Down Expand Up @@ -142,6 +146,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
#[allow(deprecated)]
PendingSwapOf::<T, I>::remove_prefix(&collection, None);
CollectionMetadataOf::<T, I>::remove(&collection);
Self::clear_roles(&collection)?;
#[allow(deprecated)]
Attribute::<T, I>::remove_prefix((&collection,), None);
CollectionAccount::<T, I>::remove(&collection_details.owner, &collection);
Expand All @@ -165,7 +170,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
item: T::ItemId,
owner: T::AccountId,
config: ItemConfig,
with_details: impl FnOnce(&CollectionDetailsFor<T, I>) -> DispatchResult,
) -> DispatchResult {
ensure!(!Item::<T, I>::contains_key(collection, item), Error::<T, I>::AlreadyExists);

Expand All @@ -175,8 +179,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
let collection_details =
maybe_collection_details.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;

with_details(collection_details)?;

if let Ok(max_supply) = CollectionMaxSupply::<T, I>::try_get(&collection) {
ensure!(collection_details.items < max_supply, Error::<T, I>::MaxSupplyReached);
}
Expand Down Expand Up @@ -218,7 +220,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
pub fn do_burn(
collection: T::CollectionId,
item: T::ItemId,
with_details: impl FnOnce(&CollectionDetailsFor<T, I>, &ItemDetailsFor<T, I>) -> DispatchResult,
with_details: impl FnOnce(&ItemDetailsFor<T, I>) -> DispatchResult,
) -> DispatchResult {
let owner = Collection::<T, I>::try_mutate(
&collection,
Expand All @@ -227,7 +229,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
maybe_collection_details.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;
let details = Item::<T, I>::get(&collection, &item)
.ok_or(Error::<T, I>::UnknownCollection)?;
with_details(collection_details, &details)?;
with_details(&details)?;

// Return the deposit.
T::Currency::unreserve(&collection_details.owner, details.deposit);
Expand Down Expand Up @@ -271,7 +273,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
let collection_config = Self::get_collection_config(&collection)?;
ensure!(
collection_config.is_setting_enabled(CollectionSetting::TransferableItems),
Error::<T, I>::ItemsNotTransferable
Error::<T, I>::ItemsNonTransferable
);

let item_config = Self::get_item_config(&collection, &item)?;
Expand Down
4 changes: 2 additions & 2 deletions frame/nfts/src/impl_nonfungibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,15 +143,15 @@ impl<T: Config<I>, I: 'static> Mutate<<T as SystemConfig>::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(
collection: &Self::CollectionId,
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::<T, I>::NoPermission.into())
Expand Down
Loading