From b036bfcab8178e496e9e9c6568b43604190ae1a4 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 20 Jul 2023 23:32:43 +0300 Subject: [PATCH 1/5] Add failure case for msg failing on ValidateBasic. --- .../host/keeper/relay_test.go | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go index 220a88073a6..83b77e0111d 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go @@ -330,6 +330,40 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { }, true, }, + { + "Msg fails its ValidateBasic: MsgTransfer has an empty receiver", + func(encoding string) { + transferPath := ibctesting.NewTransferPath(suite.chainB, suite.chainC) + suite.coordinator.Setup(transferPath) + + interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID) + suite.Require().True(found) + + msg := &transfertypes.MsgTransfer{ + SourcePort: transferPath.EndpointA.ChannelConfig.PortID, + SourceChannel: transferPath.EndpointA.ChannelID, + Token: sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100)), + Sender: interchainAccountAddr, + Receiver: "", + TimeoutHeight: suite.chainB.GetTimeoutHeight(), + TimeoutTimestamp: uint64(0), + } + + data, err := icatypes.SerializeCosmosTx(suite.chainA.GetSimApp().AppCodec(), []proto.Message{msg}, encoding) + suite.Require().NoError(err) + + icaPacketData := icatypes.InterchainAccountPacketData{ + Type: icatypes.EXECUTE_TX, + Data: data, + } + + packetData = icaPacketData.GetBytes() + + params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) + suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) + }, + false, + }, { "unregistered sdk.Msg", func(encoding string) { From eec8fe2637c9ceeee9f88edd4de9e23a48e8c8c7 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 2 Aug 2023 12:33:17 +0300 Subject: [PATCH 2/5] Move testing for host relay to check for error returns. --- .../host/keeper/relay_test.go | 67 +++++++++---------- .../27-interchain-accounts/types/codec.go | 2 +- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go index 83b77e0111d..0020a7ab635 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go @@ -20,6 +20,7 @@ import ( icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types" transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" + ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors" ibctesting "github.com/cosmos/ibc-go/v7/testing" ) @@ -33,7 +34,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { testCases := []struct { msg string malleate func(encoding string) - expPass bool + expErr error }{ { "interchain account successfully executes an arbitrary message type using the * (allow all message types) param", @@ -75,7 +76,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{"*"}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes banktypes.MsgSend", @@ -102,7 +103,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes stakingtypes.MsgDelegate", @@ -130,7 +131,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes stakingtypes.MsgDelegate and stakingtypes.MsgUndelegate sequentially", @@ -164,7 +165,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msgDelegate), sdk.MsgTypeURL(msgUndelegate)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes govtypes.MsgSubmitProposal", @@ -199,7 +200,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes govtypes.MsgVote", @@ -241,7 +242,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes disttypes.MsgFundCommunityPool", @@ -267,7 +268,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes disttypes.MsgSetWithdrawAddress", @@ -293,7 +294,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes transfertypes.MsgTransfer", @@ -328,7 +329,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "Msg fails its ValidateBasic: MsgTransfer has an empty receiver", @@ -362,7 +363,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - false, + ibcerrors.ErrInvalidAddress, }, { "unregistered sdk.Msg", @@ -382,14 +383,14 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{"/" + proto.MessageName(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - false, + icatypes.ErrUnknownDataType, }, { "cannot unmarshal interchain account packet data", func(encoding string) { packetData = []byte{} }, - false, + icatypes.ErrUnknownDataType, }, { "cannot deserialize interchain account packet data messages", @@ -403,7 +404,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { packetData = icaPacketData.GetBytes() }, - false, + icatypes.ErrUnknownDataType, }, { "invalid packet type - UNSPECIFIED", @@ -418,7 +419,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { packetData = icaPacketData.GetBytes() }, - false, + icatypes.ErrUnknownDataType, }, { "unauthorised: interchain account not found for controller port ID", @@ -435,7 +436,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { packetData = icaPacketData.GetBytes() }, - false, + icatypes.ErrInterchainAccountNotFound, }, { "unauthorised: message type not allowed", // NOTE: do not update params to explicitly force the error @@ -456,7 +457,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { packetData = icaPacketData.GetBytes() }, - false, + ibcerrors.ErrUnauthorized, }, { "unauthorised: signer address is not the interchain account associated with the controller portID", @@ -480,7 +481,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - false, + ibcerrors.ErrUnauthorized, }, } @@ -528,11 +529,10 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { txResponse, err := suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) - if tc.expPass { - suite.Require().NoError(err) + suite.Require().ErrorIs(tc.expErr, err) + if tc.expErr == nil { suite.Require().NotNil(txResponse) } else { - suite.Require().Error(err) suite.Require().Nil(txResponse) } }) @@ -550,7 +550,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { testCases := []struct { msg string malleate func(icaAddress string) - expPass bool + expErr error }{ { "interchain account successfully executes an arbitrary message type using the * (allow all message types) param", @@ -592,7 +592,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{"*"}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes banktypes.MsgSend", @@ -617,7 +617,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL((*banktypes.MsgSend)(nil))}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes govtypes.MsgSubmitProposal", @@ -646,7 +646,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL((*govtypes.MsgSubmitProposal)(nil))}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes govtypes.MsgVote", @@ -686,7 +686,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL((*govtypes.MsgVote)(nil))}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes govtypes.MsgSubmitProposal, govtypes.MsgDeposit, and then govtypes.MsgVote sequentially", @@ -727,7 +727,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL((*govtypes.MsgSubmitProposal)(nil)), sdk.MsgTypeURL((*govtypes.MsgDeposit)(nil)), sdk.MsgTypeURL((*govtypes.MsgVote)(nil))}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "interchain account successfully executes transfertypes.MsgTransfer", @@ -760,7 +760,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL((*transfertypes.MsgTransfer)(nil))}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - true, + nil, }, { "unregistered sdk.Msg", @@ -776,7 +776,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{"*"}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - false, + icatypes.ErrUnknownDataType, }, { "message type not allowed banktypes.MsgSend", @@ -801,7 +801,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL((*transfertypes.MsgTransfer)(nil))}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - false, + ibcerrors.ErrUnauthorized, }, { "unauthorised: signer address is not the interchain account associated with the controller portID", @@ -826,7 +826,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL((*banktypes.MsgSend)(nil))}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - false, + ibcerrors.ErrUnauthorized, }, } @@ -866,11 +866,10 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { txResponse, err := suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) - if tc.expPass { - suite.Require().NoError(err) + suite.Require().ErrorIs(tc.expErr, err) + if tc.expErr == nil { suite.Require().NotNil(txResponse) } else { - suite.Require().Error(err) suite.Require().Nil(txResponse) } }) diff --git a/modules/apps/27-interchain-accounts/types/codec.go b/modules/apps/27-interchain-accounts/types/codec.go index 68d19fb0db3..9828a387708 100644 --- a/modules/apps/27-interchain-accounts/types/codec.go +++ b/modules/apps/27-interchain-accounts/types/codec.go @@ -84,7 +84,7 @@ func DeserializeCosmosTx(cdc codec.Codec, data []byte, encoding string) ([]sdk.M switch encoding { case EncodingProtobuf: if err := cdc.Unmarshal(data, &cosmosTx); err != nil { - return nil, errorsmod.Wrapf(err, "cannot unmarshal CosmosTx with protobuf") + return nil, errorsmod.Wrapf(ErrUnknownDataType, "cannot unmarshal CosmosTx with protobuf") } case EncodingProto3JSON: if err := cdc.UnmarshalJSON(data, &cosmosTx); err != nil { From 393a37003c70c3a5eda71a7e81952302576ef953 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 2 Aug 2023 13:39:38 +0300 Subject: [PATCH 3/5] Use similar error checking as other tests. --- .../27-interchain-accounts/host/keeper/relay_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go index 0020a7ab635..80b3de38bd0 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go @@ -529,10 +529,12 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { txResponse, err := suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) - suite.Require().ErrorIs(tc.expErr, err) - if tc.expErr == nil { + expPass := tc.expErr == nil + if expPass { + suite.Require().Nil(err) suite.Require().NotNil(txResponse) } else { + suite.Require().ErrorIs(err, tc.expErr) suite.Require().Nil(txResponse) } }) @@ -866,10 +868,12 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { txResponse, err := suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) - suite.Require().ErrorIs(tc.expErr, err) - if tc.expErr == nil { + expPass := tc.expErr == nil + if expPass { + suite.Require().Nil(err) suite.Require().NotNil(txResponse) } else { + suite.Require().ErrorIs(err, tc.expErr) suite.Require().Nil(txResponse) } }) From 38e65394243bd82a6d4faddf15d95db2c573cfc7 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Mon, 7 Aug 2023 10:20:01 +0300 Subject: [PATCH 4/5] Use NoError, wrap err in error message. --- modules/apps/27-interchain-accounts/host/keeper/relay_test.go | 4 ++-- modules/apps/27-interchain-accounts/types/codec.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go index 80b3de38bd0..12258fa51f0 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go @@ -531,7 +531,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { expPass := tc.expErr == nil if expPass { - suite.Require().Nil(err) + suite.Require().NoError(err) suite.Require().NotNil(txResponse) } else { suite.Require().ErrorIs(err, tc.expErr) @@ -870,7 +870,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { expPass := tc.expErr == nil if expPass { - suite.Require().Nil(err) + suite.Require().NoError(err) suite.Require().NotNil(txResponse) } else { suite.Require().ErrorIs(err, tc.expErr) diff --git a/modules/apps/27-interchain-accounts/types/codec.go b/modules/apps/27-interchain-accounts/types/codec.go index 9828a387708..0f4fc2028a8 100644 --- a/modules/apps/27-interchain-accounts/types/codec.go +++ b/modules/apps/27-interchain-accounts/types/codec.go @@ -84,7 +84,7 @@ func DeserializeCosmosTx(cdc codec.Codec, data []byte, encoding string) ([]sdk.M switch encoding { case EncodingProtobuf: if err := cdc.Unmarshal(data, &cosmosTx); err != nil { - return nil, errorsmod.Wrapf(ErrUnknownDataType, "cannot unmarshal CosmosTx with protobuf") + return nil, errorsmod.Wrapf(ErrUnknownDataType, "cannot unmarshal CosmosTx with protobuf: %v", err) } case EncodingProto3JSON: if err := cdc.UnmarshalJSON(data, &cosmosTx); err != nil { From 8bfa0802dba9ae16579c29c3d633ce085f12a4ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 5 Oct 2023 11:58:57 +0200 Subject: [PATCH 5/5] fix: relay test expected result --- modules/apps/27-interchain-accounts/host/keeper/relay_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go index 2fb2dc3821c..3d4cd9ae9e2 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go @@ -820,7 +820,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { "messages": [ { "@type": "/cosmos.bank.v1beta1.MsgSend", - "from_address": "` + ibctesting.InvalidID + `", + "from_address": "` + suite.chainB.SenderAccount.GetAddress().String() + `", // unexpected signer "to_address": "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs", "amount": [{ "denom": "stake", "amount": "100" }] } @@ -836,7 +836,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() { params := types.NewParams(true, []string{sdk.MsgTypeURL((*banktypes.MsgSend)(nil))}) suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params) }, - ibcerrors.ErrUnauthorized, + icatypes.ErrUnknownDataType, }, }