-
Notifications
You must be signed in to change notification settings - Fork 52
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
Update IBC to v4.2.0 #599
Update IBC to v4.2.0 #599
Conversation
update cosmos-sdk to v0.45.10
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #599 +/- ##
==========================================
+ Coverage 42.47% 42.87% +0.40%
==========================================
Files 127 127
Lines 11827 11715 -112
==========================================
Hits 5023 5023
+ Misses 6324 6212 -112
Partials 480 480
|
app/upgrade_handler.go
Outdated
app.CrisisKeeper.InitGenesis(ctx, crisisGenesis) | ||
|
||
// transfer module consensus version has been bumped to 2 | ||
fromVM[ibctransfer.ModuleName] = app.mm.Modules[ibctransfer.ModuleName].ConsensusVersion() |
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 this correct? This will disable IBC transfer store upgrade
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.
emm, I have removed this
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 except for one optimization
crisisGenesis.ConstantFee.Denom = app.StakingKeeper.BondDenom(ctx) | ||
app.CrisisKeeper.InitGenesis(ctx, crisisGenesis) | ||
|
||
// transfer module consensus version has been bumped to 2 | ||
ctx.Logger().Info("Start to run module migrations...") | ||
newVersionMap, err := app.mm.RunMigrations(ctx, app.configurator, fromVM) |
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.
now we can just return this without anything else in the function body
crisisGenesis.ConstantFee.Denom = app.StakingKeeper.BondDenom(ctx) | ||
app.CrisisKeeper.InitGenesis(ctx, crisisGenesis) | ||
|
||
// transfer module consensus version has been bumped to 2 |
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.
If this line of comment means to show all modules that are going to do upgrading, we could provide a full list of them. Or it may be better to just remove the line.
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.
At present, only transfer is upgraded, and other modules are not upgraded for the time being.
update cosmos-sdk to v0.45.10
Closes: #XXX
Related: #XXX
Description
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)