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

Migrate 02-client to use proto encoded/decoded client states #6948

Merged
merged 5 commits into from
Aug 5, 2020

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Aug 5, 2020

Description

Followup to #6932

Migrates client states to use codec.BinaryMarshaler in 02-client. ClientState will now be passed around as a pointer. We need to maintain the amino codec registration until the flip is switched on amino encoding genesis, which will occur once IBC genesis is migrated (after consensus states migration).

Consensus state migration should be able to occur after we change the validator set to be a hash in the tendermint client: #6942. I'll probably do this change in 2 steps like client state, migrate tendermint consensus state, then flip the switch in 02-client

ref: #6254


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

@colin-axner colin-axner changed the title Migrate 02-client to use client states that use proto Migrate 02-client to use proto encoded/decoded client states Aug 5, 2020
@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #6948 into master will increase coverage by 0.01%.
The diff coverage is 72.54%.

@@            Coverage Diff             @@
##           master    #6948      +/-   ##
==========================================
+ Coverage   61.78%   61.80%   +0.01%     
==========================================
  Files         520      521       +1     
  Lines       32147    32163      +16     
==========================================
+ Hits        19861    19877      +16     
+ Misses      10676    10672       -4     
- Partials     1610     1614       +4     

@@ -27,17 +28,10 @@ func RegisterInterfaces(registry cdctypes.InterfaceRegistry) {
}

var (
amino = codec.New()
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 don't need this because 09-localhost does not internal amino encoding, just registration is needed to decode/encode client states at genesis and exporting genesis

@colin-axner colin-axner marked this pull request as ready for review August 5, 2020 13:48
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK

x/ibc/02-client/types/codec.go Show resolved Hide resolved
@fedekunze fedekunze added A:automerge Automatically merge PR once all prerequisites pass. C:Encoding x/ibc labels Aug 5, 2020
@fedekunze fedekunze merged commit 1a531cb into master Aug 5, 2020
@fedekunze fedekunze deleted the colin/migrate-client-state branch August 5, 2020 15:14
@colin-axner
Copy link
Contributor Author

colin-axner commented Aug 5, 2020

@fedekunze test-sim-after-import and test-sim-import-export failed, but I can't tell why. Should we revert? I expect it'd be fixed after full migration, but I don't know how long until full migration is done. Maybe it is just a genesis issue?

@colin-axner
Copy link
Contributor Author

colin-axner commented Aug 5, 2020

we identified the fix, non-empty genesis states weren't being tested in regular tests. I forgot to update the genesis client states to be pointers. Will post a fix with tests (probably won't get it done until tomorrow

@colin-axner colin-axner mentioned this pull request Aug 5, 2020
9 tasks
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. C:Encoding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants