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.

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: always move auth module migration to the end #10608

Merged
merged 6 commits into from Nov 25, 2021

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Nov 24, 2021

Description

This PR targets only v0.44.x release. For master we will have #10604

Closes: #10606
Fixes: #10591


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)

@@ -405,10 +405,19 @@ func (m Manager) RunMigrations(ctx sdk.Context, cfg Configurator, fromVM Version
// and the order of executing migrations matters)
// TODO: make the order user-configurable?
sortedModNames := make([]string, 0, len(m.Modules))
hasAuth := false
const authModulename = "auth" // using authtypes.ModuleName causes import cycle.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it smells, but moving it to refactoring this packages is no go for merging it in 0.44.x

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a TODO for future scope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO doesn't make sense here because this is merged only for 0.44.x, not in master.

@robert-zaremba robert-zaremba changed the base branch from master to release/v0.44.x November 24, 2021 23:17
@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Nov 24, 2021

cc: @RiccardoM , @iramiller

@iramiller
Copy link
Contributor

Are we certain that auth is the only module of the ordered set that matters? As in no external chain modules will have this same issue?

It seems like this approach will still work with our workaround that sent in each module one at a time out of an array to control the order.

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just needs changelog entry.

Issues for other chains migrations will still persist, but perhaps thats out of scope for this PR. Documentation for this as an issue should propogate to the upgrade instructions wherever they are.

@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Nov 25, 2021

Documentation for this as an issue should propogate to the upgrade instructions wherever they are.
@ValarDragon right. Also, to be clear this is only a temporal solution. In 0.45 we will have something else.

Issues for other chains migrations will still persist,

Yes, if the chains will have other dependencies, then they will need to use a hack as resented by Osmosis / Dev: https://github.com/cosmos/gaia/blob/0dd70958a10b24703c09c43745611779f0e7970f/app/app.go#L572-L593

Are we certain that auth is the only module of the ordered set that matters? As in no external chain modules will have this same issue?

No, I will update the documentation and migration guide. Proper change is breaking or requires a hack described above.

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.

lgtm, pending Changelog

@@ -405,10 +405,19 @@ func (m Manager) RunMigrations(ctx sdk.Context, cfg Configurator, fromVM Version
// and the order of executing migrations matters)
// TODO: make the order user-configurable?
sortedModNames := make([]string, 0, len(m.Modules))
hasAuth := false
const authModulename = "auth" // using authtypes.ModuleName causes import cycle.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a TODO for future scope?

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.

some docs suggestions

CHANGELOG.md Outdated Show resolved Hide resolved
docs/core/upgrade.md Outdated Show resolved Hide resolved
docs/core/upgrade.md Show resolved Hide resolved
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.

docs lgtm too.

docs/core/upgrade.md Show resolved Hide resolved
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Nov 25, 2021
@robert-zaremba robert-zaremba merged commit ecab606 into release/v0.44.x Nov 25, 2021
@robert-zaremba robert-zaremba deleted the robert/fix-migration-order branch November 25, 2021 11:17
@ValarDragon
Copy link
Contributor

ValarDragon commented Nov 25, 2021

This is going to make syncing nodes for already upgraded chains much more complicated if they upgrade

@robert-zaremba
Copy link
Collaborator Author

Good point. We didn't thought much about syncing from genesis. We were following the decision / discussion re 0.44.1 release here.
It seams we need to do more major release :)

Eengineer1 pushed a commit to cheqd/cosmos-sdk that referenced this pull request Aug 26, 2022
* fix: always move auth module migration to the end

* update migration docs

* adding changelog

* update docs

* review updates

* Update CHANGELOG.md

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In place migration: auth must be migrated after staking Upgrading to v0.44 breaks vested accounts
5 participants