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.
Fix Society v2 migration #14421
Fix Society v2 migration #14421
Changes from 28 commits
984ac11
d67ae41
93cca30
e7b08e1
315c1f4
54f3108
7ea67b3
039a55b
81725a2
4929c1a
ab2aa97
61af5d9
46913c3
8cce590
88e0836
af97daa
0b4abd8
400304c
afd9e93
c7e4ca3
e1f3c01
1a6f08e
568440b
2897ad5
039d3e1
c1dd3e7
f17bf46
b07294a
a769e38
b039f09
f344dcb
93d7ae4
803d114
e1c2599
1f9e5fe
87b3ee1
9f0e4f9
28c36cc
9878670
3461900
5b6b1a1
b0f08e0
20083ca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We don't want migrations to fail when they are already applied. I know this is only defensive and will not trigger on chain, but it could fail in the try runtime checks.
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.
But this should not happen anymore since we wrap it in a
VersionedRuntimeUpgrade
.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.
Only when you enable the experimental feature. But still, just because there is maybe some way to prevent this, I don't see any reason we can not fix it at the root of the problem.
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'll change it back to a log::warn.
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.
Keep in mind,
VersionedRuntimeUpgrade
is intended to allow developers to not need to worry about any version checking in theirOnRuntimeUpgrade
hooks.So when it is out of experimental, we need some way to mark an upgrade in a way so it can only be used when wrapped in
VersionedRuntimeUpgrade
.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.
You signal this by not making the "inner migration" public and only export the wrapped
VersionedRuntimeUpgrade
type.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 already tried removing pub from the
Unchecked
migration actually but it results in a compiler error:I tried hacking on this a little and the only way I could make it work was via a wrapper struct, but it's really verbose and ugly:
Not finished by any means, just a really rough PoC.
Could be a good candidate for a macro, but I would like to avoid that and keep it vanilla Rust for the sake of simplicity if we can.
Let me know if you have any other ideas & if I may be missing something easy, otherwise I'll create an issue to track establishing a pattern for private unchecked types and updating examples.
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.
Yeah the type needs to be public when it appears in the generic. However, you can make the type public but not export it from the
migrations
module ;)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.
Rococo
old::Members
is empty, makingcan_migrate
return a false negative.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.
Now you only return true if the list is empty? I'm confused.
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.
Members
are the new members. It returns true if new members are empty, signifying the old members have not yet been migrated.old::Members
is empty on Rococo, which was causingcan_migrate
to return false when it is supposed to return true (otherwise the on-chain version will not be updated to 2).