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

fix(evmutil): register MsgConvertCosmosCoinToERC20 on amino #1599

Merged
merged 4 commits into from
May 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions app/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,17 @@ package app

import (
"encoding/json"
"fmt"
"os"
"sort"
"testing"
"time"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
vestingtypes "github.com/cosmos/cosmos-sdk/x/auth/vesting/types"
evmtypes "github.com/evmos/ethermint/x/evm/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
abci "github.com/tendermint/tendermint/abci/types"
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

is refers to the ModuleCdc right?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

func TestLegacyMsgAreAminoRegistered(t *testing.T) {
tApp := NewTestApp()

lcdc := tApp.LegacyAmino()

// Use the proto codec as the canonical list of msg types.
protoCodec := tApp.AppCodec().(*codec.ProtoCodec)
protoRegisteredMsgs := protoCodec.InterfaceRegistry().ListImplementations(sdk.MsgInterfaceProtoName)

for i, msgName := range protoRegisteredMsgs {
// Skip msgs from dependencies that were never amino registered.
if msgName == sdk.MsgTypeURL(&evmtypes.MsgEthereumTx{}) ||
Copy link
Member

Choose a reason for hiding this comment

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

i'd love if this was a list called unregisteredMsgs or something similar

Copy link
Member Author

Choose a reason for hiding this comment

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

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

msgName == sdk.MsgTypeURL(&vestingtypes.MsgCreatePeriodicVestingAccount{}) {
continue
}

// Create an encoded json msg, then unmarshal it to instantiate the msg type.
jsonMsg := []byte(fmt.Sprintf(`{"@type": "%s"}`, msgName))

var msg sdk.Msg
err := protoCodec.UnmarshalInterfaceJSON(jsonMsg, &msg)
require.NoError(t, err)

// Only check legacy msgs for amino registration.
// Only legacy msg can be signed with amino.
_, ok := msg.(legacytx.LegacyMsg)
if !ok {
continue
}

// Check the msg is registered in amino by checking a repeat registration call panics.
panicValue, ok := catchPanic(func() {
Copy link
Member

@pirtleshell pirtleshell May 25, 2023

Choose a reason for hiding this comment

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

super clever way to check the messages are registered 👍 🥇

nit: you can probably use require.PanicsWithValue (or PanicsWithError) to test this.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

lcdc.RegisterConcrete(interface{}(msg), fmt.Sprintf("aUniqueRegistrationName%d", i), nil)
})
assert.True(t, ok, "registration did not panic, msg %s is not registered in amino", msgName)
if ok {
require.IsTypef(t, "", panicValue, "msg %s amino registration panicked with unexpected type", msgName)
aminoErrMsgPrefix := "TypeInfo already exists for"
require.Containsf(t, panicValue, aminoErrMsgPrefix, "msg %s amino registration panicked for unexpected reason", msgName)
}
}
}

// catchPanic returns the panic value of the passed function. The second return indicates if the function panicked.
func catchPanic(f func()) (panicValue interface{}, didPanic bool) {
didPanic = true

defer func() {
panicValue = recover()
}()

f()
didPanic = false
return
}

// unmarshalJSONKeys extracts keys from the top level of a json blob.
func unmarshalJSONKeys(jsonBytes []byte) ([]string, error) {
var jsonMap map[string]json.RawMessage
Expand Down
4 changes: 3 additions & 1 deletion x/evmutil/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ func (AppModuleBasic) Name() string {
}

// Registers legacy amino codec
func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {}
func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
types.RegisterLegacyAminoCodec(cdc)
}

// RegisterInterfaces registers the module's interface types
func (a AppModuleBasic) RegisterInterfaces(reg cdctypes.InterfaceRegistry) {
Expand Down
8 changes: 6 additions & 2 deletions x/evmutil/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
cdctypes "github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -12,14 +13,17 @@ import (
// RegisterLegacyAminoCodec registers the necessary evmutil interfaces and concrete types
// on the provided LegacyAmino codec. These types are used for Amino JSON serialization.
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
cdc.RegisterConcrete(&MsgConvertCoinToERC20{}, "evmutil/MsgConvertCoinToERC20", nil)
cdc.RegisterConcrete(&MsgConvertERC20ToCoin{}, "evmutil/MsgConvertERC20ToCoin", nil)
legacy.RegisterAminoMsg(cdc, &MsgConvertCoinToERC20{}, "evmutil/MsgConvertCoinToERC20")
legacy.RegisterAminoMsg(cdc, &MsgConvertERC20ToCoin{}, "evmutil/MsgConvertERC20ToCoin")
legacy.RegisterAminoMsg(cdc, &MsgConvertCosmosCoinToERC20{}, "evmutil/MsgConvertCosmosCoinToERC20")

}

func RegisterInterfaces(registry cdctypes.InterfaceRegistry) {
registry.RegisterImplementations((*sdk.Msg)(nil),
&MsgConvertCoinToERC20{},
&MsgConvertERC20ToCoin{},
&MsgConvertCosmosCoinToERC20{},
)

msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
Expand Down