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

Auto-incremental CollectionId #11796

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
6020e33
autoincrementing CollectionId
Szegoo Jul 7, 2022
995ee51
fix
Szegoo Jul 7, 2022
d109bb1
benchmarking fix
Szegoo Jul 7, 2022
bc1544d
fmt
Szegoo Jul 7, 2022
6d75a4a
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo Jul 8, 2022
56bbfa7
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo Jul 11, 2022
dd1be35
fix
Szegoo Jul 11, 2022
9945968
Merge branch 'autoincrement-collectionId' of https://github.com/Szego…
Szegoo Jul 11, 2022
6756619
update before checking
Szegoo Jul 11, 2022
72c5014
fmt
Szegoo Jul 11, 2022
3a26210
fix
Szegoo Jul 11, 2022
b974c52
fmt
Szegoo Jul 11, 2022
95c2aaf
commit
Szegoo Jul 11, 2022
3acff15
tests & fix
Jul 12, 2022
364f01b
fix
Jul 12, 2022
d75b565
commit
Szegoo Jul 12, 2022
0ac9b35
docs
Szegoo Jul 12, 2022
9001ca7
safe math
Szegoo Jul 12, 2022
83d4233
unexpose function
Szegoo Jul 12, 2022
036a494
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo Jul 12, 2022
8d76e7d
benchmark
Szegoo Jul 12, 2022
1a05a4e
fmt
Szegoo Jul 12, 2022
4c03867
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo Jul 12, 2022
3e4d3c3
better naming
Szegoo Jul 13, 2022
a0d4c88
Merge branch 'autoincrement-collectionId' of https://github.com/Szego…
Szegoo Jul 13, 2022
c946847
fix?
Szegoo Jul 13, 2022
ab21c59
Merge branch 'master' into autoincrement-collectionId
Szegoo Jul 14, 2022
1191946
merge fixes
Szegoo Jul 14, 2022
8fb3fbc
fmt
Szegoo Jul 14, 2022
2a991e7
Merge branch 'master' of https://github.com/paritytech/substrate into…
Jul 15, 2022
f51f9f6
".git/.scripts/bench-bot.sh" pallet dev pallet_uniques
Jul 15, 2022
16e2267
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo Jul 15, 2022
47ff81a
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo Jul 21, 2022
0b4dce4
wrong weight
Szegoo Jul 21, 2022
45a21e0
Update frame/uniques/src/lib.rs
Szegoo Jul 25, 2022
7066000
Update frame/uniques/src/lib.rs
Szegoo Jul 25, 2022
e37673d
using substrate trait instead of num-traits
Szegoo Jul 25, 2022
2342b07
remove unnecessary trait
Szegoo Jul 25, 2022
576b21c
emit NextCollectionIdIncremented in do_create_collection
Szegoo Jul 25, 2022
167c84c
fix in benchmarks
Szegoo Jul 25, 2022
9319f76
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo Jul 25, 2022
01f0f20
check for event & group import
Szegoo Jul 25, 2022
fb33c9a
Merge branch 'autoincrement-collectionId' of https://github.com/Szego…
Szegoo Jul 25, 2022
87effd9
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo Jul 30, 2022
bf61196
docs
Szegoo Jul 30, 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
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 16 additions & 9 deletions frame/uniques/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,10 @@ fn create_collection<T: Config<I>, I: 'static>(
let caller_lookup = T::Lookup::unlookup(caller.clone());
let collection = T::Helper::collection(0);
T::Currency::make_free_balance_be(&caller, DepositBalanceOf::<T, I>::max_value());
assert!(Uniques::<T, I>::force_create(
SystemOrigin::Root.into(),
collection,
caller_lookup.clone(),
false,
)
.is_ok());
assert!(
Uniques::<T, I>::force_create(SystemOrigin::Root.into(), caller_lookup.clone(), false,)
.is_ok()
);
(collection, caller, caller_lookup)
}

Expand Down Expand Up @@ -143,7 +140,7 @@ benchmarks_instance_pallet! {
whitelist_account!(caller);
let admin = T::Lookup::unlookup(caller.clone());
T::Currency::make_free_balance_be(&caller, DepositBalanceOf::<T, I>::max_value());
let call = Call::<T, I>::create { collection, admin };
let call = Call::<T, I>::create { admin };
}: { call.dispatch_bypass_filter(origin)? }
verify {
assert_last_event::<T, I>(Event::Created { collection: T::Helper::collection(0), creator: caller.clone(), owner: caller }.into());
Expand All @@ -152,7 +149,7 @@ benchmarks_instance_pallet! {
force_create {
let caller: T::AccountId = whitelisted_caller();
let caller_lookup = T::Lookup::unlookup(caller.clone());
}: _(SystemOrigin::Root, T::Helper::collection(0), caller_lookup, true)
}: _(SystemOrigin::Root, caller_lookup, true)
verify {
assert_last_event::<T, I>(Event::ForceCreated { collection: T::Helper::collection(0), owner: caller }.into());
}
Expand Down Expand Up @@ -408,6 +405,16 @@ benchmarks_instance_pallet! {
}.into());
}

try_increment_id {
let (_, caller, _) = create_collection::<T, I>();
Uniques::<T, I>::set_next_id(0);
}: _(SystemOrigin::Signed(caller.clone()))
verify {
assert_last_event::<T, I>(Event::NextCollectionIdIncremented {
next_id: 1u32.into()
}.into());
}

set_price {
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
Expand Down
15 changes: 15 additions & 0 deletions frame/uniques/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
},
);

let next_id = collection.saturating_add(1u32.into());

CollectionAccount::<T, I>::insert(&owner, &collection, ());
NextCollectionId::<T, I>::set(next_id);

Self::deposit_event(Event::NextCollectionIdIncremented { next_id });
Self::deposit_event(event);
Ok(())
}
Expand Down Expand Up @@ -208,6 +213,16 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Ok(())
}

#[cfg(any(test, feature = "runtime-benchmarks"))]
pub fn set_next_id(count: u32) {
NextCollectionId::<T, I>::set(count.into());
}

#[cfg(test)]
pub fn get_next_id() -> T::CollectionId {
NextCollectionId::<T, I>::get()
}

pub fn do_set_price(
collection: T::CollectionId,
item: T::ItemId,
Expand Down
54 changes: 48 additions & 6 deletions frame/uniques/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ use frame_support::{
};
use frame_system::Config as SystemConfig;
use sp_runtime::{
traits::{Saturating, StaticLookup, Zero},
traits::{AtLeast32BitUnsigned, Saturating, StaticLookup, Zero},
ArithmeticError, RuntimeDebug,
};
use sp_std::prelude::*;
Expand Down Expand Up @@ -92,7 +92,12 @@ pub mod pallet {
type Event: From<Event<Self, I>> + IsType<<Self as frame_system::Config>::Event>;

/// Identifier for the collection of item.
type CollectionId: Member + Parameter + MaxEncodedLen + Copy;
type CollectionId: Member
+ Parameter
+ MaxEncodedLen
+ Copy
+ Default
+ AtLeast32BitUnsigned;
Copy link
Contributor

Choose a reason for hiding this comment

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

@jsidorenko @Szegoo This trait bound here is breaking XCMv3, as we're using a MultiLocation for the CollectionId, which isn't an unsigned integer (but contains variants that can accept unsigned ints).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KiChjang Hmm, I will look more into this, but I am not even sure if it would be possible to have this auto-increment feature since as you said the MultiLocation is used for CollectionId, and not all variants store a uint.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, we should revert this change and really think about how exactly we should implement it. As it is, it's breaking code that was originally working without this PR.

Copy link
Member

@ggwpez ggwpez Aug 15, 2022

Choose a reason for hiding this comment

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

Looks like it is ordered, we could let the caller increment it off-chain.
PS: Not really, since the caller could set it to the highest value...

Copy link
Contributor Author

@Szegoo Szegoo Aug 15, 2022

Choose a reason for hiding this comment

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

Maybe we could for now make this feature optional for some chains that don't support XCM and don't use MultiLocation for theCollectionId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will continue working on this in #12057

Copy link
Contributor

Choose a reason for hiding this comment

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

Super! And pls point into js/uniques-v2-main-branch afterwards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make sure I understand this correctly, we should add an Incrementable trait bound to the CollectionId as I suggested earlier, so this means that we are not going to store this internal ref_id field, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how I understood it as well. @KiChjang Pls correct us if we're wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct, the idea is to let runtime configuration figure out what the next ID is, instead of having it become a concern for the uniques pallet itself.


/// The type used to identify a unique item within a collection.
type ItemId: Member + Parameter + MaxEncodedLen + Copy;
Expand Down Expand Up @@ -266,6 +271,12 @@ pub mod pallet {
pub(super) type CollectionMaxSupply<T: Config<I>, I: 'static = ()> =
StorageMap<_, Blake2_128Concat, T::CollectionId, u32, OptionQuery>;

#[pallet::storage]
/// Stores the `CollectionId` that is going to be used for the next collection.
/// This gets incremented by 1 whenever a new collection is created.
pub(super) type NextCollectionId<T: Config<I>, I: 'static = ()> =
StorageValue<_, T::CollectionId, ValueQuery>;

#[pallet::event]
#[pallet::generate_deposit(pub(super) fn deposit_event)]
pub enum Event<T: Config<I>, I: 'static = ()> {
Expand Down Expand Up @@ -357,6 +368,8 @@ pub mod pallet {
OwnershipAcceptanceChanged { who: T::AccountId, maybe_collection: Option<T::CollectionId> },
/// Max supply has been set for a collection.
CollectionMaxSupplySet { collection: T::CollectionId, max_supply: u32 },
/// Event gets emmited when the `NextCollectionId` gets incremented.
NextCollectionIdIncremented { next_id: T::CollectionId },
/// The price was set for the instance.
ItemPriceSet {
collection: T::CollectionId,
Expand Down Expand Up @@ -408,6 +421,10 @@ pub mod pallet {
MaxSupplyAlreadySet,
/// The provided max supply is less to the amount of items a collection already has.
MaxSupplyTooSmall,
/// The `CollectionId` in `NextCollectionId` is not being used.
Szegoo marked this conversation as resolved.
Show resolved Hide resolved
///
/// This means that you can directly proceed to call `create`.
NextIdNotUsed,
/// The given item ID is unknown.
UnknownItem,
/// Item is not for sale.
Expand Down Expand Up @@ -439,7 +456,6 @@ pub mod pallet {
/// `ItemDeposit` funds of sender are reserved.
///
/// Parameters:
/// - `collection`: The identifier of the new collection. This must not be currently in use.
/// - `admin`: The admin of this collection. The admin is the initial address of each
/// member of the collection's admin team.
///
Expand All @@ -449,9 +465,10 @@ pub mod pallet {
#[pallet::weight(T::WeightInfo::create())]
pub fn create(
origin: OriginFor<T>,
collection: T::CollectionId,
admin: <T::Lookup as StaticLookup>::Source,
) -> DispatchResult {
let collection = NextCollectionId::<T, I>::get();

let owner = T::CreateOrigin::ensure_origin(origin, &collection)?;
let admin = T::Lookup::lookup(admin)?;

Expand All @@ -473,7 +490,6 @@ pub mod pallet {
///
/// Unlike `create`, no funds are reserved.
///
/// - `collection`: The identifier of the new item. This must not be currently in use.
/// - `owner`: The owner of this collection of items. The owner has full superuser
/// permissions
/// over this item, but may later change and configure the permissions using
Expand All @@ -485,13 +501,14 @@ pub mod pallet {
#[pallet::weight(T::WeightInfo::force_create())]
pub fn force_create(
origin: OriginFor<T>,
collection: T::CollectionId,
owner: <T::Lookup as StaticLookup>::Source,
free_holding: bool,
) -> DispatchResult {
T::ForceOrigin::ensure_origin(origin)?;
let owner = T::Lookup::lookup(owner)?;

let collection = NextCollectionId::<T, I>::get();

Self::do_create_collection(
collection,
owner.clone(),
Expand All @@ -502,6 +519,31 @@ pub mod pallet {
)
}

/// Increments the `CollectionId` stored in `NextCollectionId`.
///
/// This is only callable when the next `CollectionId` is already being
/// used for some other collection.
///
/// The origin must be Signed and the sender must have sufficient funds
/// free.
///
/// Emits `NextCollectionIdIncremented` event when successful.
///
/// Weight: `O(1)`
#[pallet::weight(T::WeightInfo::try_increment_id())]
pub fn try_increment_id(origin: OriginFor<T>) -> DispatchResult {
Copy link
Member

@ggwpez ggwpez Jul 25, 2022

Choose a reason for hiding this comment

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

Do you think we should call this in the on_initialize so that a user can just re-send their transaction instead of having to sign two more (try_increment_id + create)? @shawntabrizi

Copy link
Member

@shawntabrizi shawntabrizi Jul 30, 2022

Choose a reason for hiding this comment

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

def no, users can pay the cost here, not the blockchain. in general, this function will not be needed on a new chain, only existing ones

ensure_signed(origin)?;
ensure!(
Collection::<T, I>::contains_key(NextCollectionId::<T, I>::get()),
Error::<T, I>::NextIdNotUsed
);

let next_id = NextCollectionId::<T, I>::get().saturating_add(1u32.into());
NextCollectionId::<T, I>::set(next_id);
Self::deposit_event(Event::NextCollectionIdIncremented { next_id });
Ok(())
}

/// Destroy a collection of fungible items.
///
/// The origin must conform to `ForceOrigin` or must be `Signed` and the sender must be the
Expand Down
Loading