From adcf958f34e9b5b24d9e2e68b88758d04d5dcfd5 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 6 Aug 2024 21:19:35 +0200 Subject: [PATCH] refactor(bank): audit/QA changes (backport #21048) (#21187) Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> --- CHANGELOG.md | 1 - tests/integration/bank/app_test.go | 46 ++++++++++++++++- x/bank/CHANGELOG.md | 1 + x/bank/README.md | 2 +- x/bank/autocli.go | 2 +- x/bank/keeper/keeper.go | 5 +- x/bank/keeper/keeper_test.go | 3 ++ x/bank/keeper/msg_server.go | 4 +- x/bank/types/inputs_outputs_test.go | 77 +++++++++++++++++++++++++++++ x/bank/types/msgs.go | 6 +-- 10 files changed, 136 insertions(+), 11 deletions(-) create mode 100644 x/bank/types/inputs_outputs_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 198363d6b8..05da226451 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,7 +67,6 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i ### Improvements * (codec) [#20122](https://github.com/cosmos/cosmos-sdk/pull/20122) Added a cache to address codec. -* (bank) [#20354](https://github.com/cosmos/cosmos-sdk/pull/20354) Reduce the number of `ValidateDenom` calls in `bank.SendCoins`. * (types) [#19869](https://github.com/cosmos/cosmos-sdk/pull/19869) Removed `Any` type from `codec/types` and replaced it with an alias for `cosmos/gogoproto/types/any`. * (server) [#19854](https://github.com/cosmos/cosmos-sdk/pull/19854) Add customizability to start command. * Add `StartCmdOptions` in `server.AddCommands` instead of `servertypes.ModuleInitFlags`. To set custom flags set them in the `StartCmdOptions` struct on the `AddFlags` field. diff --git a/tests/integration/bank/app_test.go b/tests/integration/bank/app_test.go index bb2c65d271..b4478a3e1f 100644 --- a/tests/integration/bank/app_test.go +++ b/tests/integration/bank/app_test.go @@ -118,7 +118,7 @@ func checkBalance(t *testing.T, baseApp *baseapp.BaseApp, addr sdk.AccAddress, b t.Helper() ctxCheck := baseApp.NewContext(true) keeperBalances := keeper.GetAllBalances(ctxCheck, addr) - require.True(t, balances.Equal(keeperBalances)) + require.True(t, balances.Equal(keeperBalances), balances.String(), keeperBalances.String()) } func TestSendNotEnoughBalance(t *testing.T) { @@ -477,3 +477,47 @@ func TestMsgSetSendEnabled(t *testing.T) { }) } } + +// TestSendToNonExistingAccount tests sending coins to an account that does not exist, and this account +// must not be created. +func TestSendToNonExistingAccount(t *testing.T) { + acc1 := authtypes.NewBaseAccountWithAddress(addr1) + genAccs := []authtypes.GenesisAccount{acc1} + s := createTestSuite(t, genAccs) + baseApp := s.App.BaseApp + ctx := baseApp.NewContext(false) + + require.NoError(t, testutil.FundAccount(ctx, s.BankKeeper, addr1, sdk.NewCoins(sdk.NewInt64Coin("foocoin", 42)))) + _, err := baseApp.FinalizeBlock(&abci.FinalizeBlockRequest{Height: baseApp.LastBlockHeight() + 1}) + require.NoError(t, err) + _, err = baseApp.Commit() + require.NoError(t, err) + + addr2Str, err := s.AccountKeeper.AddressCodec().BytesToString(addr2) + require.NoError(t, err) + sendMsg := types.NewMsgSend(addr1.String(), addr2Str, coins) + h := header.Info{Height: baseApp.LastBlockHeight() + 1} + txConfig := moduletestutil.MakeTestTxConfig(cdctestutil.CodecOptions{}) + _, _, err = simtestutil.SignCheckDeliver(t, txConfig, baseApp, h, []sdk.Msg{sendMsg}, "", []uint64{0}, []uint64{0}, true, true, priv1) + require.NoError(t, err) + + // Check that the account was not created + acc2 := s.AccountKeeper.GetAccount(baseApp.NewContext(true), addr2) + require.Nil(t, acc2) + + // But it does have a balance + checkBalance(t, baseApp, addr2, coins, s.BankKeeper) + + // Now we send coins back and the account should be created + sendMsg = types.NewMsgSend(addr2Str, addr1.String(), coins) + h = header.Info{Height: baseApp.LastBlockHeight() + 1} + _, _, err = simtestutil.SignCheckDeliver(t, txConfig, baseApp, h, []sdk.Msg{sendMsg}, "", []uint64{0}, []uint64{0}, true, true, priv2) + require.NoError(t, err) + + // Balance has been reduced + checkBalance(t, baseApp, addr2, sdk.NewCoins(), s.BankKeeper) + + // Check that the account was created + acc2 = s.AccountKeeper.GetAccount(baseApp.NewContext(true), addr2) + require.NotNil(t, acc2, "account should have been created %s", addr2.String()) +} diff --git a/x/bank/CHANGELOG.md b/x/bank/CHANGELOG.md index 22f1637fbf..2531ccc2bb 100644 --- a/x/bank/CHANGELOG.md +++ b/x/bank/CHANGELOG.md @@ -34,6 +34,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. * [#20517](https://github.com/cosmos/cosmos-sdk/pull/20517) `SendCoins` now checks for `SendRestrictions` before instead of after deducting coins using `subUnlockedCoins`. +* [#20354](https://github.com/cosmos/cosmos-sdk/pull/20354) Reduce the number of `ValidateDenom` calls in `bank.SendCoins`. ### API Breaking Changes diff --git a/x/bank/README.md b/x/bank/README.md index 8bd981a9a7..5ac389feb5 100644 --- a/x/bank/README.md +++ b/x/bank/README.md @@ -125,7 +125,7 @@ aforementioned state: ## Params -The bank module stores it's params in state with the prefix of `0x05`, +The bank module stores its params in state with the prefix of `0x05`, it can be updated with governance or the address with authority. * Params: `0x05 | ProtocolBuffer(Params)` diff --git a/x/bank/autocli.go b/x/bank/autocli.go index 4b4c3529f2..ead3aa80d8 100644 --- a/x/bank/autocli.go +++ b/x/bank/autocli.go @@ -95,7 +95,7 @@ To look up all denoms, do not provide any arguments.`, RpcCommandOptions: []*autocliv1.RpcCommandOptions{ { RpcMethod: "Send", - Use: "send [from_key_or_address] [to_address] [amount]", + Use: "send [from_key_or_address] [to_address] [amount ...]", Short: "Send funds from one account to another.", Long: `Send funds from one account to another. Note, the '--from' flag is ignored as it is implied from [from_key_or_address]. diff --git a/x/bank/keeper/keeper.go b/x/bank/keeper/keeper.go index 03f01765c6..fee164f6d9 100644 --- a/x/bank/keeper/keeper.go +++ b/x/bank/keeper/keeper.go @@ -414,12 +414,13 @@ func (k BaseKeeper) BurnCoins(ctx context.Context, address []byte, amounts sdk.C k.setSupply(ctx, supply) } - k.Logger.Debug("burned tokens from account", "amount", amounts.String(), "from", address) - addrStr, err := k.ak.AddressCodec().BytesToString(acc.GetAddress()) if err != nil { return err } + + k.Logger.Debug("burned tokens from account", "amount", amounts.String(), "from", addrStr) + // emit burn event return k.EventService.EventManager(ctx).EmitKV( types.EventTypeCoinBurn, diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index 6db7c0a7b9..2c4eb43ff3 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -579,6 +579,9 @@ func (suite *KeeperTestSuite) TestSupply_BurnCoins() { suite.mockBurnCoins(burnerAcc) require.NoError(keeper.BurnCoins(ctx, burnerAcc.GetAddress(), initCoins)) + suite.mockBurnCoins(burnerAcc) + require.ErrorContains(keeper.BurnCoins(ctx, burnerAcc.GetAddress(), sdk.Coins{sdk.Coin{Denom: "asd", Amount: math.NewInt(-1)}}), "-1asd: invalid coins") + supplyAfterBurn, _, err := keeper.GetPaginatedTotalSupply(ctx, &query.PageRequest{}) require.NoError(err) require.Equal(sdk.NewCoins(), keeper.GetAllBalances(ctx, burnerAcc.GetAddress())) diff --git a/x/bank/keeper/msg_server.go b/x/bank/keeper/msg_server.go index 548d4fdd11..5e7b94e13c 100644 --- a/x/bank/keeper/msg_server.go +++ b/x/bank/keeper/msg_server.go @@ -177,7 +177,7 @@ func (k msgServer) SetSendEnabled(ctx context.Context, msg *types.MsgSetSendEnab return &types.MsgSetSendEnabledResponse{}, nil } -func (k msgServer) Burn(goCtx context.Context, msg *types.MsgBurn) (*types.MsgBurnResponse, error) { +func (k msgServer) Burn(ctx context.Context, msg *types.MsgBurn) (*types.MsgBurnResponse, error) { var ( from []byte err error @@ -205,7 +205,7 @@ func (k msgServer) Burn(goCtx context.Context, msg *types.MsgBurn) (*types.MsgBu return nil, errorsmod.Wrap(sdkerrors.ErrInvalidCoins, coins.String()) } - err = k.BurnCoins(goCtx, from, coins) + err = k.BurnCoins(ctx, from, coins) if err != nil { return nil, err } diff --git a/x/bank/types/inputs_outputs_test.go b/x/bank/types/inputs_outputs_test.go new file mode 100644 index 0000000000..18bdd31f81 --- /dev/null +++ b/x/bank/types/inputs_outputs_test.go @@ -0,0 +1,77 @@ +package types_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "cosmossdk.io/math" + "cosmossdk.io/x/bank/types" + + sdk "github.com/cosmos/cosmos-sdk/types" +) + +func TestValidateInputOutputs(t *testing.T) { + tests := []struct { + name string + input types.Input + outputs []types.Output + expErr bool + }{ + { + "valid input and outputs", + types.NewInput("cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", sdk.Coins{sdk.NewInt64Coin("uatom", 1)}), + []types.Output{ + { + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{sdk.NewInt64Coin("uatom", 1)}, + }, + }, + false, + }, + { + "empty input", + types.Input{}, + []types.Output{ + { + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{sdk.NewInt64Coin("uatom", 1)}, + }, + }, + true, + }, + { + "input and output mismatch", + types.NewInput("cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", sdk.Coins{sdk.NewInt64Coin("uatom", 1)}), + []types.Output{ + { + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{sdk.NewInt64Coin("uatom", 2)}, + }, + }, + true, + }, + { + "invalid output", + types.NewInput("cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", sdk.Coins{sdk.NewInt64Coin("uatom", 1)}), + []types.Output{ + { + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{sdk.Coin{Denom: "", Amount: math.NewInt(1)}}, + }, + }, + true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := types.ValidateInputOutputs(tc.input, tc.outputs) + if tc.expErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/x/bank/types/msgs.go b/x/bank/types/msgs.go index e24487aa33..c91d7ab28c 100644 --- a/x/bank/types/msgs.go +++ b/x/bank/types/msgs.go @@ -12,17 +12,17 @@ var ( _ coretransaction.Msg = &MsgUpdateParams{} ) -// NewMsgSend - construct a msg to send coins from one account to another. +// NewMsgSend constructs a msg to send coins from one account to another. func NewMsgSend(fromAddr, toAddr string, amount sdk.Coins) *MsgSend { return &MsgSend{FromAddress: fromAddr, ToAddress: toAddr, Amount: amount} } -// NewMsgMultiSend - construct arbitrary multi-in, multi-out send msg. +// NewMsgMultiSend constructs an arbitrary multi-in, multi-out send msg. func NewMsgMultiSend(in Input, out []Output) *MsgMultiSend { return &MsgMultiSend{Inputs: []Input{in}, Outputs: out} } -// NewMsgSetSendEnabled Construct a message to set one or more SendEnabled entries. +// NewMsgSetSendEnabled constructs a message to set one or more SendEnabled entries. func NewMsgSetSendEnabled(authority string, sendEnabled []*SendEnabled, useDefaultFor []string) *MsgSetSendEnabled { return &MsgSetSendEnabled{ Authority: authority,