From 70436fa5ec1752a4ea6d7c7e7d04237893a18711 Mon Sep 17 00:00:00 2001 From: atheeshp <59333759+atheeshp@users.noreply.github.com> Date: Thu, 13 Apr 2023 01:45:31 +0530 Subject: [PATCH] refactor(x/bank): move validate basic checks to msg server (#15782) --- x/authz/keeper/msg_server_test.go | 14 -- x/bank/app_test.go | 25 --- x/bank/client/cli/tx.go | 4 + x/bank/keeper/msg_server.go | 61 ++++- x/bank/keeper/msg_server_test.go | 362 ++++++++++++++++++++++++++++++ x/bank/keeper/msg_service_test.go | 172 -------------- x/bank/types/inputs_outputs.go | 83 +++++++ x/bank/types/msgs.go | 157 ------------- x/bank/types/msgs_test.go | 220 ------------------ 9 files changed, 503 insertions(+), 595 deletions(-) create mode 100644 x/bank/keeper/msg_server_test.go delete mode 100644 x/bank/keeper/msg_service_test.go create mode 100644 x/bank/types/inputs_outputs.go diff --git a/x/authz/keeper/msg_server_test.go b/x/authz/keeper/msg_server_test.go index 53d8eb04bf..5143b8dd39 100644 --- a/x/authz/keeper/msg_server_test.go +++ b/x/authz/keeper/msg_server_test.go @@ -352,20 +352,6 @@ func (suite *TestSuite) TestExec() { return authz.NewMsgExec(grantee, []sdk.Msg{msg}) }, }, - { - name: "invalid nested msg", - malleate: func() authz.MsgExec { - return authz.NewMsgExec(grantee, []sdk.Msg{ - &banktypes.MsgSend{ - Amount: sdk.NewCoins(sdk.NewInt64Coin("steak", 2)), - FromAddress: "invalid_from_address", - ToAddress: grantee.String(), - }, - }) - }, - expErr: true, - errMsg: "invalid from address", - }, } for _, tc := range testCases { diff --git a/x/bank/app_test.go b/x/bank/app_test.go index 51993e1dd5..a47e060a96 100644 --- a/x/bank/app_test.go +++ b/x/bank/app_test.go @@ -369,18 +369,6 @@ func TestMsgSetSendEnabled(t *testing.T) { false, ) require.NoError(t, err, "making goodGovProp") - badGovProp, err := govv1.NewMsgSubmitProposal( - []sdk.Msg{ - types.NewMsgSetSendEnabled(govAddr, []*types.SendEnabled{{Denom: "bad coin name!", Enabled: true}}, nil), - }, - sdk.Coins{{Denom: "foocoin", Amount: sdkmath.NewInt(5)}}, - addr1Str, - "set default send enabled to true", - "Change send enabled", - "Modify send enabled and set to true", - false, - ) - require.NoError(t, err, "making badGovProp") testCases := []appTestCase{ { @@ -423,19 +411,6 @@ func TestMsgSetSendEnabled(t *testing.T) { accSeqs: []uint64{1}, expInError: nil, }, - { - desc: "submitted bad as gov prop", - expSimPass: false, - expPass: false, - msgs: []sdk.Msg{ - badGovProp, - }, - accSeqs: []uint64{2}, - expInError: []string{ - "invalid denom: bad coin name!", - "invalid proposal message", - }, - }, } for _, tc := range testCases { diff --git a/x/bank/client/cli/tx.go b/x/bank/client/cli/tx.go index d22d59a26d..3ccf19c360 100644 --- a/x/bank/client/cli/tx.go +++ b/x/bank/client/cli/tx.go @@ -60,6 +60,10 @@ When using '--dry-run' a key name cannot be used, only a bech32 address. return err } + if len(coins) == 0 { + return fmt.Errorf("invalid coins") + } + msg := types.NewMsgSend(clientCtx.GetFromAddress(), toAddr, coins) return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg) diff --git a/x/bank/keeper/msg_server.go b/x/bank/keeper/msg_server.go index 77d58eefd1..1a4209411d 100644 --- a/x/bank/keeper/msg_server.go +++ b/x/bank/keeper/msg_server.go @@ -27,18 +27,26 @@ func NewMsgServerImpl(keeper Keeper) types.MsgServer { } func (k msgServer) Send(goCtx context.Context, msg *types.MsgSend) (*types.MsgSendResponse, error) { - ctx := sdk.UnwrapSDKContext(goCtx) - - if err := k.IsSendEnabledCoins(ctx, msg.Amount...); err != nil { - return nil, err - } - from, err := sdk.AccAddressFromBech32(msg.FromAddress) if err != nil { - return nil, err + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid from address: %s", err) } + to, err := sdk.AccAddressFromBech32(msg.ToAddress) if err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid to address: %s", err) + } + + if !msg.Amount.IsValid() { + return nil, errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String()) + } + + if !msg.Amount.IsAllPositive() { + return nil, errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String()) + } + + ctx := sdk.UnwrapSDKContext(goCtx) + if err := k.IsSendEnabledCoins(ctx, msg.Amount...); err != nil { return nil, err } @@ -67,6 +75,22 @@ func (k msgServer) Send(goCtx context.Context, msg *types.MsgSend) (*types.MsgSe } func (k msgServer) MultiSend(goCtx context.Context, msg *types.MsgMultiSend) (*types.MsgMultiSendResponse, error) { + if len(msg.Inputs) == 0 { + return nil, types.ErrNoInputs + } + + if len(msg.Inputs) != 1 { + return nil, types.ErrMultipleSenders + } + + if len(msg.Outputs) == 0 { + return nil, types.ErrNoOutputs + } + + if err := types.ValidateInputOutputs(msg.Inputs[0], msg.Outputs); err != nil { + return nil, err + } + ctx := sdk.UnwrapSDKContext(goCtx) // NOTE: totalIn == totalOut should already have been checked @@ -97,6 +121,10 @@ func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParam return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority) } + if err := req.Params.Validate(); err != nil { + return nil, err + } + ctx := sdk.UnwrapSDKContext(goCtx) if err := k.SetParams(ctx, req.Params); err != nil { return nil, err @@ -110,6 +138,25 @@ func (k msgServer) SetSendEnabled(goCtx context.Context, msg *types.MsgSetSendEn return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), msg.Authority) } + seen := map[string]bool{} + for _, se := range msg.SendEnabled { + if _, alreadySeen := seen[se.Denom]; alreadySeen { + return nil, sdkerrors.ErrInvalidRequest.Wrapf("duplicate denom entries found for %q", se.Denom) + } + + seen[se.Denom] = true + + if err := se.Validate(); err != nil { + return nil, sdkerrors.ErrInvalidRequest.Wrapf("invalid SendEnabled denom %q: %s", se.Denom, err) + } + } + + for _, denom := range msg.UseDefaultFor { + if err := sdk.ValidateDenom(denom); err != nil { + return nil, sdkerrors.ErrInvalidRequest.Wrapf("invalid UseDefaultFor denom %q: %s", denom, err) + } + } + ctx := sdk.UnwrapSDKContext(goCtx) if len(msg.SendEnabled) > 0 { k.SetAllSendEnabled(ctx, msg.SendEnabled) diff --git a/x/bank/keeper/msg_server_test.go b/x/bank/keeper/msg_server_test.go new file mode 100644 index 0000000000..0a4902688a --- /dev/null +++ b/x/bank/keeper/msg_server_test.go @@ -0,0 +1,362 @@ +package keeper_test + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" +) + +var govAcc = authtypes.NewEmptyModuleAccount(govtypes.ModuleName, authtypes.Minter) + +func (suite *KeeperTestSuite) TestMsgUpdateParams() { + // default params + params := banktypes.DefaultParams() + + testCases := []struct { + name string + input *banktypes.MsgUpdateParams + expErr bool + expErrMsg string + }{ + { + name: "invalid authority", + input: &banktypes.MsgUpdateParams{ + Authority: "invalid", + Params: params, + }, + expErr: true, + expErrMsg: "invalid authority", + }, + { + name: "send enabled param", + input: &banktypes.MsgUpdateParams{ + Authority: suite.bankKeeper.GetAuthority(), + Params: banktypes.Params{ + SendEnabled: []*banktypes.SendEnabled{ + {Denom: "foo", Enabled: true}, + }, + }, + }, + expErr: true, + expErrMsg: "use of send_enabled in params is no longer supported", + }, + { + name: "all good", + input: &banktypes.MsgUpdateParams{ + Authority: suite.bankKeeper.GetAuthority(), + Params: params, + }, + expErr: false, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + _, err := suite.msgServer.UpdateParams(suite.ctx, tc.input) + + if tc.expErr { + suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.expErrMsg) + } else { + suite.Require().NoError(err) + } + }) + } +} + +func (suite *KeeperTestSuite) TestMsgSend() { + origCoins := sdk.NewCoins(sdk.NewInt64Coin("sendableCoin", 100)) + suite.bankKeeper.SetSendEnabled(suite.ctx, origCoins.Denoms()[0], true) + atom0 := sdk.NewCoins(sdk.NewInt64Coin("atom", 0)) + atom123eth0 := sdk.Coins{sdk.NewInt64Coin("atom", 123), sdk.NewInt64Coin("eth", 0)} + + testCases := []struct { + name string + input *banktypes.MsgSend + expErr bool + expErrMsg string + }{ + { + name: "invalid send to blocked address", + input: &banktypes.MsgSend{ + FromAddress: minterAcc.GetAddress().String(), + ToAddress: accAddrs[4].String(), + Amount: origCoins, + }, + expErr: true, + expErrMsg: "is not allowed to receive funds", + }, + { + name: "invalid coins", + input: &banktypes.MsgSend{ + FromAddress: minterAcc.GetAddress().String(), + ToAddress: baseAcc.Address, + Amount: atom0, + }, + expErr: true, + expErrMsg: "invalid coins", + }, + { + name: "123atom,0eth: invalid coins", + input: &banktypes.MsgSend{ + FromAddress: minterAcc.GetAddress().String(), + ToAddress: baseAcc.Address, + Amount: atom123eth0, + }, + expErr: true, + expErrMsg: "123atom,0eth: invalid coins", + }, + { + name: "invalid from address: empty address string is not allowed: invalid address", + input: &banktypes.MsgSend{ + FromAddress: "", + ToAddress: baseAcc.Address, + Amount: origCoins, + }, + expErr: true, + expErrMsg: "empty address string is not allowed", + }, + { + name: "invalid to address: empty address string is not allowed: invalid address", + input: &banktypes.MsgSend{ + FromAddress: minterAcc.GetAddress().String(), + ToAddress: "", + Amount: origCoins, + }, + expErr: true, + expErrMsg: "empty address string is not allowed", + }, + { + name: "all good", + input: &banktypes.MsgSend{ + FromAddress: minterAcc.GetAddress().String(), + ToAddress: baseAcc.Address, + Amount: origCoins, + }, + expErr: false, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + suite.mockMintCoins(minterAcc) + suite.bankKeeper.MintCoins(suite.ctx, minterAcc.Name, origCoins) + if !tc.expErr { + suite.mockSendCoins(suite.ctx, minterAcc, baseAcc.GetAddress()) + } + _, err := suite.msgServer.Send(suite.ctx, tc.input) + if tc.expErr { + suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.expErrMsg) + } else { + suite.Require().NoError(err) + } + }) + } +} + +func (suite *KeeperTestSuite) TestMsgMultiSend() { + origDenom := "sendableCoin" + origCoins := sdk.NewCoins(sdk.NewInt64Coin(origDenom, 100)) + sendCoins := sdk.NewCoins(sdk.NewInt64Coin(origDenom, 50)) + suite.bankKeeper.SetSendEnabled(suite.ctx, origDenom, true) + + testCases := []struct { + name string + input *banktypes.MsgMultiSend + expErr bool + expErrMsg string + }{ + { + name: "no inputs to send transaction", + input: &banktypes.MsgMultiSend{}, + expErr: true, + expErrMsg: "no inputs to send transaction", + }, + { + name: "no inputs to send transaction", + input: &banktypes.MsgMultiSend{ + Outputs: []banktypes.Output{ + {Address: accAddrs[4].String(), Coins: sendCoins}, + }, + }, + expErr: true, + expErrMsg: "no inputs to send transaction", + }, + { + name: "more than one inputs to send transaction", + input: &banktypes.MsgMultiSend{ + Inputs: []banktypes.Input{ + {Address: minterAcc.GetAddress().String(), Coins: origCoins}, + {Address: minterAcc.GetAddress().String(), Coins: origCoins}, + }, + }, + expErr: true, + expErrMsg: "multiple senders not allowed", + }, + { + name: "no outputs to send transaction", + input: &banktypes.MsgMultiSend{ + Inputs: []banktypes.Input{ + {Address: minterAcc.GetAddress().String(), Coins: origCoins}, + }, + }, + expErr: true, + expErrMsg: "no outputs to send transaction", + }, + { + name: "invalid send to blocked address", + input: &banktypes.MsgMultiSend{ + Inputs: []banktypes.Input{ + {Address: minterAcc.GetAddress().String(), Coins: origCoins}, + }, + Outputs: []banktypes.Output{ + {Address: accAddrs[0].String(), Coins: sendCoins}, + {Address: accAddrs[4].String(), Coins: sendCoins}, + }, + }, + expErr: true, + expErrMsg: "is not allowed to receive funds", + }, + { + name: "invalid send to blocked address", + input: &banktypes.MsgMultiSend{ + Inputs: []banktypes.Input{ + {Address: minterAcc.GetAddress().String(), Coins: origCoins}, + }, + Outputs: []banktypes.Output{ + {Address: accAddrs[0].String(), Coins: sendCoins}, + {Address: accAddrs[1].String(), Coins: sendCoins}, + }, + }, + expErr: false, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + suite.mockMintCoins(minterAcc) + suite.bankKeeper.MintCoins(suite.ctx, minterAcc.Name, origCoins) + if !tc.expErr { + suite.mockInputOutputCoins([]sdk.AccountI{minterAcc}, accAddrs[:2]) + } + _, err := suite.msgServer.MultiSend(suite.ctx, tc.input) + if tc.expErr { + suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.expErrMsg) + } else { + suite.Require().NoError(err) + } + }) + } +} + +func (suite *KeeperTestSuite) TestMsgSetSendEnabled() { + testCases := []struct { + name string + req *banktypes.MsgSetSendEnabled + isExpErr bool + errMsg string + }{ + { + name: "all good", + req: banktypes.NewMsgSetSendEnabled( + govAcc.GetAddress().String(), + []*banktypes.SendEnabled{ + banktypes.NewSendEnabled("atom1", true), + }, + []string{}, + ), + }, + { + name: "all good with two denoms", + req: banktypes.NewMsgSetSendEnabled( + govAcc.GetAddress().String(), + []*banktypes.SendEnabled{ + banktypes.NewSendEnabled("atom1", true), + banktypes.NewSendEnabled("atom2", true), + }, + []string{"defcoinc", "defcoind"}, + ), + }, + { + name: "duplicate denoms", + req: banktypes.NewMsgSetSendEnabled( + govAcc.GetAddress().String(), + []*banktypes.SendEnabled{ + banktypes.NewSendEnabled("atom", true), + banktypes.NewSendEnabled("atom", true), + }, + []string{}, + ), + isExpErr: true, + errMsg: `duplicate denom entries found for "atom": invalid request`, + }, + { + name: "bad first denom name, (invalid send enabled denom present in list)", + req: banktypes.NewMsgSetSendEnabled( + govAcc.GetAddress().String(), + []*banktypes.SendEnabled{ + banktypes.NewSendEnabled("not a denom", true), + banktypes.NewSendEnabled("somecoin", true), + }, + []string{}, + ), + isExpErr: true, + errMsg: `invalid SendEnabled denom "not a denom": invalid denom: not a denom: invalid request`, + }, + { + name: "bad second denom name, (invalid send enabled denom present in list)", + req: banktypes.NewMsgSetSendEnabled( + govAcc.GetAddress().String(), + []*banktypes.SendEnabled{ + banktypes.NewSendEnabled("somecoin", true), + banktypes.NewSendEnabled("not a denom", true), + }, + []string{}, + ), + isExpErr: true, + errMsg: `invalid SendEnabled denom "not a denom": invalid denom: not a denom: invalid request`, + }, + { + name: "invalid UseDefaultFor denom", + req: banktypes.NewMsgSetSendEnabled( + govAcc.GetAddress().String(), + []*banktypes.SendEnabled{ + banktypes.NewSendEnabled("atom", true), + }, + []string{"not a denom"}, + ), + isExpErr: true, + errMsg: `invalid UseDefaultFor denom "not a denom": invalid denom: not a denom: invalid request`, + }, + { + name: "invalid authority", + req: banktypes.NewMsgSetSendEnabled( + "invalid", + []*banktypes.SendEnabled{ + banktypes.NewSendEnabled("atom", true), + }, + []string{}, + ), + isExpErr: true, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + _, err := suite.msgServer.SetSendEnabled(suite.ctx, tc.req) + + if tc.isExpErr { + suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.errMsg) + } else { + suite.Require().NoError(err) + } + }) + } +} diff --git a/x/bank/keeper/msg_service_test.go b/x/bank/keeper/msg_service_test.go deleted file mode 100644 index a6454b6a85..0000000000 --- a/x/bank/keeper/msg_service_test.go +++ /dev/null @@ -1,172 +0,0 @@ -package keeper_test - -import ( - sdk "github.com/cosmos/cosmos-sdk/types" - banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" -) - -func (suite *KeeperTestSuite) TestMsgUpdateParams() { - // default params - params := banktypes.DefaultParams() - - testCases := []struct { - name string - input *banktypes.MsgUpdateParams - expErr bool - expErrMsg string - }{ - { - name: "invalid authority", - input: &banktypes.MsgUpdateParams{ - Authority: "invalid", - Params: params, - }, - expErr: true, - expErrMsg: "invalid authority", - }, - { - name: "send enabled param", - input: &banktypes.MsgUpdateParams{ - Authority: suite.bankKeeper.GetAuthority(), - Params: banktypes.Params{ - SendEnabled: []*banktypes.SendEnabled{ - {Denom: "foo", Enabled: true}, - }, - }, - }, - expErr: false, - }, - { - name: "all good", - input: &banktypes.MsgUpdateParams{ - Authority: suite.bankKeeper.GetAuthority(), - Params: params, - }, - expErr: false, - }, - } - - for _, tc := range testCases { - tc := tc - suite.Run(tc.name, func() { - _, err := suite.msgServer.UpdateParams(suite.ctx, tc.input) - - if tc.expErr { - suite.Require().Error(err) - suite.Require().Contains(err.Error(), tc.expErrMsg) - } else { - suite.Require().NoError(err) - } - }) - } -} - -func (suite *KeeperTestSuite) TestMsgSend() { - origCoins := sdk.NewCoins(sdk.NewInt64Coin("sendableCoin", 100)) - suite.bankKeeper.SetSendEnabled(suite.ctx, origCoins.Denoms()[0], true) - - testCases := []struct { - name string - input *banktypes.MsgSend - expErr bool - expErrMsg string - }{ - { - name: "invalid send to blocked address", - input: &banktypes.MsgSend{ - FromAddress: minterAcc.GetAddress().String(), - ToAddress: accAddrs[4].String(), - Amount: origCoins, - }, - expErr: true, - expErrMsg: "is not allowed to receive funds", - }, - { - name: "all good", - input: &banktypes.MsgSend{ - FromAddress: minterAcc.GetAddress().String(), - ToAddress: baseAcc.Address, - Amount: origCoins, - }, - expErr: false, - }, - } - - for _, tc := range testCases { - tc := tc - suite.Run(tc.name, func() { - suite.mockMintCoins(minterAcc) - suite.bankKeeper.MintCoins(suite.ctx, minterAcc.Name, origCoins) - if !tc.expErr { - suite.mockSendCoins(suite.ctx, minterAcc, baseAcc.GetAddress()) - } - _, err := suite.msgServer.Send(suite.ctx, tc.input) - if tc.expErr { - suite.Require().Error(err) - suite.Require().Contains(err.Error(), tc.expErrMsg) - } else { - suite.Require().NoError(err) - } - }) - } -} - -func (suite *KeeperTestSuite) TestMsgMultiSend() { - origDenom := "sendableCoin" - origCoins := sdk.NewCoins(sdk.NewInt64Coin(origDenom, 100)) - sendCoins := sdk.NewCoins(sdk.NewInt64Coin(origDenom, 50)) - suite.bankKeeper.SetSendEnabled(suite.ctx, origDenom, true) - - testCases := []struct { - name string - input *banktypes.MsgMultiSend - expErr bool - expErrMsg string - }{ - { - name: "invalid send to blocked address", - input: &banktypes.MsgMultiSend{ - Inputs: []banktypes.Input{ - {Address: minterAcc.GetAddress().String(), Coins: origCoins}, - }, - Outputs: []banktypes.Output{ - {Address: accAddrs[0].String(), Coins: sendCoins}, - {Address: accAddrs[4].String(), Coins: sendCoins}, - }, - }, - expErr: true, - expErrMsg: "is not allowed to receive funds", - }, - { - name: "invalid send to blocked address", - input: &banktypes.MsgMultiSend{ - Inputs: []banktypes.Input{ - {Address: minterAcc.GetAddress().String(), Coins: origCoins}, - }, - Outputs: []banktypes.Output{ - {Address: accAddrs[0].String(), Coins: sendCoins}, - {Address: accAddrs[1].String(), Coins: sendCoins}, - }, - }, - expErr: false, - }, - } - - for _, tc := range testCases { - tc := tc - suite.Run(tc.name, func() { - suite.mockMintCoins(minterAcc) - suite.bankKeeper.MintCoins(suite.ctx, minterAcc.Name, origCoins) - if !tc.expErr { - suite.mockInputOutputCoins([]sdk.AccountI{minterAcc}, accAddrs[:2]) - } - _, err := suite.msgServer.MultiSend(suite.ctx, tc.input) - if tc.expErr { - suite.Require().Error(err) - suite.Require().Contains(err.Error(), tc.expErrMsg) - } else { - suite.Require().NoError(err) - } - }) - } -} diff --git a/x/bank/types/inputs_outputs.go b/x/bank/types/inputs_outputs.go new file mode 100644 index 0000000000..cca4c03988 --- /dev/null +++ b/x/bank/types/inputs_outputs.go @@ -0,0 +1,83 @@ +package types + +import ( + errorsmod "cosmossdk.io/errors" + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// ValidateInputOutputs validates that each respective input and output is +// valid and that the sum of inputs is equal to the sum of outputs. +func ValidateInputOutputs(input Input, outputs []Output) error { + var totalIn, totalOut sdk.Coins + + if err := input.ValidateBasic(); err != nil { + return err + } + totalIn = input.Coins + + for _, out := range outputs { + if err := out.ValidateBasic(); err != nil { + return err + } + + totalOut = totalOut.Add(out.Coins...) + } + + // make sure inputs and outputs match + if !totalIn.Equal(totalOut) { + return ErrInputOutputMismatch + } + + return nil +} + +// ValidateBasic - validate transaction input +func (in Input) ValidateBasic() error { + if _, err := sdk.AccAddressFromBech32(in.Address); err != nil { + return sdkerrors.ErrInvalidAddress.Wrapf("invalid input address: %s", err) + } + + if !in.Coins.IsValid() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, in.Coins.String()) + } + + if !in.Coins.IsAllPositive() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, in.Coins.String()) + } + + return nil +} + +// NewInput - create a transaction input, used with MsgMultiSend +func NewInput(addr sdk.AccAddress, coins sdk.Coins) Input { + return Input{ + Address: addr.String(), + Coins: coins, + } +} + +// ValidateBasic - validate transaction output +func (out Output) ValidateBasic() error { + if _, err := sdk.AccAddressFromBech32(out.Address); err != nil { + return sdkerrors.ErrInvalidAddress.Wrapf("invalid output address: %s", err) + } + + if !out.Coins.IsValid() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, out.Coins.String()) + } + + if !out.Coins.IsAllPositive() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, out.Coins.String()) + } + + return nil +} + +// NewOutput - create a transaction output, used with MsgMultiSend +func NewOutput(addr sdk.AccAddress, coins sdk.Coins) Output { + return Output{ + Address: addr.String(), + Coins: coins, + } +} diff --git a/x/bank/types/msgs.go b/x/bank/types/msgs.go index 5f8453bff7..f3a70d6f3e 100644 --- a/x/bank/types/msgs.go +++ b/x/bank/types/msgs.go @@ -1,10 +1,7 @@ package types import ( - errorsmod "cosmossdk.io/errors" - sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" ) @@ -19,33 +16,10 @@ var ( ) // NewMsgSend - construct a msg to send coins from one account to another. -// - func NewMsgSend(fromAddr, toAddr sdk.AccAddress, amount sdk.Coins) *MsgSend { return &MsgSend{FromAddress: fromAddr.String(), ToAddress: toAddr.String(), Amount: amount} } -// ValidateBasic Implements Msg. -func (msg MsgSend) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.FromAddress); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid from address: %s", err) - } - - if _, err := sdk.AccAddressFromBech32(msg.ToAddress); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid to address: %s", err) - } - - if !msg.Amount.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String()) - } - - if !msg.Amount.IsAllPositive() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String()) - } - - return nil -} - // GetSignBytes Implements Msg. func (msg MsgSend) GetSignBytes() []byte { return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg)) @@ -62,26 +36,6 @@ func NewMsgMultiSend(in Input, out []Output) *MsgMultiSend { return &MsgMultiSend{Inputs: []Input{in}, Outputs: out} } -// ValidateBasic Implements Msg. -func (msg MsgMultiSend) ValidateBasic() error { - // this just makes sure the input and all the outputs are properly formatted, - // not that they actually have the money inside - - if len(msg.Inputs) == 0 { - return ErrNoInputs - } - - if len(msg.Inputs) != 1 { - return ErrMultipleSenders - } - - if len(msg.Outputs) == 0 { - return ErrNoOutputs - } - - return ValidateInputOutputs(msg.Inputs[0], msg.Outputs) -} - // GetSignBytes Implements Msg. func (msg MsgMultiSend) GetSignBytes() []byte { return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg)) @@ -98,82 +52,6 @@ func (msg MsgMultiSend) GetSigners() []sdk.AccAddress { return []sdk.AccAddress{addrs} } -// ValidateBasic - validate transaction input -func (in Input) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(in.Address); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid input address: %s", err) - } - - if !in.Coins.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, in.Coins.String()) - } - - if !in.Coins.IsAllPositive() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, in.Coins.String()) - } - - return nil -} - -// NewInput - create a transaction input, used with MsgMultiSend -func NewInput(addr sdk.AccAddress, coins sdk.Coins) Input { - return Input{ - Address: addr.String(), - Coins: coins, - } -} - -// ValidateBasic - validate transaction output -func (out Output) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(out.Address); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid output address: %s", err) - } - - if !out.Coins.IsValid() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, out.Coins.String()) - } - - if !out.Coins.IsAllPositive() { - return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, out.Coins.String()) - } - - return nil -} - -// NewOutput - create a transaction output, used with MsgMultiSend -func NewOutput(addr sdk.AccAddress, coins sdk.Coins) Output { - return Output{ - Address: addr.String(), - Coins: coins, - } -} - -// ValidateInputOutputs validates that each respective input and output is -// valid and that the sum of inputs is equal to the sum of outputs. -func ValidateInputOutputs(input Input, outputs []Output) error { - var totalIn, totalOut sdk.Coins - - if err := input.ValidateBasic(); err != nil { - return err - } - totalIn = input.Coins - - for _, out := range outputs { - if err := out.ValidateBasic(); err != nil { - return err - } - - totalOut = totalOut.Add(out.Coins...) - } - - // make sure inputs and outputs match - if !totalIn.Equal(totalOut) { - return ErrInputOutputMismatch - } - - return nil -} - // GetSigners returns the signer addresses that are expected to sign the result // of GetSignBytes. func (msg MsgUpdateParams) GetSigners() []sdk.AccAddress { @@ -188,11 +66,6 @@ func (msg MsgUpdateParams) GetSignBytes() []byte { return sdk.MustSortJSON(bz) } -// ValidateBasic performs basic MsgUpdateParams message validation. -func (msg MsgUpdateParams) ValidateBasic() error { - return msg.Params.Validate() -} - // NewMsgSetSendEnabled Construct a message to set one or more SendEnabled entries. func NewMsgSetSendEnabled(authority string, sendEnabled []*SendEnabled, useDefaultFor []string) *MsgSetSendEnabled { return &MsgSetSendEnabled{ @@ -212,33 +85,3 @@ func (msg MsgSetSendEnabled) GetSigners() []sdk.AccAddress { addr, _ := sdk.AccAddressFromBech32(msg.Authority) return []sdk.AccAddress{addr} } - -// ValidateBasic runs basic validation on this MsgSetSendEnabled. -func (msg MsgSetSendEnabled) ValidateBasic() error { - if len(msg.Authority) > 0 { - if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err) - } - } - - seen := map[string]bool{} - for _, se := range msg.SendEnabled { - if _, alreadySeen := seen[se.Denom]; alreadySeen { - return sdkerrors.ErrInvalidRequest.Wrapf("duplicate denom entries found for %q", se.Denom) - } - - seen[se.Denom] = true - - if err := se.Validate(); err != nil { - return sdkerrors.ErrInvalidRequest.Wrapf("invalid SendEnabled denom %q: %s", se.Denom, err) - } - } - - for _, denom := range msg.UseDefaultFor { - if err := sdk.ValidateDenom(denom); err != nil { - return sdkerrors.ErrInvalidRequest.Wrapf("invalid UseDefaultFor denom %q: %s", denom, err) - } - } - - return nil -} diff --git a/x/bank/types/msgs_test.go b/x/bank/types/msgs_test.go index b772c4924d..4f6462da26 100644 --- a/x/bank/types/msgs_test.go +++ b/x/bank/types/msgs_test.go @@ -3,7 +3,6 @@ package types import ( "testing" - sdkmath "cosmossdk.io/math" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -12,41 +11,6 @@ import ( govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" ) -func TestMsgSendValidation(t *testing.T) { - addr1 := sdk.AccAddress([]byte("from________________")) - addr2 := sdk.AccAddress([]byte("to__________________")) - addrEmpty := sdk.AccAddress([]byte("")) - addrLong := sdk.AccAddress([]byte("Purposefully long address")) - - atom123 := sdk.NewCoins(sdk.NewInt64Coin("atom", 123)) - atom0 := sdk.NewCoins(sdk.NewInt64Coin("atom", 0)) - atom123eth123 := sdk.NewCoins(sdk.NewInt64Coin("atom", 123), sdk.NewInt64Coin("eth", 123)) - atom123eth0 := sdk.Coins{sdk.NewInt64Coin("atom", 123), sdk.NewInt64Coin("eth", 0)} - - cases := []struct { - expectedErr string // empty means no error expected - msg *MsgSend - }{ - {"", NewMsgSend(addr1, addr2, atom123)}, // valid send - {"", NewMsgSend(addr1, addr2, atom123eth123)}, // valid send with multiple coins - {"", NewMsgSend(addrLong, addr2, atom123)}, // valid send with long addr sender - {"", NewMsgSend(addr1, addrLong, atom123)}, // valid send with long addr recipient - {": invalid coins", NewMsgSend(addr1, addr2, atom0)}, // non positive coin - {"123atom,0eth: invalid coins", NewMsgSend(addr1, addr2, atom123eth0)}, // non positive coin in multicoins - {"invalid from address: empty address string is not allowed: invalid address", NewMsgSend(addrEmpty, addr2, atom123)}, - {"invalid to address: empty address string is not allowed: invalid address", NewMsgSend(addr1, addrEmpty, atom123)}, - } - - for _, tc := range cases { - err := tc.msg.ValidateBasic() - if tc.expectedErr == "" { - require.Nil(t, err) - } else { - require.EqualError(t, err, tc.expectedErr) - } - } -} - func TestMsgSendGetSignBytes(t *testing.T) { addr1 := sdk.AccAddress([]byte("input")) addr2 := sdk.AccAddress([]byte("output")) @@ -140,93 +104,6 @@ func TestOutputValidation(t *testing.T) { } } -func TestMsgMultiSendValidation(t *testing.T) { - addr1 := sdk.AccAddress([]byte("_______alice________")) - addr2 := sdk.AccAddress([]byte("________bob_________")) - addr3 := sdk.AccAddress([]byte("_______addr3________")) - atom123 := sdk.NewCoins(sdk.NewInt64Coin("atom", 123)) - atom124 := sdk.NewCoins(sdk.NewInt64Coin("atom", 124)) - atom246 := sdk.NewCoins(sdk.NewInt64Coin("atom", 246)) - - input1 := NewInput(addr1, atom123) - input2 := NewInput(addr1, atom246) - output1 := NewOutput(addr2, atom123) - output2 := NewOutput(addr2, atom124) - output3 := NewOutput(addr2, atom123) - output4 := NewOutput(addr3, atom123) - - var emptyAddr sdk.AccAddress - - cases := []struct { - valid bool - tx MsgMultiSend - expErrMsg string - }{ - {false, MsgMultiSend{}, "no inputs to send transaction"}, // no input or output - {false, MsgMultiSend{Inputs: []Input{input1}}, "no outputs to send transaction"}, // just input - {false, MsgMultiSend{Outputs: []Output{output1}}, "no inputs to send transaction"}, // just output - { - false, - MsgMultiSend{ - Inputs: []Input{NewInput(emptyAddr, atom123)}, // invalid input - Outputs: []Output{output1}, - }, - "invalid input address", - }, - { - false, - MsgMultiSend{ - Inputs: []Input{input1}, - Outputs: []Output{{emptyAddr.String(), atom123}}, // invalid output - }, - "invalid output address", - }, - { - false, - MsgMultiSend{ - Inputs: []Input{input1}, - Outputs: []Output{output2}, // amounts don't match - }, - "sum inputs != sum outputs", - }, - { - true, - MsgMultiSend{ - Inputs: []Input{input1}, - Outputs: []Output{output1}, - }, - "", - }, - { - false, - MsgMultiSend{ - Inputs: []Input{input1, input2}, - Outputs: []Output{output3, output4}, - }, - "multiple senders not allowed", - }, - { - true, - MsgMultiSend{ - Inputs: []Input{NewInput(addr2, atom123.MulInt(sdkmath.NewInt(2)))}, - Outputs: []Output{output1, output1}, - }, - "", - }, - } - - for i, tc := range cases { - err := tc.tx.ValidateBasic() - if tc.valid { - require.Nil(t, err, "%d: %+v", i, err) - require.Nil(t, err) - } else { - require.NotNil(t, err, "%d", i) - require.Contains(t, err.Error(), tc.expErrMsg) - } - } -} - func TestMsgMultiSendGetSignBytes(t *testing.T) { addr1 := sdk.AccAddress([]byte("input")) addr2 := sdk.AccAddress([]byte("output")) @@ -287,100 +164,3 @@ func TestMsgSetSendEnabledGetSigners(t *testing.T) { actual := msg.GetSigners() assert.Equal(t, expected, actual) } - -func TestMsgSetSendEnabledValidateBasic(t *testing.T) { - govModuleAddr := authtypes.NewModuleAddress(govtypes.ModuleName).String() - tests := []struct { - name string - msg MsgSetSendEnabled - exp string - }{ - { - name: "valid with two entries", - msg: MsgSetSendEnabled{ - Authority: govModuleAddr, - SendEnabled: []*SendEnabled{ - {"somecoina", true}, - {"somecoinb", false}, - }, - UseDefaultFor: []string{"defcoinc", "defcoind"}, - }, - exp: "", - }, - { - name: "valid with two entries but no authority", - msg: MsgSetSendEnabled{ - Authority: "", - SendEnabled: []*SendEnabled{ - {"somecoina", true}, - {"somecoinb", false}, - }, - UseDefaultFor: []string{"defcoinc", "defcoind"}, - }, - exp: "", - }, - { - name: "bad authority", - msg: MsgSetSendEnabled{ - Authority: "farva", - SendEnabled: []*SendEnabled{ - {"somecoina", true}, - {"somecoinb", false}, - }, - }, - exp: "invalid authority address: decoding bech32 failed: invalid bech32 string length 5: invalid address", - }, - { - name: "bad first denom name", - msg: MsgSetSendEnabled{ - Authority: govModuleAddr, - SendEnabled: []*SendEnabled{ - {"Not A Denom", true}, - {"somecoinb", false}, - }, - }, - exp: `invalid SendEnabled denom "Not A Denom": invalid denom: Not A Denom: invalid request`, - }, - { - name: "bad second denom", - msg: MsgSetSendEnabled{ - Authority: govModuleAddr, - SendEnabled: []*SendEnabled{ - {"somecoina", true}, - {"", false}, - }, - }, - exp: `invalid SendEnabled denom "": invalid denom: : invalid request`, - }, - { - name: "duplicate denom", - msg: MsgSetSendEnabled{ - Authority: govModuleAddr, - SendEnabled: []*SendEnabled{ - {"copycoin", true}, - {"copycoin", false}, - }, - }, - exp: `duplicate denom entries found for "copycoin": invalid request`, - }, - { - name: "bad denom to delete", - msg: MsgSetSendEnabled{ - Authority: govModuleAddr, - UseDefaultFor: []string{"very \t bad denom string~~~!"}, - }, - exp: "invalid UseDefaultFor denom \"very \\t bad denom string~~~!\": invalid denom: very \t bad denom string~~~!: invalid request", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(tt *testing.T) { - actual := tc.msg.ValidateBasic() - if len(tc.exp) > 0 { - require.EqualError(tt, actual, tc.exp) - } else { - require.NoError(tt, actual) - } - }) - } -}