-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add democracy migration and fix contracts migration #12
Conversation
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 good!
Getting "Corrupted State" errors:
|
Solved through rebuilds 🤷 |
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 good, if every migration is using MaxBlockWeight, does it sum those? Wouldn't that make the weight too larger to go in, in a single tx?
fn on_runtime_upgrade() -> Weight { | ||
for i in 0..=SegmentIndex::get() { | ||
UnderConstruction::migrate_key_from_blake(i); | ||
} | ||
// TODO: determine weight | ||
0 | ||
T::MaximumBlockWeight::get() |
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 use babe so not sure how necessary this one is?
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.
True, probably isn't. Don't think it's worth it to remove it, though.
frame/democracy/src/migration.rs
Outdated
} | ||
|
||
pub fn migrate_hasher<T: Trait>() -> Weight { | ||
// TODO: is this valid? |
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.
So far, we haven't had an vetos/cancellations. Should be fine!
frame/democracy/src/migration.rs
Outdated
} | ||
|
||
pub fn migrate_remove_unused_storage<T: Trait>() -> Weight { | ||
// TODO: is this valid? |
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.
So we delete things after we move them?
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'm not sure where VotersFor is defined or if we have it on our substrate commit that mainnet is pegged to. What are the potential reprecussions of removing these?
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.
VotersFor
seems empty on current edgeware. (Checked on https://polkadot.js.org/apps/#/chainstate with some indices. referendumCount
is 8.)
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.
So we delete things after we move them?
Nope, the ones deleted here are not moved.
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.
The repercussions I can see are the following: This migration just drops some state that currently doesn't exist on edgeware. If people were to create proposals and vote on them while this migration is in process, some data might be lost. AFAICT this would not corrupt any state, people would just have to vote again.
These upgrades are performed by |
So returning |
This PR migrates democracy for the Edgeware node migration.
Includes migration code inspired by:
Other notes:
Related to hicommonwealth/edgeware-node#164