fix(evmutil): register MsgConvertCosmosCoinToERC20 on amino #1599
Conversation
also use new register method that checks length to catch ledger issues
| @@ -63,6 +69,64 @@ func TestExport(t *testing.T) { | |||
| assert.Len(t, exportedApp.Validators, 1) // no validators set in default genesis | |||
| } | |||
|
|
|||
| // TestLegacyMsgAreAminoRegistered checks if all known msg types are registered on the app's amino codec. | |||
| // It doesn't check if they are registered on the module codecs used for signature checking. | |||
There was a problem hiding this comment.
is refers to the ModuleCdc right?
There was a problem hiding this comment.
yeah, I was thinking we could add tests to the modules to cover them. But if we replace them with a global codec as they've done in v0.46 (cosmos/cosmos-sdk#11240) we could run this test on that global codec.
| } | ||
|
|
||
| // Check the msg is registered in amino by checking a repeat registration call panics. | ||
| panicValue, ok := catchPanic(func() { |
There was a problem hiding this comment.
super clever way to check the messages are registered 👍 🥇
nit: you can probably use require.PanicsWithValue (or PanicsWithError) to test this.
There was a problem hiding this comment.
thanks, I started with PanicsWithValue/Error but the panic returns an error msg containing the go type name of the msg (eg "types.MsgPlaceBid"). I couldn't think of a way to reconstruct it to do the comparison.
|
|
||
| for i, msgName := range msgProtoNames { | ||
| // skip external unregistered msgs | ||
| if msgName == sdk.MsgTypeURL(&evmtypes.MsgEthereumTx{}) || |
There was a problem hiding this comment.
i'd love if this was a list called unregisteredMsgs or something similar
There was a problem hiding this comment.
agreed, it's a bit confusing, changed to protoRegisteredMsgs as at this point it's unclear if these msgs are all registered on amino or not
Description
Noticed the new
MsgConvertCosmosCoinToERC20wasn't registered on amino. So added a test to help catch this in the future by checking all msgs registered on the proto codec are on amino. It's not perfect, we should ideally checkmsg.GetSignBytesreturns properly encoded msgs (it uses a local amino, not the app's amino). But it did catch a pre-existing issue where we hadn't setup the registration inmodule.go.Also added this to the list of gotchas with link to explainer. It's unlikely to cause issues but can in very particular cases mess with tx signing.
Checklist