Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: improve x/upgrade app wiring (part of upgrade audit) #14216

Merged
merged 38 commits into from
Dec 14, 2022
Merged

refactor: improve x/upgrade app wiring (part of upgrade audit) #14216

merged 38 commits into from
Dec 14, 2022

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Dec 8, 2022 •
edited
Loading

Description

ref #14209 (comment), Closes #14187

  • Improve app config configuration of x/upgrade.
  • Set the genesis init and export order directly in app config.
  • Do not overwrite initchainer when set in app.

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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@julienrbrt julienrbrt changed the title refactor: improve x/upgrade app config refactor: improve x/upgrade app wiring Dec 8, 2022
@aaronc aaronc self-assigned this Dec 8, 2022
julienrbrt
julienrbrt commented Dec 8, 2022
View reviewed changes
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) {
Copy link
Member Author

@julienrbrt julienrbrt Dec 8, 2022 •
edited
Loading

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.

Base automatically changed from julien/appv2 to main December 8, 2022 22:08
@julienrbrt julienrbrt added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Dec 8, 2022
@julienrbrt julienrbrt requested a review from a team December 13, 2022 16:00
aaronc
aaronc reviewed Dec 13, 2022
View reviewed changes
Copy link
Member

@aaronc aaronc left a 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

x/upgrade/module.go Outdated Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 13, 2022
View reviewed changes
x/upgrade/module.go Fixed Show fixed Hide fixed
alexanderbez
alexanderbez reviewed Dec 13, 2022
View reviewed changes
x/upgrade/keeper/msg_server.go
}

// 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 {
Copy link
Contributor

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?

Copy link
Member Author

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

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 13, 2022
View reviewed changes
x/upgrade/module.go Fixed Show fixed Hide fixed
aaronc
aaronc approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@aaronc aaronc left a 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.

@aaronc
Copy link
Member

aaronc commented Dec 14, 2022

@AmauryM are you able to take a look at this?

amaury1093
amaury1093 reviewed Dec 14, 2022
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a 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.

@julienrbrt
Copy link
Member Author

julienrbrt commented Dec 14, 2022 •
edited
Loading

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: julien?expand=1 (compare)

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 app.RegisterModules . I believe we do not need to manually set it in app.go too. Let me see if it works 👀

EDIT: actually it still does not because RegisterModules is on an app and not on an appBuilder, so we still need the logic in initgenesis.
EDIT2: Tried a few tweaks, but it does not work. The module map is not available at that time, so that's unfortunately not a solution.

@sonarcloud
Copy link

sonarcloud bot commented Dec 14, 2022

[Cosmos SDK - SimApp] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Dec 14, 2022

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@julienrbrt
Copy link
Member Author

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.

@julienrbrt julienrbrt merged commit 05a6da3 into main Dec 14, 2022
@julienrbrt julienrbrt deleted the julien/upgrade branch December 14, 2022 20:21
mergify bot pushed a commit that referenced this pull request Dec 14, 2022
@julienrbrt @mergify
refactor: improve x/upgrade app wiring (part of upgrade audit) (#14216
091ce21 
)

(cherry picked from commit 05a6da3)

# Conflicts:
#	x/upgrade/abci_test.go
@mergify mergify bot mentioned this pull request Dec 14, 2022
Merged
julienrbrt added a commit that referenced this pull request Dec 15, 2022
@mergify @julienrbrt
refactor: improve x/upgrade app wiring (part of upgrade audit) (bac…
e1f73a2 
…kport #14216) (#14306)

Co-authored-by: Julien Robert <julien@rbrt.fr>
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@julienrbrt
refactor: improve x/upgrade app wiring (part of upgrade audit) (cos…
4df32b1 
…mos#14216)
@0xstepit 0xstepit mentioned this pull request Jul 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amaury1093 amaury1093 amaury1093 left review comments

@aaronc aaronc aaronc approved these changes

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

Assignees

@aaronc aaronc

@amaury1093 amaury1093

@likhita-809 likhita-809

Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release C:x/upgrade
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Audit x/upgrade v0.46..v0.47
5 participants
@julienrbrt @aaronc @amaury1093 @alexanderbez @likhita-809