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

feat: Add x/gov v043->v046 migrations #11036

Merged
merged 15 commits into from
Feb 3, 2022
Merged

feat: Add x/gov v043->v046 migrations #11036

merged 15 commits into from
Feb 3, 2022

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Jan 27, 2022 •
edited
Loading

Description

Closes: #10869

depends on:

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)

@cmwaters
Copy link
Contributor

Hey @AmauryM, I had to add some conversion functions as part of #11029. I think they mostly go in the other direction i.e. from v1beta2 -> v1beta1 but might be good to sync up so we don't have too many merge conflicts

@amaury1093
Copy link
Contributor Author

amaury1093 commented Jan 28, 2022 •
edited
Loading

Ah yeah, thanks, I just saw that. Yeah, you put in a migrate.go and I put in convert.go, so I think merge conflicts should be fine. I think your PR will be merged first, after that I'll put them all in one file and standardize the naming.

@github-actions github-actions bot added C:CLI C:x/genutil genutil module issues labels Jan 31, 2022
@amaury1093 amaury1093 marked this pull request as ready for review February 2, 2022 10:22
cmwaters
cmwaters approved these changes Feb 2, 2022
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM

x/gov/keeper/migrations.go
@@ -19,3 +20,8 @@ func NewMigrator(keeper Keeper) Migrator {
func (m Migrator) Migrate1to2(ctx sdk.Context) error {
return v043.MigrateStore(ctx, m.keeper.storeKey, m.keeper.cdc)
}

// Migrate2to3 migrates from version 2 to 3.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use 1, 2, and 3 version numbers - do they relate to anything. Might it be more intuitive to specify SDK version migrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided in adr-041 to use uint here. I guess the idea back then was to prepare when modules will have their own go.mods. It might be that this ConsensusVersion == the major version in go.mod. Or not. Anyways, I think it's better for modules to have independent consensus versions than be tied up to SDK versions.

(note the v04* migration folders could probably be renamed though).

x/gov/migrations/v046/convert.go Outdated Show resolved Hide resolved
x/gov/module.go
@@ -188,7 +192,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw
}

// ConsensusVersion implements AppModule/ConsensusVersion.
func (AppModule) ConsensusVersion() uint64 { return 2 }
func (AppModule) ConsensusVersion() uint64 { return 3 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see this is where the versioning numbers come from

atheeshp
atheeshp approved these changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@atheeshp atheeshp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm, except the failing tests.

x/gov/migrations/v046/convert.go Outdated Show resolved Hide resolved
@blushi
Copy link
Contributor

blushi commented Feb 3, 2022

lgtm, except the failing tests.

sim tests are failing on master

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 3, 2022
@mergify mergify bot merged commit dbc19b3 into master Feb 3, 2022
@mergify mergify bot deleted the am/10869-migrations branch February 3, 2022 10:52
@glnro glnro mentioned this pull request Apr 14, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atheeshp atheeshp atheeshp approved these changes

@cmwaters cmwaters cmwaters approved these changes

@aaronc aaronc Awaiting requested review from aaronc

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@likhita-809 likhita-809 Awaiting requested review from likhita-809

@blushi blushi Awaiting requested review from blushi

@aleem1314 aleem1314 Awaiting requested review from aleem1314

Assignees
No one assigned
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:x/genutil genutil module issues C:x/gov
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gov: add migration functionality from v1beta1 to v1beta2
4 participants
@amaury1093 @cmwaters @blushi @atheeshp