From a3786a9a3efba3dac66d3323c7cad7c9d03e3b4a Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 23 May 2024 14:42:23 +0100 Subject: [PATCH 1/3] chore: refactoring msgs_test.go to use expError --- modules/apps/transfer/types/msgs_test.go | 56 +++++++++++++----------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index 602551980c6..f0bd6974a01 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -3,6 +3,7 @@ package types_test import ( "testing" + "github.com/stretchr/testify/require" sdkmath "cosmossdk.io/math" @@ -14,6 +15,8 @@ import ( "github.com/cosmos/ibc-go/v8/modules/apps/transfer" "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + host "github.com/cosmos/ibc-go/v8/modules/core/24-host" + ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" ibctesting "github.com/cosmos/ibc-go/v8/testing" ) @@ -50,41 +53,44 @@ var ( // TestMsgTransferValidation tests ValidateBasic for MsgTransfer func TestMsgTransferValidation(t *testing.T) { testCases := []struct { - name string - msg *types.MsgTransfer - expPass bool + name string + msg *types.MsgTransfer + expError error }{ - {"valid msg with base denom", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ""), true}, - {"valid msg with trace hash", types.NewMsgTransfer(validPort, validChannel, ibcCoins, sender, receiver, timeoutHeight, 0, ""), true}, - {"multidenom", types.NewMsgTransfer(validPort, validChannel, coins.Add(ibcCoins...), sender, receiver, timeoutHeight, 0, ""), true}, - {"invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, invalidIBCCoins, sender, receiver, timeoutHeight, 0, ""), false}, - {"too short port id", types.NewMsgTransfer(invalidShortPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ""), false}, - {"too long port id", types.NewMsgTransfer(invalidLongPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ""), false}, - {"port id contains non-alpha", types.NewMsgTransfer(invalidPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ""), false}, - {"too short channel id", types.NewMsgTransfer(validPort, invalidShortChannel, coins, sender, receiver, timeoutHeight, 0, ""), false}, - {"too long channel id", types.NewMsgTransfer(validPort, invalidLongChannel, coins, sender, receiver, timeoutHeight, 0, ""), false}, - {"too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ibctesting.GenerateString(types.MaximumMemoLength+1)), false}, - {"channel id contains non-alpha", types.NewMsgTransfer(validPort, invalidChannel, coins, sender, receiver, timeoutHeight, 0, ""), false}, - {"invalid denom", types.NewMsgTransfer(validPort, validChannel, invalidDenomCoins, sender, receiver, timeoutHeight, 0, ""), false}, - {"zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, ""), false}, - {"missing sender address", types.NewMsgTransfer(validPort, validChannel, coins, emptyAddr, receiver, timeoutHeight, 0, ""), false}, - {"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, "", timeoutHeight, 0, ""), false}, - {"too long recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, ibctesting.GenerateString(types.MaximumReceiverLength+1), timeoutHeight, 0, ""), false}, - {"empty coins", types.NewMsgTransfer(validPort, validChannel, sdk.NewCoins(), sender, receiver, timeoutHeight, 0, ""), false}, - {"multidenom: invalid denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidDenomCoins...), sender, receiver, timeoutHeight, 0, ""), false}, - {"multidenom: invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidIBCCoins...), sender, receiver, timeoutHeight, 0, ""), false}, - {"multidenom: zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, ""), false}, - {"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, timeoutHeight, 0, ""), false}, + {"valid msg with base denom", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ""), nil}, + {"valid msg with trace hash", types.NewMsgTransfer(validPort, validChannel, ibcCoins, sender, receiver, timeoutHeight, 0, ""), nil}, + {"multidenom", types.NewMsgTransfer(validPort, validChannel, coins.Add(ibcCoins...), sender, receiver, timeoutHeight, 0, ""), nil}, + {"invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, invalidIBCCoins, sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, + {"too short port id", types.NewMsgTransfer(invalidShortPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ""), host.ErrInvalidID}, + {"too long port id", types.NewMsgTransfer(invalidLongPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ""), host.ErrInvalidID}, + {"port id contains non-alpha", types.NewMsgTransfer(invalidPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ""), host.ErrInvalidID}, + {"too short channel id", types.NewMsgTransfer(validPort, invalidShortChannel, coins, sender, receiver, timeoutHeight, 0, ""), host.ErrInvalidID}, + {"too long channel id", types.NewMsgTransfer(validPort, invalidLongChannel, coins, sender, receiver, timeoutHeight, 0, ""), host.ErrInvalidID}, + {"too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ibctesting.GenerateString(types.MaximumMemoLength+1)), types.ErrInvalidMemo}, + {"channel id contains non-alpha", types.NewMsgTransfer(validPort, invalidChannel, coins, sender, receiver, timeoutHeight, 0, ""), host.ErrInvalidID}, + {"invalid denom", types.NewMsgTransfer(validPort, validChannel, invalidDenomCoins, sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, + {"zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, ""), types.ErrInvalidAmount}, + {"missing sender address", types.NewMsgTransfer(validPort, validChannel, coins, emptyAddr, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidAddress}, + {"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, "", timeoutHeight, 0, ""), ibcerrors.ErrInvalidAddress}, + {"too long recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, ibctesting.GenerateString(types.MaximumReceiverLength+1), timeoutHeight, 0, ""), ibcerrors.ErrInvalidAddress}, + {"empty coins", types.NewMsgTransfer(validPort, validChannel, sdk.NewCoins(), sender, receiver, timeoutHeight, 0, ""), types.ErrInvalidAmount}, + {"multidenom: invalid denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidDenomCoins...), sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, + {"multidenom: invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidIBCCoins...), sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, + {"multidenom: zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, ""), types.ErrInvalidAmount}, + {"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, } for i, tc := range testCases { tc := tc err := tc.msg.ValidateBasic() - if tc.expPass { + + expPass := tc.expError == nil + if expPass { require.NoError(t, err, "valid test case %d failed: %s", i, tc.name) } else { require.Error(t, err, "invalid test case %d passed: %s", i, tc.name) + require.ErrorIs(t, err, tc.expError, "expected error %s but got %s", tc.expError, err) } } } From f4b13479848299a8348e8610fe97c921e7594fba Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 23 May 2024 16:21:25 +0100 Subject: [PATCH 2/3] chore: updating expected errors --- modules/apps/transfer/types/msgs.go | 4 ++-- modules/apps/transfer/types/msgs_test.go | 11 +++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index fd28b8778e5..8003b1e9f21 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -102,7 +102,7 @@ func (msg MsgTransfer) ValidateBasic() error { for _, coin := range msg.GetCoins() { if err := validateIBCCoin(coin); err != nil { - return errorsmod.Wrap(err, coin.String()) + return errorsmod.Wrapf(ibcerrors.ErrInvalidCoins, "%s: %s", err.Error(), coin.String()) } } @@ -137,7 +137,7 @@ func validateIBCCoin(coin sdk.Coin) error { return errorsmod.Wrap(ErrInvalidAmount, "amount must be positive") } if err := validateIBCDenom(coin.GetDenom()); err != nil { - return err + return errorsmod.Wrap(ErrInvalidDenomForTransfer, err.Error()) } return nil diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index f0bd6974a01..e0e83e22b83 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -1,9 +1,10 @@ package types_test import ( + host "github.com/cosmos/ibc-go/v8/modules/core/24-host" + ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" "testing" - "github.com/stretchr/testify/require" sdkmath "cosmossdk.io/math" @@ -15,8 +16,6 @@ import ( "github.com/cosmos/ibc-go/v8/modules/apps/transfer" "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" - host "github.com/cosmos/ibc-go/v8/modules/core/24-host" - ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" ibctesting "github.com/cosmos/ibc-go/v8/testing" ) @@ -69,14 +68,14 @@ func TestMsgTransferValidation(t *testing.T) { {"too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ibctesting.GenerateString(types.MaximumMemoLength+1)), types.ErrInvalidMemo}, {"channel id contains non-alpha", types.NewMsgTransfer(validPort, invalidChannel, coins, sender, receiver, timeoutHeight, 0, ""), host.ErrInvalidID}, {"invalid denom", types.NewMsgTransfer(validPort, validChannel, invalidDenomCoins, sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, - {"zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, ""), types.ErrInvalidAmount}, + {"zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, {"missing sender address", types.NewMsgTransfer(validPort, validChannel, coins, emptyAddr, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidAddress}, {"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, "", timeoutHeight, 0, ""), ibcerrors.ErrInvalidAddress}, {"too long recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, ibctesting.GenerateString(types.MaximumReceiverLength+1), timeoutHeight, 0, ""), ibcerrors.ErrInvalidAddress}, - {"empty coins", types.NewMsgTransfer(validPort, validChannel, sdk.NewCoins(), sender, receiver, timeoutHeight, 0, ""), types.ErrInvalidAmount}, + {"empty coins", types.NewMsgTransfer(validPort, validChannel, sdk.NewCoins(), sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, {"multidenom: invalid denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidDenomCoins...), sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, {"multidenom: invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidIBCCoins...), sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, - {"multidenom: zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, ""), types.ErrInvalidAmount}, + {"multidenom: zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, {"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, } From d5f1389c2b56d53a3d66c47e93d1a52146debd19 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 23 May 2024 21:53:15 +0300 Subject: [PATCH 3/3] chore: update MsgUpdateParams and lint --- modules/apps/transfer/types/msgs_test.go | 34 +++++++++++++----------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index e0e83e22b83..8e9c8656c63 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -1,8 +1,6 @@ package types_test import ( - host "github.com/cosmos/ibc-go/v8/modules/core/24-host" - ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" "testing" "github.com/stretchr/testify/require" @@ -16,6 +14,8 @@ import ( "github.com/cosmos/ibc-go/v8/modules/apps/transfer" "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + host "github.com/cosmos/ibc-go/v8/modules/core/24-host" + ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" ibctesting "github.com/cosmos/ibc-go/v8/testing" ) @@ -79,17 +79,16 @@ func TestMsgTransferValidation(t *testing.T) { {"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, timeoutHeight, 0, ""), ibcerrors.ErrInvalidCoins}, } - for i, tc := range testCases { + for _, tc := range testCases { tc := tc err := tc.msg.ValidateBasic() expPass := tc.expError == nil if expPass { - require.NoError(t, err, "valid test case %d failed: %s", i, tc.name) + require.NoError(t, err) } else { - require.Error(t, err, "invalid test case %d passed: %s", i, tc.name) - require.ErrorIs(t, err, tc.expError, "expected error %s but got %s", tc.expError, err) + require.ErrorIs(t, err, tc.expError) } } } @@ -108,23 +107,25 @@ func TestMsgTransferGetSigners(t *testing.T) { // TestMsgUpdateParamsValidateBasic tests ValidateBasic for MsgUpdateParams func TestMsgUpdateParamsValidateBasic(t *testing.T) { testCases := []struct { - name string - msg *types.MsgUpdateParams - expPass bool + name string + msg *types.MsgUpdateParams + expError error }{ - {"success: valid signer and valid params", types.NewMsgUpdateParams(ibctesting.TestAccAddress, types.DefaultParams()), true}, - {"failure: invalid signer with valid params", types.NewMsgUpdateParams(invalidAddress, types.DefaultParams()), false}, - {"failure: empty signer with valid params", types.NewMsgUpdateParams(emptyAddr, types.DefaultParams()), false}, + {"success: valid signer and valid params", types.NewMsgUpdateParams(ibctesting.TestAccAddress, types.DefaultParams()), nil}, + {"failure: invalid signer with valid params", types.NewMsgUpdateParams(invalidAddress, types.DefaultParams()), ibcerrors.ErrInvalidAddress}, + {"failure: empty signer with valid params", types.NewMsgUpdateParams(emptyAddr, types.DefaultParams()), ibcerrors.ErrInvalidAddress}, } - for i, tc := range testCases { + for _, tc := range testCases { tc := tc err := tc.msg.ValidateBasic() - if tc.expPass { - require.NoError(t, err, "valid test case %d failed: %s", i, tc.name) + + expPass := tc.expError == nil + if expPass { + require.NoError(t, err) } else { - require.Error(t, err, "invalid test case %d passed: %s", i, tc.name) + require.ErrorIs(t, err, tc.expError) } } } @@ -150,6 +151,7 @@ func TestMsgUpdateParamsGetSigners(t *testing.T) { 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])