Skip to content

Commit

Permalink
refactor!: allow for serialization of proto message without fulfillme…
Browse files Browse the repository at this point in the history
…nt of sdk.Msg interface (#2607) (#2626)

## Description

ref: #2397

---

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.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md).
- [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit 5260c77)

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
  • Loading branch information
3 people authored Oct 31, 2022
1 parent 4b0b5af commit ed4e336
Show file tree
Hide file tree
Showing 12 changed files with 94 additions and 53 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking

* (apps/27-interchain-accounts) [\#2607](https://github.com/cosmos/ibc-go/pull/2607) `SerializeCosmosTx` now takes in a `[]proto.Message` instead of `[]sdk.Msg`.
* (apps/transfer) [\#2446](https://github.com/cosmos/ibc-go/pull/2446) Remove `SendTransfer` function in favor of a private `sendTransfer` function. All IBC transfers must be initiated with `MsgTransfer`.
* (apps/29-fee) [\#2395](https://github.com/cosmos/ibc-go/pull/2395) Remove param space from ics29 NewKeeper function. The field was unused.
* (apps/27-interchain-accounts) [\#2133](https://github.com/cosmos/ibc-go/pull/2133) Generates genesis protos in a separate directory to avoid circular import errors. The protobuf package name has changed for the genesis types.
Expand Down
2 changes: 1 addition & 1 deletion docs/apps/interchain-accounts/auth-modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ if !found {
// The appropriate serialization function should be called. The host chain must be able to deserialize the transaction.
// If the host chain is using the ibc-go host module, `SerializeCosmosTx` should be used.
msg := &banktypes.MsgSend{FromAddress: fromAddr, ToAddress: toAddr, Amount: amt}
data, err := icatypes.SerializeCosmosTx(keeper.cdc, []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(keeper.cdc, []proto.Message{msg})
if err != nil {
return err
}
Expand Down
2 changes: 2 additions & 0 deletions docs/migrations/v5-to-v6.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ func DefaultParams() Params {

#### API breaking changes

`SerializeCosmosTx` takes in a `[]proto.Message` instead of `[]sdk.Message`. This allows for the serialization of proto messages without requiring the fulfillment of the `sdk.Msg` interface.

The `27-interchain-accounts` genesis types have been moved to their own package: `modules/apps/27-interchain-acccounts/genesis/types`.
This change facilitates the addition of the ICS27 controller submodule `MsgServer` and avoids cyclic imports. This should have minimal disruption to chain developers integrating `27-interchain-accounts`.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdktypes "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
"github.com/gogo/protobuf/proto"

"github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/controller/keeper"
"github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/controller/types"
Expand Down Expand Up @@ -169,7 +170,7 @@ func (suite *KeeperTestSuite) TestSubmitTx() {
Amount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.Codec, []sdk.Msg{icaMsg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.Codec, []proto.Message{icaMsg})
suite.Require().NoError(err)

packetData := icatypes.InterchainAccountPacketData{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper_test
import (
sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
"github.com/gogo/protobuf/proto"

icatypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/types"
clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
Expand Down Expand Up @@ -34,7 +35,7 @@ func (suite *KeeperTestSuite) TestSendTx() {
Amount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
}

data, err := icatypes.SerializeCosmosTx(suite.chainB.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainB.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

packetData = icatypes.InterchainAccountPacketData{
Expand All @@ -50,7 +51,7 @@ func (suite *KeeperTestSuite) TestSendTx() {
interchainAccountAddr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.Require().True(found)

msgsBankSend := []sdk.Msg{
msgsBankSend := []proto.Message{
&banktypes.MsgSend{
FromAddress: interchainAccountAddr,
ToAddress: suite.chainB.SenderAccount.GetAddress().String(),
Expand Down Expand Up @@ -121,7 +122,7 @@ func (suite *KeeperTestSuite) TestSendTx() {
Amount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
}

data, err := icatypes.SerializeCosmosTx(suite.chainB.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainB.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

packetData = icatypes.InterchainAccountPacketData{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
"github.com/gogo/protobuf/proto"
"github.com/stretchr/testify/require"

"github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/controller/types"
Expand Down Expand Up @@ -155,7 +156,7 @@ func TestMsgSendTxValidateBasic(t *testing.T) {
Amount: ibctesting.TestCoins,
}

data, err := icatypes.SerializeCosmosTx(simapp.MakeTestEncodingConfig().Marshaler, []sdk.Msg{msgBankSend})
data, err := icatypes.SerializeCosmosTx(simapp.MakeTestEncodingConfig().Marshaler, []proto.Message{msgBankSend})
require.NoError(t, err)

packetData := icatypes.InterchainAccountPacketData{
Expand Down Expand Up @@ -191,7 +192,7 @@ func TestMsgSendTxGetSigners(t *testing.T) {
Amount: ibctesting.TestCoins,
}

data, err := icatypes.SerializeCosmosTx(simapp.MakeTestEncodingConfig().Marshaler, []sdk.Msg{msgBankSend})
data, err := icatypes.SerializeCosmosTx(simapp.MakeTestEncodingConfig().Marshaler, []proto.Message{msgBankSend})
require.NoError(t, err)

packetData := icatypes.InterchainAccountPacketData{
Expand Down
17 changes: 9 additions & 8 deletions modules/apps/27-interchain-accounts/host/client/cli/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/version"
"github.com/gogo/protobuf/proto"
"github.com/spf13/cobra"

icatypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/types"
Expand Down Expand Up @@ -90,17 +91,17 @@ which submits pre-built packet data containing messages to be executed on the ho
// generatePacketData takes in message bytes and a memo and serializes the message into an
// instance of InterchainAccountPacketData which is returned as bytes.
func generatePacketData(cdc *codec.ProtoCodec, msgBytes []byte, memo string) ([]byte, error) {
sdkMessages, err := convertBytesIntoSdkMessages(cdc, msgBytes)
protoMessages, err := convertBytesIntoProtoMessages(cdc, msgBytes)
if err != nil {
return nil, err
}

return generateIcaPacketDataFromSdkMessages(cdc, sdkMessages, memo)
return generateIcaPacketDataFromProtoMessages(cdc, protoMessages, memo)
}

// convertBytesIntoSdkMessages returns a list of sdk messages from bytes. The bytes can be in the form of a single
// convertBytesIntoProtoMessages returns a list of proto messages from bytes. The bytes can be in the form of a single
// message, or a json array of messages.
func convertBytesIntoSdkMessages(cdc *codec.ProtoCodec, msgBytes []byte) ([]sdk.Msg, error) {
func convertBytesIntoProtoMessages(cdc *codec.ProtoCodec, msgBytes []byte) ([]proto.Message, error) {
var rawMessages []json.RawMessage
if err := json.Unmarshal(msgBytes, &rawMessages); err != nil {
// if we fail to unmarshal a list of messages, we assume we are just dealing with a single message.
Expand All @@ -110,10 +111,10 @@ func convertBytesIntoSdkMessages(cdc *codec.ProtoCodec, msgBytes []byte) ([]sdk.
return nil, err
}

return []sdk.Msg{msg}, nil
return []proto.Message{msg}, nil
}

sdkMessages := make([]sdk.Msg, len(rawMessages))
sdkMessages := make([]proto.Message, len(rawMessages))
for i, anyJSON := range rawMessages {
var msg sdk.Msg
if err := cdc.UnmarshalInterfaceJSON(anyJSON, &msg); err != nil {
Expand All @@ -126,8 +127,8 @@ func convertBytesIntoSdkMessages(cdc *codec.ProtoCodec, msgBytes []byte) ([]sdk.
return sdkMessages, nil
}

// generateIcaPacketDataFromSdkMessages generates ica packet data as bytes from a given set of sdk messages and a memo.
func generateIcaPacketDataFromSdkMessages(cdc *codec.ProtoCodec, sdkMessages []sdk.Msg, memo string) ([]byte, error) {
// generateIcaPacketDataFromProtoMessages generates ica packet data as bytes from a given set of proto encoded sdk messages and a memo.
func generateIcaPacketDataFromProtoMessages(cdc *codec.ProtoCodec, sdkMessages []proto.Message, memo string) ([]byte, error) {
icaPacketDataBytes, err := icatypes.SerializeCosmosTx(cdc, sdkMessages)
if err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
ToAddress: suite.chainB.SenderAccount.GetAddress().String(),
Amount: amount,
}
data, err := icatypes.SerializeCosmosTx(suite.chainA.Codec, []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.Codec, []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand Down Expand Up @@ -655,7 +655,7 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose()
Amount: tokenAmt,
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand Down
47 changes: 34 additions & 13 deletions modules/apps/27-interchain-accounts/host/keeper/relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/gogo/protobuf/proto"

"github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/host/types"
icatypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/types"
Expand Down Expand Up @@ -55,7 +56,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Option: govtypes.OptionYes,
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand All @@ -82,7 +83,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Amount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand Down Expand Up @@ -110,7 +111,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Amount: sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(5000)),
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand Down Expand Up @@ -144,7 +145,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Amount: sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(5000)),
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msgDelegate, msgUndelegate})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msgDelegate, msgUndelegate})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand Down Expand Up @@ -179,7 +180,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Proposer: interchainAccountAddr,
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand Down Expand Up @@ -221,7 +222,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Option: govtypes.OptionYes,
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand All @@ -247,7 +248,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Depositor: interchainAccountAddr,
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand All @@ -273,7 +274,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
WithdrawAddress: suite.chainB.SenderAccount.GetAddress().String(),
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand Down Expand Up @@ -312,7 +313,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
TimeoutTimestamp: uint64(0),
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand All @@ -327,6 +328,26 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
},
true,
},
{
"unregistered sdk.Msg",
func() {
msg := &banktypes.MsgSendResponse{}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Type: icatypes.EXECUTE_TX,
Data: data,
}

packetData = icaPacketData.GetBytes()

params := types.NewParams(true, []string{"/" + proto.MessageName(msg)})
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
},
false,
},
{
"cannot unmarshal interchain account packet data",
func() {
Expand All @@ -351,7 +372,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
{
"invalid packet type - UNSPECIFIED",
func() {
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{&banktypes.MsgSend{}})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{&banktypes.MsgSend{}})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand All @@ -368,7 +389,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
func() {
path.EndpointA.ChannelConfig.PortID = "invalid-port-id"

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{&banktypes.MsgSend{}})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{&banktypes.MsgSend{}})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand All @@ -389,7 +410,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Amount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand All @@ -410,7 +431,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
Amount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))),
}

data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []sdk.Msg{msg})
data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg})
suite.Require().NoError(err)

icaPacketData := icatypes.InterchainAccountPacketData{
Expand Down
3 changes: 2 additions & 1 deletion modules/apps/27-interchain-accounts/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/gogo/protobuf/proto"
)

// ModuleCdc references the global interchain accounts module codec. Note, the codec
Expand All @@ -25,7 +26,7 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
// SerializeCosmosTx serializes a slice of sdk.Msg's using the CosmosTx type. The sdk.Msg's are
// packed into Any's and inserted into the Messages field of a CosmosTx. The proto marshaled CosmosTx
// bytes are returned. Only the ProtoCodec is supported for serializing messages.
func SerializeCosmosTx(cdc codec.BinaryCodec, msgs []sdk.Msg) (bz []byte, err error) {
func SerializeCosmosTx(cdc codec.BinaryCodec, msgs []proto.Message) (bz []byte, err error) {
// only ProtoCodec is supported
if _, ok := cdc.(*codec.ProtoCodec); !ok {
return nil, sdkerrors.Wrap(ErrInvalidCodec, "only ProtoCodec is supported for receiving messages on the host chain")
Expand Down
Loading

0 comments on commit ed4e336

Please sign in to comment.