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 Tendermint client state to proto #6932

Merged
merged 7 commits into from
Aug 4, 2020

Conversation

colin-axner
Copy link
Contributor

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

Description

First step in 02-client migration. Migrates tendermint client state to proto, but leaves 02-client keeper using amino for the codec. This will be done in a followup pr to keep a single pr from becoming too large.

uses lots of work from #6254 (thanks @fedekunze !)

copy/pasted diffs from #6926 because I didn't want to wait for merge to start work


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

@@ -63,7 +63,8 @@ func (ev Evidence) String() string {

// Hash implements Evidence interface
func (ev Evidence) Hash() tmbytes.HexBytes {
bz := SubModuleCdc.MustMarshalBinaryBare(ev)
// TODO use submodule cdc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using SubModuleCdc here was throwing a proto codec error. Given that the direct followup pr will fix this I thought this is fine for now

Copy link
Member

Choose a reason for hiding this comment

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

Why is this erroring? Because Evidence hasn't transitioned to proto yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds right, I didn't want to spend the time investigating the issue since it wasn't pressing for this pr. I probably need to add the RegisterInterfaces call from x/evidence

@@ -51,7 +50,7 @@ func (msg *MsgCreateClient) ProtoMessage() {}

// NewMsgCreateClient creates a new MsgCreateClient instance
func NewMsgCreateClient(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't migrate msgs until the header is migrated

@colin-axner colin-axner marked this pull request as ready for review August 4, 2020 14:14
@fedekunze fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 4, 2020
@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #6932 into master will increase coverage by 5.44%.
The diff coverage is 56.41%.

@@            Coverage Diff             @@
##           master    #6932      +/-   ##
==========================================
+ Coverage   55.60%   61.04%   +5.44%     
==========================================
  Files         457      378      -79     
  Lines       27440    24603    -2837     
==========================================
- Hits        15257    15018     -239     
+ Misses      11083     8419    -2664     
- Partials     1100     1166      +66     

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Looks good!! Just a couple questions for my own understanding

@@ -63,7 +63,8 @@ func (ev Evidence) String() string {

// Hash implements Evidence interface
func (ev Evidence) Hash() tmbytes.HexBytes {
bz := SubModuleCdc.MustMarshalBinaryBare(ev)
// TODO use submodule cdc
Copy link
Member

Choose a reason for hiding this comment

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

Why is this erroring? Because Evidence hasn't transitioned to proto yet?


// RegisterInterfaces registers the tendermint concrete evidence and client-related
// implementations and interfaces.
func RegisterInterfaces(registry cdctypes.InterfaceRegistry) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this function get called anywhere yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I forgot to add the call to x/ibc/types/codec.go which calls it from the module manager callback. Nevertheless this just needs to be added in the followup pr anyways

@mergify mergify bot merged commit 79cee06 into master Aug 4, 2020
@mergify mergify bot deleted the colin/tm-client-state-migration branch August 4, 2020 16:31
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.

3 participants