-
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
1472 upgrade to polkadot release v0942 #1552
Conversation
ae1f8ad
to
e0754c6
Compare
pallets/messages/src/rpc/src/lib.rs
Outdated
@@ -100,7 +99,7 @@ where | |||
'loops: for block_number in from..to { | |||
let list: Vec<MessageResponse> = api | |||
.get_messages_by_schema_and_block( | |||
&at, | |||
self.client.info().best_hash, |
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 think this is a function call and not memoized, so we'd need to keep the at so it doesn't get called n times.
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.
Nice catch, I didn't catch the loop part, focused on the BlockId removal.
pallets/msa/src/rpc/src/lib.rs
Outdated
let results = delegator_msa_ids | ||
.par_iter() | ||
.map(|delegator_msa_id| { | ||
let api = self.client.runtime_api(); | ||
// api.has_delegation returns Result<bool, ApiError>), so _or(false) should not happen, | ||
// but just in case, protect against panic | ||
let has_delegation: bool = match api.has_delegation( | ||
&at, | ||
self.client.info().best_hash, |
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.
Another here that is inside a loop
/// Storage: System Account (r:999 w:999) | ||
/// Proof: System Account (max_values: None, max_size: Some(128), added: 2603, mode: MaxEncodedLen) | ||
/// The range of component `u` is `[1, 1000]`. | ||
fn upgrade_accounts(u: u32, ) -> Weight { |
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.
Are we going to need to call this once it is deployed to do the migration?
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 think so, there is a release note about needing to run an upgrade script. I grabbed this code from substrate to get things compiling, I'll check the benchmark output to see if this comes out correct.
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 know it is still a WIP, but a few notes, and you'll want to trigger all the benchmarks once it is ready to go.
Replaced with #1590 |
Goal
The goal of this PR is
Closes
Discussion
Checklist