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

chore: adding upgrade handler for 09-localhost removal #1671

Merged

Conversation

damiannolan
Copy link
Member

@damiannolan damiannolan commented Jul 7, 2022

Description

  • Adding upgrade handler for pruning of 09-localhost client and consensus states
  • Adding migration docs

closes: #1201


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@damiannolan
Copy link
Member Author

I think this PR also requires some migration docs added to instruct devs to wire up the upgrade handler.
I will add some!

I've added a new directory upgrades/v5 under core. I am open to suggestions on moving this to somewhere else but I think this is a nice pattern to use for upgrade handlers for ibc core logic. Inspired from osmosis here

@damiannolan damiannolan requested a review from seantking as a code owner July 7, 2022 12:51
Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Maybe we could call the folder migrations? Not very strongly opposed to upgrades anyway, but thought that migrations is how the folder is called in docs and we could also have migrations that not necessarily need to run on the upgrade to a major version. Well, just an idea!

docs/migrations/v4-to-v5.md Outdated Show resolved Hide resolved
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Looks great :)

Might be nice to add information to the localhost removal changelog entry that this function can be used to remove existing localhost implementations

docs/migrations/v4-to-v5.md Show resolved Hide resolved
modules/core/upgrades/v5/upgrades.go Outdated Show resolved Hide resolved
@damiannolan
Copy link
Member Author

Maybe we could call the folder migrations? Not very strongly opposed to upgrades anyway, but thought that migrations is how the folder is called in docs and we could also have migrations that not necessarily need to run on the upgrade to a major version. Well, just an idea!

Which approach should we take with naming? I chose upgrades as it was an "upgrade handler" in the context of the sdk that was being added, as opposed to the legacy genesis migration approach but I'm not really fussy. We could rename the docs/migrations directory to docs/upgrades or we go with package migrations in the go code. I guess its best to have some consistency. My personal vote is for package upgrades with a vX sub package, but I'm happy to go with the consensus.

@colin-axner
Copy link
Contributor

Which approach should we take with naming?

I like migrations. Typically this functionality would be run by SDK's RunMigrations function. It isn't only because most chains don't have a localhost thus making the extra computation unnecessary. I also want to avoid confusion with client/connection/channel upgrades

@damiannolan
Copy link
Member Author

Which approach should we take with naming?

I like migrations. Typically this functionality would be run by SDK's RunMigrations function. It isn't only because most chains don't have a localhost thus making the extra computation unnecessary. I also want to avoid confusion with client/connection/channel upgrades

Sounds good, that's a compelling enough argument for me! I'll update this today with the naming changes throughout 👍

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Just a small nit..
Nice work, @damiannolan!

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@charleenfei charleenfei left a comment

Choose a reason for hiding this comment

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

nice :)

const Localhost string = "09-localhost"

// MigrateToV5 prunes the 09-Localhost client and associated consensus states from the ibc store
func MigrateToV5(ctx sdk.Context, clientKeeper clientkeeper.Keeper) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we could potentially rename this function to make it more explicit as to what it does? (something like Migrate_RemoveLocalHostClient)

// ...

app.UpgradeKeeper.SetUpgradeHandler(
upgradeName,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: upgradeName can be MigrateToV5 or MigrateLocalHostClient?

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
@damiannolan damiannolan merged commit 04df7cd into 02-client-refactor Jul 15, 2022
@damiannolan damiannolan deleted the damian/1201-migrations-for-09-localhost branch July 15, 2022 21:23
oshorefueled pushed a commit to ComposableFi/ibc-go that referenced this pull request Aug 9, 2022
* adding localhost migration code with tests

* updating test cases

* renaming test func

* updating migration docs and moving to v4-to-v5.md

* fixing indentation of code snippet

* Update docs/migrations/v4-to-v5.md

* adding sample query check for localhost client to docs

* updating changelog to provide info on localhost upgrade handler

* updating migrations docs with nit

* renaming upgrades to migrations

* updating function namings, tests and docs

* Update CHANGELOG.md

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants