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

Migration logic for 09-localhost removal #1201

Closed
5 tasks
seantking opened this issue Mar 30, 2022 · 2 comments
Closed
5 tasks

Migration logic for 09-localhost removal #1201

seantking opened this issue Mar 30, 2022 · 2 comments
Assignees
Labels
02-client 09-localhost needs discussion Issues that need discussion before they can be worked on

Comments

@seantking
Copy link
Contributor

Summary

As part of the removal of 09-localhost light client implementation, we need to add migration logic in order to delete any localhost existing in the client store.

The following should be done in two separate PRs:

  • Genesis migration logic
  • Upgrade handler migration logic

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@seantking seantking added this to ibc-go Mar 30, 2022
@crodriguezvega crodriguezvega moved this to Todo in ibc-go Mar 30, 2022
@crodriguezvega crodriguezvega added this to the 02-client refactor milestone Mar 30, 2022
@crodriguezvega crodriguezvega moved this from Todo to Backlog in ibc-go May 1, 2022
@colin-axner colin-axner moved this from Backlog to Todo in ibc-go May 18, 2022
@colin-axner
Copy link
Contributor

The migration logic should iterate through all client states and check if the clientID matches "localhost". The idea is to remove any existing localhost client (if it was created). Note there can only be one localhost client, so the could may break early if it is found and deleted.

Upgrade handler migation

The consensus version needs to be incremented to indicate that upgrade migrations should be ran. Register the migration. Add the migrator. In 02-client as well. Write the migration code. Iterate over the clients, when localhost is found, delete the client/consensus state for it. Please write good tests.

Genesis migration

I question whether we need genesis migrations, since there is only 1 localhost. The chain doing a dump state/restart could manually remove the localhost and its associated consensus states (this is assuming a chain ever enabled a localhost client that doesn't function).

If we do decide to support genesis migrations. They should be done in a similar way to v1 migrations.

Here is the entry point (note, I'd name the directory v5 (or whatever migration this is for) instead of v500. There should be a corresponding function called in 02-client. This should iterate over all the clients and look for a client with the name exported.Localhost. Note, we will need the old localhost proto files generated into this folder and it needs to implement the ClientState interface, in the same way I did for the solomachine. Once you find the localhost, delete it from the genesis and delete all of its consensus states. You'll need to find the consensus states as well

Good tests should be written. I ran into many subtle bugs writing the previous migration code for v1

@seantking seantking added the needs discussion Issues that need discussion before they can be worked on label Jun 27, 2022
@seantking
Copy link
Contributor Author

Flagged with needs discussion during planning session. It seems unlikely that anyone has ever used the 09-localhost so if it's not necessary to write the migration it might be best to not waste developer time.

@seantking seantking moved this from Todo to On hold in ibc-go Jun 27, 2022
@damiannolan damiannolan moved this from On hold to In progress in ibc-go Jul 6, 2022
@damiannolan damiannolan moved this from In progress to In review in ibc-go Jul 7, 2022
Repository owner moved this from In review to Done in ibc-go Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02-client 09-localhost needs discussion Issues that need discussion before they can be worked on
Projects
Archived in project
Development

No branches or pull requests

4 participants