This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
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.
Auto-incremental CollectionId #11796
Auto-incremental CollectionId #11796
Changes from 41 commits
6020e33
995ee51
d109bb1
bc1544d
6d75a4a
56bbfa7
dd1be35
9945968
6756619
72c5014
3a26210
b974c52
95c2aaf
3acff15
364f01b
d75b565
0ac9b35
9001ca7
83d4233
036a494
8d76e7d
1a05a4e
4c03867
3e4d3c3
a0d4c88
c946847
ab21c59
1191946
8fb3fbc
2a991e7
f51f9f6
16e2267
47ff81a
0b4dce4
45a21e0
7066000
e37673d
2342b07
576b21c
167c84c
9319f76
01f0f20
fb33c9a
87effd9
bf61196
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
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.
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
)? @shawntabriziThere 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.
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