From 7c8236d2ed5752922004dbf8cbed403ea7d3eac3 Mon Sep 17 00:00:00 2001
From: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>
Date: Tue, 27 Jun 2023 19:22:23 +0530
Subject: [PATCH] refactor: x/bank audit changes (#16690)

---
 api/cosmos/bank/module/v1/module.pulsar.go |  2 +-
 api/cosmos/bank/v1beta1/query.pulsar.go    |  1 +
 api/cosmos/bank/v1beta1/tx.pulsar.go       |  1 +
 proto/cosmos/bank/module/v1/module.proto   |  2 +-
 proto/cosmos/bank/v1beta1/query.proto      |  1 +
 proto/cosmos/bank/v1beta1/tx.proto         |  1 +
 x/bank/client/cli/suite_test.go            |  2 +-
 x/bank/keeper/grpc_query_test.go           |  8 +--
 x/bank/keeper/invariants.go                |  4 ++
 x/bank/keeper/keeper_test.go               | 73 +++++++++++++++++++++-
 x/bank/types/msgs.go                       | 24 -------
 x/bank/types/msgs_test.go                  | 19 ------
 x/bank/types/query.pb.go                   |  1 +
 x/bank/types/tx.pb.go                      |  1 +
 14 files changed, 89 insertions(+), 51 deletions(-)

diff --git a/api/cosmos/bank/module/v1/module.pulsar.go b/api/cosmos/bank/module/v1/module.pulsar.go
index 64e252a4af..6d65ecfb24 100644
--- a/api/cosmos/bank/module/v1/module.pulsar.go
+++ b/api/cosmos/bank/module/v1/module.pulsar.go
@@ -576,7 +576,7 @@ type Module struct {
 	sizeCache     protoimpl.SizeCache
 	unknownFields protoimpl.UnknownFields
 
-	// blocked_module_accounts configures exceptional module accounts which should be blocked from receiving funds.
+	// blocked_module_accounts_override configures exceptional module accounts which should be blocked from receiving funds.
 	// If left empty it defaults to the list of account names supplied in the auth module configuration as
 	// module_account_permissions
 	BlockedModuleAccountsOverride []string `protobuf:"bytes,1,rep,name=blocked_module_accounts_override,json=blockedModuleAccountsOverride,proto3" json:"blocked_module_accounts_override,omitempty"`
diff --git a/api/cosmos/bank/v1beta1/query.pulsar.go b/api/cosmos/bank/v1beta1/query.pulsar.go
index 0442d8bc49..db041ff688 100644
--- a/api/cosmos/bank/v1beta1/query.pulsar.go
+++ b/api/cosmos/bank/v1beta1/query.pulsar.go
@@ -11862,6 +11862,7 @@ type QueryParamsResponse struct {
 	sizeCache     protoimpl.SizeCache
 	unknownFields protoimpl.UnknownFields
 
+	// params provides the parameters of the bank module.
 	Params *Params `protobuf:"bytes,1,opt,name=params,proto3" json:"params,omitempty"`
 }
 
diff --git a/api/cosmos/bank/v1beta1/tx.pulsar.go b/api/cosmos/bank/v1beta1/tx.pulsar.go
index 952e29e2d0..64c6508750 100644
--- a/api/cosmos/bank/v1beta1/tx.pulsar.go
+++ b/api/cosmos/bank/v1beta1/tx.pulsar.go
@@ -4133,6 +4133,7 @@ type MsgSetSendEnabled struct {
 	sizeCache     protoimpl.SizeCache
 	unknownFields protoimpl.UnknownFields
 
+	// authority is the address that controls the module.
 	Authority string `protobuf:"bytes,1,opt,name=authority,proto3" json:"authority,omitempty"`
 	// send_enabled is the list of entries to add or update.
 	SendEnabled []*SendEnabled `protobuf:"bytes,2,rep,name=send_enabled,json=sendEnabled,proto3" json:"send_enabled,omitempty"`
diff --git a/proto/cosmos/bank/module/v1/module.proto b/proto/cosmos/bank/module/v1/module.proto
index 51e3158b68..eb26a80c9d 100644
--- a/proto/cosmos/bank/module/v1/module.proto
+++ b/proto/cosmos/bank/module/v1/module.proto
@@ -10,7 +10,7 @@ message Module {
     go_import: "github.com/cosmos/cosmos-sdk/x/bank"
   };
 
-  // blocked_module_accounts configures exceptional module accounts which should be blocked from receiving funds.
+  // blocked_module_accounts_override configures exceptional module accounts which should be blocked from receiving funds.
   // If left empty it defaults to the list of account names supplied in the auth module configuration as
   // module_account_permissions
   repeated string blocked_module_accounts_override = 1;
diff --git a/proto/cosmos/bank/v1beta1/query.proto b/proto/cosmos/bank/v1beta1/query.proto
index 9aed764777..52fe779f1e 100644
--- a/proto/cosmos/bank/v1beta1/query.proto
+++ b/proto/cosmos/bank/v1beta1/query.proto
@@ -267,6 +267,7 @@ message QueryParamsRequest {}
 
 // QueryParamsResponse defines the response type for querying x/bank parameters.
 message QueryParamsResponse {
+  // params provides the parameters of the bank module.
   Params params = 1 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
 }
 
diff --git a/proto/cosmos/bank/v1beta1/tx.proto b/proto/cosmos/bank/v1beta1/tx.proto
index c780be3786..a4e8fae41f 100644
--- a/proto/cosmos/bank/v1beta1/tx.proto
+++ b/proto/cosmos/bank/v1beta1/tx.proto
@@ -105,6 +105,7 @@ message MsgSetSendEnabled {
   option (cosmos.msg.v1.signer) = "authority";
   option (amino.name)           = "cosmos-sdk/MsgSetSendEnabled";
 
+  // authority is the address that controls the module.
   string authority = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
 
   // send_enabled is the list of entries to add or update.
diff --git a/x/bank/client/cli/suite_test.go b/x/bank/client/cli/suite_test.go
index cf68074bf5..9ba83b31b6 100644
--- a/x/bank/client/cli/suite_test.go
+++ b/x/bank/client/cli/suite_test.go
@@ -22,7 +22,7 @@ type CLITestSuite struct {
 	baseCtx client.Context
 }
 
-func TestMigrateTestSuite(t *testing.T) {
+func TestCLITestSuite(t *testing.T) {
 	suite.Run(t, new(CLITestSuite))
 }
 
diff --git a/x/bank/keeper/grpc_query_test.go b/x/bank/keeper/grpc_query_test.go
index 42eaaae718..2bb6bbc3e3 100644
--- a/x/bank/keeper/grpc_query_test.go
+++ b/x/bank/keeper/grpc_query_test.go
@@ -274,10 +274,10 @@ func (suite *KeeperTestSuite) TestQueryParams() {
 	suite.Require().Equal(suite.bankKeeper.GetParams(suite.ctx), res.GetParams())
 }
 
-func (suite *KeeperTestSuite) QueryDenomsMetadataRequest() {
+func (suite *KeeperTestSuite) TestQueryDenomsMetadataRequest() {
 	var (
 		req         *types.QueryDenomsMetadataRequest
-		expMetadata = []types.Metadata{}
+		expMetadata = []types.Metadata(nil)
 	)
 
 	testCases := []struct {
@@ -376,7 +376,7 @@ func (suite *KeeperTestSuite) QueryDenomsMetadataRequest() {
 	}
 }
 
-func (suite *KeeperTestSuite) QueryDenomMetadataRequest() {
+func (suite *KeeperTestSuite) TestQueryDenomMetadataRequest() {
 	var (
 		req         *types.QueryDenomMetadataRequest
 		expMetadata = types.Metadata{}
@@ -406,7 +406,7 @@ func (suite *KeeperTestSuite) QueryDenomMetadataRequest() {
 		{
 			"success",
 			func() {
-				expMetadata := types.Metadata{
+				expMetadata = types.Metadata{
 					Description: "The native staking token of the Cosmos Hub.",
 					DenomUnits: []*types.DenomUnit{
 						{
diff --git a/x/bank/keeper/invariants.go b/x/bank/keeper/invariants.go
index 72240a1ac3..69ee798a1d 100644
--- a/x/bank/keeper/invariants.go
+++ b/x/bank/keeper/invariants.go
@@ -17,6 +17,10 @@ func RegisterInvariants(ir sdk.InvariantRegistry, k Keeper) {
 // AllInvariants runs all invariants of the X/bank module.
 func AllInvariants(k Keeper) sdk.Invariant {
 	return func(ctx sdk.Context) (string, bool) {
+		res, stop := NonnegativeBalanceInvariant(k)(ctx)
+		if stop {
+			return res, stop
+		}
 		return TotalSupply(k)(ctx)
 	}
 }
diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go
index c2b0722052..425c1a3a6f 100644
--- a/x/bank/keeper/keeper_test.go
+++ b/x/bank/keeper/keeper_test.go
@@ -48,7 +48,7 @@ const (
 
 var (
 	holderAcc    = authtypes.NewEmptyModuleAccount(holder)
-	burnerAcc    = authtypes.NewEmptyModuleAccount(authtypes.Burner, authtypes.Burner)
+	burnerAcc    = authtypes.NewEmptyModuleAccount(authtypes.Burner, authtypes.Burner, authtypes.Staking)
 	minterAcc    = authtypes.NewEmptyModuleAccount(authtypes.Minter, authtypes.Minter)
 	mintAcc      = authtypes.NewEmptyModuleAccount(banktypes.MintModuleName, authtypes.Minter)
 	multiPermAcc = authtypes.NewEmptyModuleAccount(multiPerm, authtypes.Burner, authtypes.Minter, authtypes.Staking)
@@ -218,6 +218,16 @@ func (suite *KeeperTestSuite) mockSpendableCoins(ctx sdk.Context, acc sdk.Accoun
 	suite.authKeeper.EXPECT().GetAccount(ctx, acc.GetAddress()).Return(acc)
 }
 
+func (suite *KeeperTestSuite) mockDelegateCoinsFromAccountToModule(acc *authtypes.BaseAccount, moduleAcc *authtypes.ModuleAccount) {
+	suite.authKeeper.EXPECT().GetModuleAccount(suite.ctx, moduleAcc.Name).Return(moduleAcc)
+	suite.mockDelegateCoins(suite.ctx, acc, moduleAcc)
+}
+
+func (suite *KeeperTestSuite) mockUndelegateCoinsFromModuleToAccount(moduleAcc *authtypes.ModuleAccount, accAddr *authtypes.BaseAccount) {
+	suite.authKeeper.EXPECT().GetModuleAccount(suite.ctx, moduleAcc.Name).Return(moduleAcc)
+	suite.mockUnDelegateCoins(suite.ctx, accAddr, moduleAcc)
+}
+
 func (suite *KeeperTestSuite) mockDelegateCoins(ctx context.Context, acc, mAcc sdk.AccountI) {
 	vacc, ok := acc.(banktypes.VestingAccount)
 	if ok {
@@ -314,6 +324,67 @@ func (suite *KeeperTestSuite) TestSendCoinsFromModuleToAccount_Blocklist() {
 	))
 }
 
+func (suite *KeeperTestSuite) TestSupply_DelegateUndelegateCoins() {
+	ctx := suite.ctx
+	require := suite.Require()
+	authKeeper, keeper := suite.authKeeper, suite.bankKeeper
+
+	// set initial balances
+	suite.mockMintCoins(mintAcc)
+	require.NoError(keeper.MintCoins(ctx, banktypes.MintModuleName, initCoins))
+
+	suite.mockSendCoinsFromModuleToAccount(mintAcc, holderAcc.GetAddress())
+	require.NoError(keeper.SendCoinsFromModuleToAccount(ctx, banktypes.MintModuleName, holderAcc.GetAddress(), initCoins))
+
+	authKeeper.EXPECT().GetModuleAddress("").Return(nil)
+	require.Panics(func() {
+		_ = keeper.SendCoinsFromModuleToAccount(ctx, "", holderAcc.GetAddress(), initCoins) //nolint:errcheck // we're testing for a panic, not an error
+	})
+
+	authKeeper.EXPECT().GetModuleAddress(burnerAcc.Name).Return(burnerAcc.GetAddress())
+	authKeeper.EXPECT().GetModuleAccount(ctx, "").Return(nil)
+	require.Panics(func() {
+		_ = keeper.SendCoinsFromModuleToModule(ctx, authtypes.Burner, "", initCoins) //nolint:errcheck // we're testing for a panic, not an error
+	})
+
+	authKeeper.EXPECT().GetModuleAddress("").Return(nil)
+	require.Panics(func() {
+		_ = keeper.SendCoinsFromModuleToAccount(ctx, "", baseAcc.GetAddress(), initCoins) //nolint:errcheck // we're testing for a panic, not an error
+	})
+
+	authKeeper.EXPECT().GetModuleAddress(holderAcc.Name).Return(holderAcc.GetAddress())
+	authKeeper.EXPECT().GetAccount(suite.ctx, holderAcc.GetAddress()).Return(holderAcc)
+	require.Error(
+		keeper.SendCoinsFromModuleToAccount(ctx, holderAcc.GetName(), baseAcc.GetAddress(), initCoins.Add(initCoins...)),
+	)
+	suite.mockSendCoinsFromModuleToModule(holderAcc, burnerAcc)
+	require.NoError(
+		keeper.SendCoinsFromModuleToModule(ctx, holderAcc.GetName(), authtypes.Burner, initCoins),
+	)
+
+	require.Equal(sdk.NewCoins(), keeper.GetAllBalances(ctx, holderAcc.GetAddress()))
+	require.Equal(initCoins, keeper.GetAllBalances(ctx, burnerAcc.GetAddress()))
+
+	suite.mockSendCoinsFromModuleToAccount(burnerAcc, baseAcc.GetAddress())
+	require.NoError(
+		keeper.SendCoinsFromModuleToAccount(ctx, authtypes.Burner, baseAcc.GetAddress(), initCoins),
+	)
+	require.Equal(sdk.NewCoins(), keeper.GetAllBalances(ctx, burnerAcc.GetAddress()))
+	require.Equal(initCoins, keeper.GetAllBalances(ctx, baseAcc.GetAddress()))
+
+	suite.mockDelegateCoinsFromAccountToModule(baseAcc, burnerAcc)
+
+	sdkCtx := sdk.UnwrapSDKContext(ctx)
+	require.NoError(keeper.DelegateCoinsFromAccountToModule(sdkCtx, baseAcc.GetAddress(), authtypes.Burner, initCoins))
+	require.Equal(sdk.NewCoins(), keeper.GetAllBalances(ctx, baseAcc.GetAddress()))
+	require.Equal(initCoins, keeper.GetAllBalances(ctx, burnerAcc.GetAddress()))
+
+	suite.mockUndelegateCoinsFromModuleToAccount(burnerAcc, baseAcc)
+	require.NoError(keeper.UndelegateCoinsFromModuleToAccount(sdkCtx, authtypes.Burner, baseAcc.GetAddress(), initCoins))
+	require.Equal(sdk.NewCoins(), keeper.GetAllBalances(ctx, burnerAcc.GetAddress()))
+	require.Equal(initCoins, keeper.GetAllBalances(ctx, baseAcc.GetAddress()))
+}
+
 func (suite *KeeperTestSuite) TestSupply_SendCoins() {
 	ctx := suite.ctx
 	require := suite.Require()
diff --git a/x/bank/types/msgs.go b/x/bank/types/msgs.go
index a020fdd47e..9ce68e4834 100644
--- a/x/bank/types/msgs.go
+++ b/x/bank/types/msgs.go
@@ -26,24 +26,6 @@ func NewMsgMultiSend(in Input, out []Output) *MsgMultiSend {
 	return &MsgMultiSend{Inputs: []Input{in}, Outputs: out}
 }
 
-// GetSigners Implements Msg.
-func (msg MsgMultiSend) GetSigners() []sdk.AccAddress {
-	// should not happen as ValidateBasic would have failed
-	if len(msg.Inputs) == 0 {
-		return nil
-	}
-
-	addrs, _ := sdk.AccAddressFromBech32(msg.Inputs[0].Address)
-	return []sdk.AccAddress{addrs}
-}
-
-// GetSigners returns the signer addresses that are expected to sign the result
-// of GetSignBytes.
-func (msg MsgUpdateParams) GetSigners() []sdk.AccAddress {
-	authority, _ := sdk.AccAddressFromBech32(msg.Authority)
-	return []sdk.AccAddress{authority}
-}
-
 // NewMsgSetSendEnabled Construct a message to set one or more SendEnabled entries.
 func NewMsgSetSendEnabled(authority string, sendEnabled []*SendEnabled, useDefaultFor []string) *MsgSetSendEnabled {
 	return &MsgSetSendEnabled{
@@ -52,9 +34,3 @@ func NewMsgSetSendEnabled(authority string, sendEnabled []*SendEnabled, useDefau
 		UseDefaultFor: useDefaultFor,
 	}
 }
-
-// GetSigners returns the expected signers for MsgSoftwareUpgrade.
-func (msg MsgSetSendEnabled) GetSigners() []sdk.AccAddress {
-	addr, _ := sdk.AccAddressFromBech32(msg.Authority)
-	return []sdk.AccAddress{addr}
-}
diff --git a/x/bank/types/msgs_test.go b/x/bank/types/msgs_test.go
index b7f98f08b5..2b2b6d35e5 100644
--- a/x/bank/types/msgs_test.go
+++ b/x/bank/types/msgs_test.go
@@ -9,7 +9,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/codec/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 )
 
 func TestMsgSendGetSignBytes(t *testing.T) {
@@ -121,16 +120,6 @@ func TestMsgMultiSendGetSignBytes(t *testing.T) {
 	require.Equal(t, expected, string(res))
 }
 
-func TestMsgMultiSendGetSigners(t *testing.T) {
-	addr := sdk.AccAddress([]byte("input111111111111111"))
-	input := NewInput(addr, nil)
-	msg := NewMsgMultiSend(input, nil)
-
-	res := msg.GetSigners()
-	require.Equal(t, 1, len(res))
-	require.True(t, addr.Equals(res[0]))
-}
-
 func TestNewMsgSetSendEnabled(t *testing.T) {
 	// Punt. Just setting one to all non-default values and making sure they're as expected.
 	msg := NewMsgSetSendEnabled("milton", []*SendEnabled{{"barrycoin", true}}, []string{"billcoin"})
@@ -160,11 +149,3 @@ func TestMsgSetSendEnabledGetSignBytes(t *testing.T) {
 	actual := string(actualBz)
 	assert.Equal(t, expected, actual)
 }
-
-func TestMsgSetSendEnabledGetSigners(t *testing.T) {
-	govModuleAddr := authtypes.NewModuleAddress(GovModuleName)
-	msg := NewMsgSetSendEnabled(govModuleAddr.String(), nil, nil)
-	expected := []sdk.AccAddress{govModuleAddr}
-	actual := msg.GetSigners()
-	assert.Equal(t, expected, actual)
-}
diff --git a/x/bank/types/query.pb.go b/x/bank/types/query.pb.go
index 5c040434c9..7947c9501b 100644
--- a/x/bank/types/query.pb.go
+++ b/x/bank/types/query.pb.go
@@ -648,6 +648,7 @@ var xxx_messageInfo_QueryParamsRequest proto.InternalMessageInfo
 
 // QueryParamsResponse defines the response type for querying x/bank parameters.
 type QueryParamsResponse struct {
+	// params provides the parameters of the bank module.
 	Params Params `protobuf:"bytes,1,opt,name=params,proto3" json:"params"`
 }
 
diff --git a/x/bank/types/tx.pb.go b/x/bank/types/tx.pb.go
index 12b6b19611..74171878e6 100644
--- a/x/bank/types/tx.pb.go
+++ b/x/bank/types/tx.pb.go
@@ -309,6 +309,7 @@ var xxx_messageInfo_MsgUpdateParamsResponse proto.InternalMessageInfo
 //
 // Since: cosmos-sdk 0.47
 type MsgSetSendEnabled struct {
+	// authority is the address that controls the module.
 	Authority string `protobuf:"bytes,1,opt,name=authority,proto3" json:"authority,omitempty"`
 	// send_enabled is the list of entries to add or update.
 	SendEnabled []*SendEnabled `protobuf:"bytes,2,rep,name=send_enabled,json=sendEnabled,proto3" json:"send_enabled,omitempty"`