x/ibc migrate genesis proto

[atheeshp]

ref: #5917

closes: #XXXX

---

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

[codecov]

Codecov Report

Merging #6878 into master will increase coverage by 0.15%.
The diff coverage is 45.79%.

@@            Coverage Diff             @@
##           master    #6878      +/-   ##
==========================================
+ Coverage   61.86%   62.01%   +0.15%     
==========================================
  Files         421      519      +98     
  Lines       25498    32060    +6562     
==========================================
+ Hits        15775    19883    +4108     
- Misses       8371    10559    +2188     
- Partials     1352     1618     +266     

[fedekunze]

Thanks @atheeshp! @colin-axner and I can take it from here. We are working on the final migrations to switch ibc to be full-proto compatible

[atheeshp]

Thanks @atheeshp! @colin-axner and I can take it from here. We are working on the final migrations to switch ibc to be full-proto compatible

Sure, go on.

[fedekunze]

tested ACK

[colin-axner]

🎉

[colin-axner]

sims on import/export are still failing for me locally. Is there something we need to do to flip the switch to proto from amino?

[sahith-narahari]

sims on import/export are still failing for me locally. Is there something we need to do to flip the switch to proto from amino?

@colin-axner I'm taking a look now, will let you know if I can find what's causing this

[sahith-narahari]

@colin-axner @fedekunze I see ClientState is not a proto message, is there a reason for not migrating these types.

cosmos-sdk/x/ibc/07-tendermint/types/client_state.go

Line 28 in 5676e72

type ClientState struct {

cosmos-sdk/x/ibc/09-localhost/types/client_state.go

Line 27 in 5676e72

type ClientState struct {

[fedekunze]

We need to remove them. They have already been migrated to protobuf

@sahith-narahari I see that your diff if outdated? Those types are already migrated on this branch

[sahith-narahari]

Oh yes, you're right. Sorry, my bad

[colin-axner]

we are going to automerge this since the same sims failing are still failing on master. We will open up a followup pr to try to fix the sims (since the changes should be outside the scope of this pr), which I suspect requires using proto instead of amino, unless we should still be able to be amino compatible while using Any in our keeper? From what I can tell debugging the issue seems to be that amino encoding doesn't decode our types into a pointer which causes marshaling in our keeper to fail

[colin-axner]

so specifically the issue here is:

we want to marshal/unmarshal *types.ClientState but with the hybrid codec it unmarshal it to types.ClientState which causes an encoding error. With the proto codec this works fine as our unit test proves, but I can't switch the sims to encode/decode via proto only because:

cannot protobuf JSON encode unsupported type: simapp.GenesisState since GenesisState isn't defined as a proto message.

My understanding between the compatibility of amino and proto is very limited. I have been under the impression, IBC will not support any amino compatibility. Maybe there is still work to be done to switch to proto for genesis? Is there a way to proto define map[string]json.RawMessage? Is there a way IBC can temporarily support amino via codec registration just to pass sims before the switch is made? Is it expected that amino support encoding *codectypes.Any correctly?

Would love help with this, I'm just running in circles atm.

cc/ @sahith-narahari