-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: LP v2 migrations #1966
feat: LP v2 migrations #1966
Conversation
0a07256
to
8ec41e9
Compare
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.
No need to merge this PR as DomainRouters
has been completely deprecated. Going to use this PR as base for migrating the previous DomainRouters
to the new settings storage(s).
} | ||
} | ||
|
||
pub mod v2_update_message_queue { |
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.
Note: Dev and Demo were already upgraded to the intermediary storage version 2 which still supported gateway::DomainRouters
storage but removed all routers but the AxelarEvm
ones. The corresponding commit hash is 8ec41e9
Therefore, we need to provide migrations from v0 and v2.
let _ = transactional::with_storage_layer(|| -> DispatchResult { | ||
let (r, w) = Self::migrate_domain_routers()?; | ||
log::info!("{LOG_PREFIX} PRE Migration counts {w:?} updated domains and {} removed domains", r.saturating_sub(w)); | ||
writes = w; | ||
Err(DispatchError::Other("Reverting on purpose")) | ||
}); |
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.
Note: There are two pre_upgrade
hooks in this PR which utilize a transactional layer to ensure the exact migration call which is applied in the corresponding on_runtime_upgrade
does not fail. However, since those migration calls mutate storage, I am reverting the storage changes by failing the transactional layer on purpose.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1966 +/- ##
==========================================
- Coverage 48.99% 48.28% -0.72%
==========================================
Files 183 183
Lines 13201 13403 +202
==========================================
+ Hits 6468 6471 +3
- Misses 6733 6932 +199 ☔ View full report in Codecov by Sentry. |
Message = GatewayMessage<Message, RouterId>, | ||
>, | ||
{ | ||
fn migrate_message_queue(mut reads: u64, mut writes: u64) { |
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 these reads/writes args be references?
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.
They could be yes. But is there any advantage to gain from that? Where do you see an issue with the current style?
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 the caller of this fn - https://github.com/centrifuge/centrifuge-chain/pull/1966/files#diff-701a3e1aba84712266444a4d0cdee431c7a71b7cd635255592de9e26f6d77714R252, if you don't provide mutable references they will remain 0?
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.
Fixed by bf97ee8
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!
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.
Thank you very much for tackling these @wischli!
Would you mind approving? Requiring your approval as code owner. |
Description
RelayerList
storagegateway::DomainRouters
storage toaxelar_router::Configuration
andaxelar_router::ChainNameById
v2::GatewayMessage
tov3::GatewayMessage
Dev
Migration failed here first such that I removed the old entry and re-added it manually.
Demo
Centrifuge Chain
Checklist: