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
fix(pallet-allocations): fix try-runtime and the migration code #643
Conversation
Remove the attempt to remove CoinsConsumed which was already removed in a previous deployment of release 2.0.21. Fix the try-runtime test which could have indicated this error to us before merging the migration code.
Codecov Report
@@ Coverage Diff @@
## master #643 +/- ##
==========================================
+ Coverage 88.47% 88.66% +0.18%
==========================================
Files 35 35
Lines 6292 6279 -13
==========================================
Hits 5567 5567
+ Misses 725 712 -13
Continue to review full report at Codecov.
|
…r own way of tests through the try-runtime feature
V0_0_0Legacy, // To handle Legacy version | ||
V2_0_21, | ||
V0, // Legacy version | ||
V1, // Adds storage info |
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.
Is it not right way to enumerate by Pallet version ?
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 think is it's hard to aim at a release version inside the code. For example, here 2.0.21 is already released but this code implies the change should have been part of that.
|
||
#[cfg(feature = "try-runtime")] | ||
use crate::BalanceOf; | ||
pub fn on_runtime_upgrade<T: Config>() -> 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.
Can we take this as suggested migration immplementation pattern and apply to other PR's ?
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 would suggest it
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.
LGTM, Added some clarification questions on implementation.
Remove the attempt to remove CoinsConsumed which was already removed in a previous deployment of release 2.0.21. Fix the try-runtime test which could have indicated this error to us before merging the migration code.