-
Notifications
You must be signed in to change notification settings - Fork 19
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
bugfix: schema name migration #2012
Conversation
Codecov ReportAttention: Patch coverage 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.
Looks good--one question/discussion point: would it be worthwhile to add a script/program somewhere that could be run (manually, I guess) against mainnet after an upgrade to validate things from a client perspective? Might be a useful pattern to have in place for future migrations...
Otherwise, 🚢 it!
// check to ensure updates took place | ||
let known_schemas = get_known_schemas::<T>(); | ||
for (schema_id, schema_name) in known_schemas.into_iter() { | ||
// safe to use unwrap since only used in try_runtime |
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.
Curious nit: why doesn't lint complain about unwrap here? I don't see it disabled at the top of the file?
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.
It might be since it's under "try-runtime" feature flag
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.
A few non-blocking comments, but looks good. Still trying to run the try-runtime locally, but those issues are on me.
- Reviewed Code
- Trying to run locally
@@ -1,4 +1,2 @@ | |||
/// migrations to v2 | |||
pub mod v2; | |||
/// migrations to v3 | |||
pub mod v3; |
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.
Shouldn't this one go away as well?
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 wanted to keep at least the last migration for each pallet for easier access but I can remove this usage part so it does not get included in the code.
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.
- Reviewed code, looks good.
🚢 it!
Why not submit transactions for these instead of writing a migration script? |
Good question!
|
Goal
The goal of this PR is to make to fix the schema names on main-net that incorrectly got updated with test-net values.
Closes #2006
Changes
Checklist
Verification on Mainnet
Verification on Paseo