Make module.Manager.RunMigrations handle new modules correctly #8988
Conversation
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=f44b0ad374e44d5ba3a9f7924ac2767a |
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=1459145c4b51e354c09d071bb74546735a10c094 |
Codecov Report
@@ Coverage Diff @@
## master #8988 +/- ##
==========================================
+ Coverage 58.88% 58.90% +0.02%
==========================================
Files 571 571
Lines 32120 32115 -5
==========================================
+ Hits 18914 18918 +4
+ Misses 10984 10978 -6
+ Partials 2222 2219 -3
|
There was a problem hiding this comment.
Yes, good catch.
We have one small test for RunMigrations here, do you have ideas for better tests?
I'm running today some manual tests using gaia, will try to find a way to incorporate them in the SDK somehow (maybe inside cosmovisor?)
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=70fb6a51da6a45a39ea4f467c08c4809 |
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=fac90784f5f4d3842e656cbce3c1b105c1470089 |
|
|
||
| // only run migrations when the from version is > 0 | ||
| // from version will be 0 when a new module is added and migrations shouldn't be run in this case | ||
| if fromVersion > 0 { |
There was a problem hiding this comment.
I'm thinking about the hub right now. It has fromVersion == 0 for all modules.
I agree with skipping & calling InitGenesis #8989 (with an empty genesis) for new modules with fromVersion == 0. However, for the hub with pre-existing modules with fromversion == 0, we should do something too.
The easiest way I can think of is add a hardcoded map of old module versions inside the hub's UpgradeHandler, something like this
There was a problem hiding this comment.
Yep I see no other choice if this is the first upgrade. We need to make sure this is well documented.
That's fine. It would be nice to have a case for new module.
Sure, if you can figure out how to do that! |
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=7e4d96bda13b4a828edea520395e2866 |
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=697d8d52f97aa50d15a6d0819ef7f713eb3bbbdf |
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=1da5b12c3ee0489a8c61621241f34edc |
|
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=631e807f0a121573aa88104e77ca23c2f22e1f36 |
Came across this when looking at the migrations code.
Also I noticed there are actually no tests for
RunMigrations. Should we add them @AmauryM ?Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes