x/ibc: amino json support #8422
Conversation
| suite.Require().NoError(err) | ||
| expString = fmt.Sprintf( | ||
| `{"type":"cosmos-sdk/MsgSubmitMisbehaviour","value":{"client_id":"clientid","client_state":,"signer":"%s"}}`, | ||
| // string(sdk.MustSortJSON(types.ModuleCdc.LegacyAmino.MustMarshalJSON(misbehaviour))), // FIXME: header panics on amino json marshal |
There was a problem hiding this comment.
the misbehaviour headers JSON marshaling is panicking:
panic: Unregistered interface crypto.isPublicKey_Sum [recovered]
panic: Unregistered interface crypto.isPublicKey_SumThere was a problem hiding this comment.
@robert-zaremba @clevinson @amaurymartiny @aaronc any hints?
There was a problem hiding this comment.
you will need to register that interface here https://github.com/cosmos/cosmos-sdk/blob/master/crypto/codec/amino.go. Not sure how other modules do this since it's not registered already.
There was a problem hiding this comment.
ah I think it's actually the Tendermint pubkey interface:
// PublicKey defines the keys available for use with Tendermint Validators
type PublicKey struct {
// Types that are valid to be assigned to Sum:
// *PublicKey_Ed25519
// *PublicKey_Secp256K1
Sum isPublicKey_Sum `protobuf_oneof:"sum"`
}There was a problem hiding this comment.
oh, I don't fully understand this code, I can take a look in the morning.
The sdk handles tendermint keys as its own, comes in from abci and gets converted before any usage. This should be done here as well to avoid the dependency on tendermint. I remember doing some work a while ago in the sdk to help with this.
There was a problem hiding this comment.
yeah so the ibc tendermint header imports some types that use the validator proto msgs (which contain the pubkey). Maybe you can coordinate tomorrow morning with @colin-axner?
There was a problem hiding this comment.
Relaying from this comment from @ebuchman cosmos/gaia#568 (comment), the general consensus seems to be to limit this PR to only include amino support for IBC MsgTransfer, and no other IBC messages.
There was a problem hiding this comment.
I will prune back this pr to just do the change for MsgTransfer (I might just open a new pr since that might be easiest). How can we test MsgTransfer with a ledger to ensure that everything does actually work fine? Maybe someone can point me to some tools to use?
|
Adding the Stargate backport label here, as this PR should be cherry-picked to the |
|
superseded by #8437 |
|
Thanks @colin-axner |
closes #8266