From ff5a93e3e60028367943fb8c3d31f7159a382ac0 Mon Sep 17 00:00:00 2001 From: Gjermund Garaba Date: Tue, 11 Jun 2024 16:09:40 +0200 Subject: [PATCH 1/2] feat(transfer): add validation for transfer msg forwarding path --- modules/apps/transfer/types/msgs.go | 11 ++++ modules/apps/transfer/types/msgs_test.go | 77 +++++++++++++----------- 2 files changed, 52 insertions(+), 36 deletions(-) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index 3c10f3b5779..a50a6e5719a 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -102,6 +102,17 @@ func (msg MsgTransfer) ValidateBasic() error { return errorsmod.Wrapf(ErrInvalidMemo, "memo must not exceed %d bytes", MaximumMemoLength) } + if msg.ForwardingPath != nil { + if err := msg.ForwardingPath.Validate(); err != nil { + return err + } + + // We cannot have non-empty memo and non-empty forwarding path hops at the same time. + if len(msg.ForwardingPath.Hops) > 0 && msg.Memo != "" { + return errorsmod.Wrapf(ErrInvalidMemo, "memo must be empty if forwarding path hops is not empty: %s, %s", msg.Memo, msg.ForwardingPath.Hops) + } + } + for _, coin := range msg.GetCoins() { if err := validateIBCCoin(coin); err != nil { return errorsmod.Wrapf(ibcerrors.ErrInvalidCoins, "%s: %s", err.Error(), coin.String()) diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index 2f153eacca0..18cf046a8e1 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -79,19 +79,24 @@ func TestMsgTransferValidation(t *testing.T) { {"multidenom: zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, "", nil), ibcerrors.ErrInvalidCoins}, {"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, timeoutHeight, 0, "", nil), ibcerrors.ErrInvalidCoins}, {"multidenom: both token and tokens are set", &types.MsgTransfer{validPort, validChannel, coin, sender, receiver, timeoutHeight, 0, "", coins, nil}, ibcerrors.ErrInvalidCoins}, + {"memo must be empty if forwarding path hops is not empty", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "memo", types.NewForwardingInfo("", validHop)), types.ErrInvalidMemo}, + {"invalid forwarding info port", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo("", &types.Hop{PortId: invalidPort, ChannelId: validChannel})), host.ErrInvalidID}, + {"invalid forwarding info channel", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo("", &types.Hop{PortId: validPort, ChannelId: invalidChannel})), host.ErrInvalidID}, + {"invalid forwarding info too many hops", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo("", generateHops(types.MaximumNumberOfForwardingHops+1)...)), types.ErrInvalidForwardingInfo}, + {"invalid forwarding info too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo(ibctesting.GenerateString(types.MaximumMemoLength+1), validHop)), types.ErrInvalidMemo}, } for _, tc := range testCases { - tc := tc - - err := tc.msg.ValidateBasic() - - expPass := tc.expError == nil - if expPass { - require.NoError(t, err) - } else { - require.ErrorIs(t, err, tc.expError) - } + t.Run(tc.name, func(t *testing.T) { + err := tc.msg.ValidateBasic() + + expPass := tc.expError == nil + if expPass { + require.NoError(t, err) + } else { + require.ErrorIs(t, err, tc.expError) + } + }) } } @@ -119,16 +124,16 @@ func TestMsgUpdateParamsValidateBasic(t *testing.T) { } for _, tc := range testCases { - tc := tc - - err := tc.msg.ValidateBasic() - - expPass := tc.expError == nil - if expPass { - require.NoError(t, err) - } else { - require.ErrorIs(t, err, tc.expError) - } + t.Run(tc.name, func(t *testing.T) { + err := tc.msg.ValidateBasic() + + expPass := tc.expError == nil + if expPass { + require.NoError(t, err) + } else { + require.ErrorIs(t, err, tc.expError) + } + }) } } @@ -144,21 +149,21 @@ func TestMsgUpdateParamsGetSigners(t *testing.T) { } for _, tc := range testCases { - tc := tc - - msg := types.MsgUpdateParams{ - Signer: tc.address.String(), - Params: types.DefaultParams(), - } - - encodingCfg := moduletestutil.MakeTestEncodingConfig(transfer.AppModuleBasic{}) - signers, _, err := encodingCfg.Codec.GetMsgV1Signers(&msg) - - if tc.expPass { - require.NoError(t, err) - require.Equal(t, tc.address.Bytes(), signers[0]) - } else { - require.Error(t, err) - } + t.Run(tc.name, func(t *testing.T) { + msg := types.MsgUpdateParams{ + Signer: tc.address.String(), + Params: types.DefaultParams(), + } + + encodingCfg := moduletestutil.MakeTestEncodingConfig(transfer.AppModuleBasic{}) + signers, _, err := encodingCfg.Codec.GetMsgV1Signers(&msg) + + if tc.expPass { + require.NoError(t, err) + require.Equal(t, tc.address.Bytes(), signers[0]) + } else { + require.Error(t, err) + } + }) } } From a40a80ba882eb1ff2b15c1217e660c66eb0d7b80 Mon Sep 17 00:00:00 2001 From: Gjermund Garaba Date: Wed, 12 Jun 2024 18:17:42 +0200 Subject: [PATCH 2/2] Remove pointer from Hop in NewMsgTransfer test --- modules/apps/transfer/types/msgs_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index 18cf046a8e1..447fd619a36 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -80,8 +80,8 @@ func TestMsgTransferValidation(t *testing.T) { {"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, timeoutHeight, 0, "", nil), ibcerrors.ErrInvalidCoins}, {"multidenom: both token and tokens are set", &types.MsgTransfer{validPort, validChannel, coin, sender, receiver, timeoutHeight, 0, "", coins, nil}, ibcerrors.ErrInvalidCoins}, {"memo must be empty if forwarding path hops is not empty", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "memo", types.NewForwardingInfo("", validHop)), types.ErrInvalidMemo}, - {"invalid forwarding info port", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo("", &types.Hop{PortId: invalidPort, ChannelId: validChannel})), host.ErrInvalidID}, - {"invalid forwarding info channel", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo("", &types.Hop{PortId: validPort, ChannelId: invalidChannel})), host.ErrInvalidID}, + {"invalid forwarding info port", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo("", types.Hop{PortId: invalidPort, ChannelId: validChannel})), host.ErrInvalidID}, + {"invalid forwarding info channel", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo("", types.Hop{PortId: validPort, ChannelId: invalidChannel})), host.ErrInvalidID}, {"invalid forwarding info too many hops", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo("", generateHops(types.MaximumNumberOfForwardingHops+1)...)), types.ErrInvalidForwardingInfo}, {"invalid forwarding info too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwardingInfo(ibctesting.GenerateString(types.MaximumMemoLength+1), validHop)), types.ErrInvalidMemo}, }