This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Auto-incremental CollectionId #11796
Merged
paritytech-processbot
merged 45 commits into
paritytech:master
from
Szegoo:autoincrement-collectionId
Jul 30, 2022
Merged
Changes from all commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
6020e33
autoincrementing CollectionId
Szegoo 995ee51
fix
Szegoo d109bb1
benchmarking fix
Szegoo bc1544d
fmt
Szegoo 6d75a4a
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo 56bbfa7
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo dd1be35
fix
Szegoo 9945968
Merge branch 'autoincrement-collectionId' of https://github.com/Szego…
Szegoo 6756619
update before checking
Szegoo 72c5014
fmt
Szegoo 3a26210
fix
Szegoo b974c52
fmt
Szegoo 95c2aaf
commit
Szegoo 3acff15
tests & fix
364f01b
fix
d75b565
commit
Szegoo 0ac9b35
docs
Szegoo 9001ca7
safe math
Szegoo 83d4233
unexpose function
Szegoo 036a494
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo 8d76e7d
benchmark
Szegoo 1a05a4e
fmt
Szegoo 4c03867
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo 3e4d3c3
better naming
Szegoo a0d4c88
Merge branch 'autoincrement-collectionId' of https://github.com/Szego…
Szegoo c946847
fix?
Szegoo ab21c59
Merge branch 'master' into autoincrement-collectionId
Szegoo 1191946
merge fixes
Szegoo 8fb3fbc
fmt
Szegoo 2a991e7
Merge branch 'master' of https://github.com/paritytech/substrate into…
f51f9f6
".git/.scripts/bench-bot.sh" pallet dev pallet_uniques
16e2267
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo 47ff81a
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo 0b4dce4
wrong weight
Szegoo 45a21e0
Update frame/uniques/src/lib.rs
Szegoo 7066000
Update frame/uniques/src/lib.rs
Szegoo e37673d
using substrate trait instead of num-traits
Szegoo 2342b07
remove unnecessary trait
Szegoo 576b21c
emit NextCollectionIdIncremented in do_create_collection
Szegoo 167c84c
fix in benchmarks
Szegoo 9319f76
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo 01f0f20
check for event & group import
Szegoo fb33c9a
Merge branch 'autoincrement-collectionId' of https://github.com/Szego…
Szegoo 87effd9
Merge branch 'paritytech:master' into autoincrement-collectionId
Szegoo bf61196
docs
Szegoo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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::*; | ||
|
@@ -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; | ||
|
||
/// The type used to identify a unique item within a collection. | ||
type ItemId: Member + Parameter + MaxEncodedLen + Copy; | ||
|
@@ -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 = ()> { | ||
|
@@ -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, | ||
|
@@ -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. | ||
|
@@ -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. | ||
/// | ||
|
@@ -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)?; | ||
|
||
|
@@ -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 | ||
|
@@ -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(), | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we should call this in the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 theCollectionId
, which isn't an unsigned integer (but contains variants that can accept unsigned ints).There was a problem hiding this comment.
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 forCollectionId
, and not all variants store a uint.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
afterwardsThere was a problem hiding this comment.
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 theCollectionId
as I suggested earlier, so this means that we are not going to store this internalref_id
field, right?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.