-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor: improve x/upgrade
app wiring (part of upgrade audit)
#14216
Conversation
x/upgrade
app configx/upgrade
app wiring
x/upgrade/module.go
Outdated
return UpgradeOutputs{UpgradeKeeper: k, Module: m, GovHandler: gh, BaseAppOption: baseappOpt} | ||
} | ||
|
||
func InvokeKeeperOptions(upgradeKeeper keeper.Keeper, modules map[string]appmodule.AppModule, baseApp *baseapp.BaseApp) { |
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.
This does not work because BaseApp is inaccessible. Unfortunately, I don't think this can be in app config, as the store is not available until we call Build
and after that the baseapp options are invoked.
7573b51
to
fc065c4
Compare
fc065c4
to
666a54d
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.
Thanks for refactoring this to make it work without the InitChainer @julienrbrt ! Just wondering how much we should handle legacy module registration with this invoked. Maybe for legacy module registration we require a manual InitChainer? That should be okay but we should document the expectations clearly
} | ||
|
||
// NewMsgServerImpl returns an implementation of the upgrade MsgServer interface | ||
// for the provided Keeper. | ||
func NewMsgServerImpl(k Keeper) types.MsgServer { | ||
func NewMsgServerImpl(k *Keeper) types.MsgServer { |
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.
Why is a reference needed?
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.
Because we want to set the version setter to the keeper here: https://github.com/cosmos/cosmos-sdk/pull/14216/files#diff-bde406b9030d9305e4d44025b556dae6d7c3dd3c924d28038d846910631a8a44R218-R220
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.
This is an improvement. It's maybe not 100% ideal but probably good enough for now. Would appreciate others input here specifically on changing the keeper to use a pointer. Ideally all this stuff works smoothly with app wiring and doesn't feel hacky. This feels less hacky than before but maybe not 100% clean either.
@AmauryM are you able to take a look at 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.
Instead of using SetInitVersionMap
, could we pass a new arg into the upgrade keeper?
- with depinject, this new arg is provided by the app
- in old simapp, we do it in app.go
here's a branch: https://github.com/cosmos/cosmos-sdk/compare/julien/upgrade...am/julien-upgrade?expand=1
It feels slightly cleaner to me than the Setter/pointer version here.
Oh wow, this looks better because this takes in account modules registered using EDIT: actually it still does not because |
dfd09ff
to
2eabd59
Compare
[Cosmos SDK - SimApp] Kudos, SonarCloud Quality Gate passed! |
[Cosmos SDK] Kudos, SonarCloud Quality Gate passed! |
Merging this now to conclude the audit, as it is indeed better than currently. We can improve in a follow-up if there is a better way. |
) (cherry picked from commit 05a6da3) # Conflicts: # x/upgrade/abci_test.go
Description
ref #14209 (comment), Closes #14187
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change