-
Notifications
You must be signed in to change notification settings - Fork 101
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
Bump versions for sessions keys migration #263
Bump versions for sessions keys migration #263
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.
It is an interesting tie to safeguard a migration to a specific spec version. But the downside is that an emergency upgrade like 1.1.2 can disrupt the flow :)
Somewhat more ideally, a migration would be smart enough to know if it is executed or not. In some migrations, this is possible, in some it is harder but can be achieved if we attach a unique identifier to all impls of OnRuntimeUpgrade
and then we will never have to worry about duplicate or stale migrations again.
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.
Ty!
@kianenigma yes, exactly. The root cause of the problem is that some runtime structures, as |
/merge |
@@ -1860,7 +1860,7 @@ pub mod migrations { | |||
/// Upgrade Session keys to exclude `ImOnline` key. | |||
/// When this is removed, should also remove `OldSessionKeys`. | |||
pub struct UpgradeSessionKeys; | |||
const UPGRADE_SESSION_KEYS_FROM_SPEC: u32 = 1001002; | |||
const UPGRADE_SESSION_KEYS_FROM_SPEC: u32 = 1001003; |
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 would be safer to create a storage key like :upgrade_session_keys_migration_executed
and then manually cleaning it up via governance afterwards.
Fixes #262
CC @bkchr