From 18b0fa05070c6800d84e452c8e209f25a1cf5ba2 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 9 Dec 2022 11:10:21 -0500 Subject: [PATCH] refactor: audit x/bank package changes in 0.47 release (#14076) --- x/bank/keeper/grpc_query.go | 9 ++- x/bank/keeper/keeper_test.go | 80 ++++++++++++++--------- x/bank/keeper/send.go | 101 ++++++++++++++++------------- x/bank/module.go | 7 +- x/bank/types/genesis.go | 19 ++++-- x/bank/types/keys.go | 20 ------ x/bank/types/keys_test.go | 54 --------------- x/bank/types/msgs.go | 10 ++- x/bank/types/send_authorization.go | 21 +++--- 9 files changed, 148 insertions(+), 173 deletions(-) diff --git a/x/bank/keeper/grpc_query.go b/x/bank/keeper/grpc_query.go index 60d0be75c630..f1c5b3081bb7 100644 --- a/x/bank/keeper/grpc_query.go +++ b/x/bank/keeper/grpc_query.go @@ -4,7 +4,7 @@ import ( "context" "cosmossdk.io/math" - + gogotypes "github.com/cosmos/gogoproto/types" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -262,12 +262,16 @@ func (k BaseKeeper) SendEnabled(goCtx context.Context, req *types.QuerySendEnabl } else { store := k.getSendEnabledPrefixStore(ctx) var err error + resp.Pagination, err = query.FilteredPaginate( store, req.Pagination, func(key []byte, value []byte, accumulate bool) (bool, error) { if accumulate { - resp.SendEnabled = append(resp.SendEnabled, types.NewSendEnabled(string(key), types.IsTrueB(value))) + var enabled gogotypes.BoolValue + k.cdc.MustUnmarshal(value, &enabled) + + resp.SendEnabled = append(resp.SendEnabled, types.NewSendEnabled(string(key), enabled.Value)) } return true, nil }, @@ -276,5 +280,6 @@ func (k BaseKeeper) SendEnabled(goCtx context.Context, req *types.QuerySendEnabl return nil, status.Error(codes.Internal, err.Error()) } } + return resp, nil } diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index 45746bd39ecb..977903128898 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -8,9 +8,7 @@ import ( "cosmossdk.io/math" "github.com/golang/mock/gomock" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" - abci "github.com/tendermint/tendermint/abci/types" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" tmtime "github.com/tendermint/tendermint/types/time" @@ -20,13 +18,11 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" "github.com/cosmos/cosmos-sdk/types/query" - authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" vesting "github.com/cosmos/cosmos-sdk/x/auth/vesting/types" "github.com/cosmos/cosmos-sdk/x/bank/exported" "github.com/cosmos/cosmos-sdk/x/bank/keeper" banktestutil "github.com/cosmos/cosmos-sdk/x/bank/testutil" - banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" @@ -84,6 +80,10 @@ type KeeperTestSuite struct { encCfg moduletestutil.TestEncodingConfig } +func TestKeeperTestSuite(t *testing.T) { + suite.Run(t, new(KeeperTestSuite)) +} + func (suite *KeeperTestSuite) SetupTest() { key := sdk.NewKVStoreKey(banktypes.StoreKey) testCtx := testutil.DefaultContextWithDB(suite.T(), key, sdk.NewTransientStoreKey("transient_test")) @@ -216,7 +216,7 @@ func (suite *KeeperTestSuite) TestGetAuthority() { suite.T().Run(name, func(t *testing.T) { kpr := NewKeeperWithAuthority(expected) actual := kpr.GetAuthority() - assert.Equal(t, expected, actual) + suite.Require().Equal(expected, actual) }) } } @@ -1367,6 +1367,7 @@ func (suite *KeeperTestSuite) TestIsSendEnabledDenom() { for _, def := range []bool{true, false} { params := banktypes.Params{DefaultSendEnabled: def} require.NoError(bankKeeper.SetParams(ctx, params)) + for _, tc := range tests { suite.T().Run(fmt.Sprintf("%s default %t", tc.denom, def), func(t *testing.T) { actual := suite.bankKeeper.IsSendEnabledDenom(suite.ctx, tc.denom) @@ -1374,7 +1375,8 @@ func (suite *KeeperTestSuite) TestIsSendEnabledDenom() { if tc.expDef { exp = def } - assert.Equal(t, exp, actual) + + require.Equal(exp, actual) }) } } @@ -1382,6 +1384,7 @@ func (suite *KeeperTestSuite) TestIsSendEnabledDenom() { func (suite *KeeperTestSuite) TestGetSendEnabledEntry() { ctx, bankKeeper := suite.ctx, suite.bankKeeper + require := suite.Require() bankKeeper.SetAllSendEnabled(ctx, []*banktypes.SendEnabled{ {Denom: "gettruecoin", Enabled: true}, @@ -1413,8 +1416,8 @@ func (suite *KeeperTestSuite) TestGetSendEnabledEntry() { for _, tc := range tests { suite.T().Run(tc.denom, func(t *testing.T) { actualSE, actualF := bankKeeper.GetSendEnabledEntry(ctx, tc.denom) - assert.Equal(t, tc.expF, actualF, "found") - assert.Equal(t, tc.expSE, actualSE, "SendEnabled") + require.Equal(tc.expF, actualF, "found") + require.Equal(tc.expSE, actualSE, "SendEnabled") }) } } @@ -1473,11 +1476,12 @@ func (suite *KeeperTestSuite) TestSetSendEnabled() { for _, def := range []bool{true, false} { params := banktypes.Params{DefaultSendEnabled: def} require.NoError(bankKeeper.SetParams(ctx, params)) + for _, tc := range tests { suite.T().Run(fmt.Sprintf("%s default %t", tc.name, def), func(t *testing.T) { bankKeeper.SetSendEnabled(ctx, tc.denom, tc.value) actual := bankKeeper.IsSendEnabledDenom(ctx, tc.denom) - assert.Equal(t, tc.value, actual) + require.Equal(tc.value, actual) }) } } @@ -1544,12 +1548,14 @@ func (suite *KeeperTestSuite) TestSetAllSendEnabled() { for _, def := range []bool{true, false} { params := banktypes.Params{DefaultSendEnabled: def} require.NoError(bankKeeper.SetParams(ctx, params)) + for _, tc := range tests { suite.T().Run(fmt.Sprintf("%s default %t", tc.name, def), func(t *testing.T) { bankKeeper.SetAllSendEnabled(ctx, tc.sendEnableds) + for _, se := range tc.sendEnableds { actual := bankKeeper.IsSendEnabledDenom(ctx, se.Denom) - assert.Equal(t, se.Enabled, actual, se.Denom) + require.Equal(se.Enabled, actual, se.Denom) } }) } @@ -1583,7 +1589,8 @@ func (suite *KeeperTestSuite) TestIterateSendEnabledEntries() { count++ return false }) - assert.Equal(t, 0, count) + + require.Equal(0, count) }) alpha := strings.Split("abcdefghijklmnopqrstuvwxyz", "") @@ -1598,6 +1605,7 @@ func (suite *KeeperTestSuite) TestIterateSendEnabledEntries() { for _, def := range []bool{true, false} { params := banktypes.Params{DefaultSendEnabled: def} require.NoError(bankKeeper.SetParams(ctx, params)) + var seen []string suite.T().Run(fmt.Sprintf("all denoms have expected values default %t", def), func(t *testing.T) { bankKeeper.IterateSendEnabledEntries(ctx, func(denom string, sendEnabled bool) (stop bool) { @@ -1606,12 +1614,14 @@ func (suite *KeeperTestSuite) TestIterateSendEnabledEntries() { if strings.HasSuffix(denom, "false") { exp = false } - assert.Equal(t, exp, sendEnabled, denom) + + require.Equal(exp, sendEnabled, denom) return false }) }) + suite.T().Run(fmt.Sprintf("all denoms were seen default %t", def), func(t *testing.T) { - assert.ElementsMatch(t, denoms, seen) + require.ElementsMatch(denoms, seen) }) } @@ -1623,7 +1633,8 @@ func (suite *KeeperTestSuite) TestIterateSendEnabledEntries() { count++ return false }) - assert.Equal(t, 0, count) + + require.Equal(0, count) }) } @@ -1633,7 +1644,7 @@ func (suite *KeeperTestSuite) TestGetAllSendEnabledEntries() { suite.T().Run("no entries", func(t *testing.T) { actual := bankKeeper.GetAllSendEnabledEntries(ctx) - assert.Len(t, actual, 0) + require.Len(actual, 0) }) alpha := strings.Split("abcdefghijklmnopqrstuvwxyz", "") @@ -1648,6 +1659,7 @@ func (suite *KeeperTestSuite) TestGetAllSendEnabledEntries() { for _, def := range []bool{true, false} { params := banktypes.Params{DefaultSendEnabled: def} require.NoError(bankKeeper.SetParams(ctx, params)) + var seen []string suite.T().Run(fmt.Sprintf("all denoms have expected values default %t", def), func(t *testing.T) { actual := bankKeeper.GetAllSendEnabledEntries(ctx) @@ -1657,11 +1669,13 @@ func (suite *KeeperTestSuite) TestGetAllSendEnabledEntries() { if strings.HasSuffix(se.Denom, "false") { exp = false } - assert.Equal(t, exp, se.Enabled, se.Denom) + + require.Equal(exp, se.Enabled, se.Denom) } }) + suite.T().Run(fmt.Sprintf("all denoms were seen default %t", def), func(t *testing.T) { - assert.ElementsMatch(t, denoms, seen) + require.ElementsMatch(denoms, seen) }) } @@ -1671,7 +1685,7 @@ func (suite *KeeperTestSuite) TestGetAllSendEnabledEntries() { suite.T().Run("no entries again after deleting all of them", func(t *testing.T) { actual := bankKeeper.GetAllSendEnabledEntries(ctx) - assert.Len(t, actual, 0) + require.Len(actual, 0) }) } @@ -1690,12 +1704,15 @@ func (suite *KeeperTestSuite) TestMigrator_Migrate3to4() { for _, def := range []bool{true, false} { params := banktypes.Params{DefaultSendEnabled: def} require.NoError(bankKeeper.SetParams(ctx, params)) + suite.T().Run(fmt.Sprintf("default %t does not change", def), func(t *testing.T) { legacySubspace := func(ps banktypes.Params) mockSubspace { return mockSubspace{ps: ps} }(banktypes.NewParams(def)) + migrator := keeper.NewMigrator(bankKeeper, legacySubspace) require.NoError(migrator.Migrate3to4(ctx)) + actual := bankKeeper.GetParams(ctx) require.Equal(params.DefaultSendEnabled, actual.DefaultSendEnabled) }) @@ -1708,18 +1725,24 @@ func (suite *KeeperTestSuite) TestMigrator_Migrate3to4() { {Denom: fmt.Sprintf("falsecoin%t", def), Enabled: false}, }, } + require.NoError(bankKeeper.SetParams(ctx, params)) + suite.T().Run(fmt.Sprintf("default %t send enabled info moved to store", def), func(t *testing.T) { legacySubspace := func(ps banktypes.Params) mockSubspace { return mockSubspace{ps: ps} }(banktypes.NewParams(def)) + migrator := keeper.NewMigrator(bankKeeper, legacySubspace) require.NoError(migrator.Migrate3to4(ctx)) + newParams := bankKeeper.GetParams(ctx) - assert.Len(t, newParams.SendEnabled, 0) + require.Len(newParams.SendEnabled, 0) + require.Equal(def, newParams.DefaultSendEnabled) + for _, se := range params.SendEnabled { actual := bankKeeper.IsSendEnabledDenom(ctx, se.Denom) - assert.Equal(t, se.Enabled, actual, se.Denom) + require.Equal(se.Enabled, actual, se.Denom) } }) } @@ -1728,6 +1751,7 @@ func (suite *KeeperTestSuite) TestMigrator_Migrate3to4() { func (suite *KeeperTestSuite) TestSetParams() { ctx, bankKeeper := suite.ctx, suite.bankKeeper require := suite.Require() + params := banktypes.NewParams(true) params.SendEnabled = []*banktypes.SendEnabled{ {Denom: "paramscointrue", Enabled: true}, @@ -1737,21 +1761,17 @@ func (suite *KeeperTestSuite) TestSetParams() { suite.Run("stored params are as expected", func() { actual := bankKeeper.GetParams(ctx) - suite.Assert().True(actual.DefaultSendEnabled, "DefaultSendEnabled") - suite.Assert().Len(actual.SendEnabled, 0, "SendEnabled") + require.True(actual.DefaultSendEnabled, "DefaultSendEnabled") + require.Len(actual.SendEnabled, 0, "SendEnabled") }) suite.Run("send enabled params converted to store", func() { actual := bankKeeper.GetAllSendEnabledEntries(ctx) if suite.Assert().Len(actual, 2) { - suite.Equal("paramscoinfalse", actual[0].Denom, "actual[0].Denom") - suite.False(actual[0].Enabled, "actual[0].Enabled") - suite.Equal("paramscointrue", actual[1].Denom, "actual[1].Denom") - suite.True(actual[1].Enabled, "actual[1].Enabled") + require.Equal("paramscoinfalse", actual[0].Denom, "actual[0].Denom") + require.False(actual[0].Enabled, "actual[0].Enabled") + require.Equal("paramscointrue", actual[1].Denom, "actual[1].Denom") + require.True(actual[1].Enabled, "actual[1].Enabled") } }) } - -func TestKeeperTestSuite(t *testing.T) { - suite.Run(t, new(KeeperTestSuite)) -} diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index dad56a3f8827..f1573a6c1d50 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -3,6 +3,8 @@ package keeper import ( "fmt" + gogotypes "github.com/cosmos/gogoproto/types" + "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/store/prefix" storetypes "github.com/cosmos/cosmos-sdk/store/types" @@ -100,12 +102,16 @@ func (k BaseSendKeeper) GetParams(ctx sdk.Context) (params types.Params) { // SetParams sets the total set of bank parameters. // -//nolint:staticcheck // params.SendEnabled is deprecated but it should be here regardless. +// Note: params.SendEnabled is deprecated but it should be here regardless. +// +//nolint:staticcheck func (k BaseSendKeeper) SetParams(ctx sdk.Context, params types.Params) error { - // normally SendEnabled is deprecated but we still support it for backwards compatibility - // using params.Validate() would fail due to the SendEnabled deprecation + // Normally SendEnabled is deprecated but we still support it for backwards + // compatibility. Using params.Validate() would fail due to the SendEnabled + // deprecation. if len(params.SendEnabled) > 0 { k.SetAllSendEnabled(ctx, params.SendEnabled) + // override params without SendEnabled params = types.NewParams(params.DefaultSendEnabled) } @@ -115,6 +121,7 @@ func (k BaseSendKeeper) SetParams(ctx sdk.Context, params types.Params) error { if err != nil { return err } + store.Set(types.ParamsKey, bz) return nil } @@ -139,6 +146,7 @@ func (k BaseSendKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input, if err != nil { return err } + ctx.EventManager().EmitEvent( sdk.NewEvent( sdk.EventTypeMessage, @@ -152,8 +160,8 @@ func (k BaseSendKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input, if err != nil { return err } - err = k.addCoins(ctx, outAddress, out.Coins) - if err != nil { + + if err := k.addCoins(ctx, outAddress, out.Coins); err != nil { return err } @@ -213,7 +221,7 @@ func (k BaseSendKeeper) SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAd ), sdk.NewEvent( sdk.EventTypeMessage, - sdk.NewAttribute(types.AttributeKeySender, fromAddrString), + sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()), ), }) @@ -255,15 +263,15 @@ func (k BaseSendKeeper) subUnlockedCoins(ctx sdk.Context, addr sdk.AccAddress, a } } - // emit coin spent event ctx.EventManager().EmitEvent( types.NewCoinSpentEvent(addr, amt), ) + return nil } -// addCoins increase the addr balance by the given amt. Fails if the provided amt is invalid. -// It emits a coin received event. +// addCoins increase the addr balance by the given amt. Fails if the provided +// amt is invalid. It emits a coin received event. func (k BaseSendKeeper) addCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) error { if !amt.IsValid() { return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, amt.String()) @@ -343,6 +351,7 @@ func (k BaseSendKeeper) setBalance(ctx sdk.Context, addr sdk.AccAddress, balance if err != nil { return err } + accountStore.Set([]byte(balance.Denom), amount) // Store a reverse index from denomination to account address with a @@ -356,28 +365,23 @@ func (k BaseSendKeeper) setBalance(ctx sdk.Context, addr sdk.AccAddress, balance return nil } -// IsSendEnabledCoins checks the coins provide and returns an ErrSendDisabled if -// any of the coins are not configured for sending. Returns nil if sending is enabled -// for all provided coin +// IsSendEnabledCoins checks the coins provided and returns an ErrSendDisabled +// if any of the coins are not configured for sending. Returns nil if sending is +// enabled for all provided coins. func (k BaseSendKeeper) IsSendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error { if len(coins) == 0 { return nil } + store := ctx.KVStore(k.storeKey) - haveDefault := false - var defaultVal bool - getDefault := func() bool { - if !haveDefault { - defaultVal = k.GetParams(ctx).DefaultSendEnabled - haveDefault = true - } - return defaultVal - } + defaultVal := k.GetParams(ctx).DefaultSendEnabled + for _, coin := range coins { - if !k.getSendEnabledOrDefault(store, coin.Denom, getDefault) { + if !k.getSendEnabledOrDefault(store, coin.Denom, defaultVal) { return types.ErrSendDisabled.Wrapf("%s transfers are currently disabled", coin.Denom) } } + return nil } @@ -399,7 +403,7 @@ func (k BaseSendKeeper) GetBlockedAddresses() map[string]bool { // IsSendEnabledDenom returns the current SendEnabled status of the provided denom. func (k BaseSendKeeper) IsSendEnabledDenom(ctx sdk.Context, denom string) bool { - return k.getSendEnabledOrDefault(ctx.KVStore(k.storeKey), denom, func() bool { return k.GetParams(ctx).DefaultSendEnabled }) + return k.getSendEnabledOrDefault(ctx.KVStore(k.storeKey), denom, k.GetParams(ctx).DefaultSendEnabled) } // GetSendEnabledEntry gets a SendEnabled entry for the given denom. @@ -409,6 +413,7 @@ func (k BaseSendKeeper) GetSendEnabledEntry(ctx sdk.Context, denom string) (type if !found { return types.SendEnabled{}, false } + return types.SendEnabled{Denom: denom, Enabled: sendEnabled}, true } @@ -419,18 +424,19 @@ func (k BaseSendKeeper) SetSendEnabled(ctx sdk.Context, denom string, value bool } // SetAllSendEnabled sets all the provided SendEnabled entries in the bank store. -func (k BaseSendKeeper) SetAllSendEnabled(ctx sdk.Context, sendEnableds []*types.SendEnabled) { +func (k BaseSendKeeper) SetAllSendEnabled(ctx sdk.Context, entries []*types.SendEnabled) { store := ctx.KVStore(k.storeKey) - for _, se := range sendEnableds { - k.setSendEnabledEntry(store, se.Denom, se.Enabled) + for _, entry := range entries { + k.setSendEnabledEntry(store, entry.Denom, entry.Enabled) } } // setSendEnabledEntry sets SendEnabled for the given denom to the give value in the provided store. func (k BaseSendKeeper) setSendEnabledEntry(store sdk.KVStore, denom string, value bool) { key := types.CreateSendEnabledKey(denom) - val := types.ToBoolB(value) - store.Set(key, []byte{val}) + + bz := k.cdc.MustMarshal(&gogotypes.BoolValue{Value: value}) + store.Set(key, bz) } // DeleteSendEnabled deletes the SendEnabled flags for one or more denoms. @@ -456,25 +462,30 @@ func (k BaseSendKeeper) IterateSendEnabledEntries(ctx sdk.Context, cb func(denom for ; iterator.Valid(); iterator.Next() { denom := string(iterator.Key()) - val := types.IsTrueB(iterator.Value()) - if cb(denom, val) { + + var enabled gogotypes.BoolValue + k.cdc.MustUnmarshal(iterator.Value(), &enabled) + + if cb(denom, enabled.Value) { break } } } // GetAllSendEnabledEntries gets all the SendEnabled entries that are stored. -// Any denoms not returned use the default value (set in Params). +// Any denominations not returned use the default value (set in Params). func (k BaseSendKeeper) GetAllSendEnabledEntries(ctx sdk.Context) []types.SendEnabled { var rv []types.SendEnabled k.IterateSendEnabledEntries(ctx, func(denom string, sendEnabled bool) bool { rv = append(rv, types.SendEnabled{Denom: denom, Enabled: sendEnabled}) return false }) + return rv } -// getSendEnabled returns whether send is enabled and whether that flag was set for a denom. +// getSendEnabled returns whether send is enabled and whether that flag was set +// for a denom. // // Example usage: // @@ -488,25 +499,25 @@ func (k BaseSendKeeper) getSendEnabled(store sdk.KVStore, denom string) (bool, b if !store.Has(key) { return false, false } - v := store.Get(key) - if len(v) != 1 { - return false, false - } - switch v[0] { - case types.TrueB: - return true, true - case types.FalseB: - return false, true - default: + + bz := store.Get(key) + if bz == nil { return false, false } + + var enabled gogotypes.BoolValue + k.cdc.MustUnmarshal(bz, &enabled) + + return enabled.Value, true } -// getSendEnabledOrDefault gets the send_enabled value for a denom. If it's not in the store, this will return the result of the getDefault function. -func (k BaseSendKeeper) getSendEnabledOrDefault(store sdk.KVStore, denom string, getDefault func() bool) bool { +// getSendEnabledOrDefault gets the SendEnabled value for a denom. If it's not +// in the store, this will return defaultVal. +func (k BaseSendKeeper) getSendEnabledOrDefault(store sdk.KVStore, denom string, defaultVal bool) bool { sendEnabled, found := k.getSendEnabled(store, denom) if found { return sendEnabled } - return getDefault() + + return defaultVal } diff --git a/x/bank/module.go b/x/bank/module.go index 16e522a57350..4ad2803f8c51 100644 --- a/x/bank/module.go +++ b/x/bank/module.go @@ -6,15 +6,14 @@ import ( "fmt" "time" + modulev1 "cosmossdk.io/api/cosmos/bank/module/v1" + "cosmossdk.io/core/appmodule" + "cosmossdk.io/depinject" gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime" "github.com/spf13/cobra" abci "github.com/tendermint/tendermint/abci/types" "golang.org/x/exp/maps" - modulev1 "cosmossdk.io/api/cosmos/bank/module/v1" - "cosmossdk.io/core/appmodule" - "cosmossdk.io/depinject" - "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" diff --git a/x/bank/types/genesis.go b/x/bank/types/genesis.go index 2e73b898ada8..e414247f3ce0 100644 --- a/x/bank/types/genesis.go +++ b/x/bank/types/genesis.go @@ -107,31 +107,38 @@ func GetGenesisStateFromAppState(cdc codec.JSONCodec, appState map[string]json.R return &genesisState } -// MigrateSendEnabled moves the SendEnabled info from Params into the GenesisState.SendEnabled field and removes them from Params. -// If the Params.SendEnabled slice is empty, this is a noop. -// If the main SendEnabled slice already has entries, the Params.SendEnabled entries are added. -// In case of the same demon in both, preference is given to the existing (main GenesisState field) entry. +// MigrateSendEnabled moves the SendEnabled info from Params into the +// GenesisState.SendEnabled field and removes them from Params. If the +// Params.SendEnabled slice is empty, this is a noop. +// +// If the main SendEnabled slice already has entries, the Params.SendEnabled +// entries are added. In case of the same demon in both, preference is given to +// the existing (main GenesisState field) entry. func (g *GenesisState) MigrateSendEnabled() { g.SendEnabled = g.GetAllSendEnabled() g.Params.SendEnabled = nil } -// GetAllSendEnabled returns all the SendEnabled entries from both the SendEnabled field and the Params. -// If a denom has an entry in both, the entry in the SendEnabled field takes precedence over one in Params. +// GetAllSendEnabled returns all the SendEnabled entries from both the SendEnabled +// field and the Params. If a denom has an entry in both, the entry in the +// SendEnabled field takes precedence over one in Params. func (g GenesisState) GetAllSendEnabled() []SendEnabled { if len(g.Params.SendEnabled) == 0 { return g.SendEnabled } + rv := make([]SendEnabled, len(g.SendEnabled)) knownSendEnabled := map[string]bool{} for i, se := range g.SendEnabled { rv[i] = se knownSendEnabled[se.Denom] = true } + for _, se := range g.Params.SendEnabled { if _, known := knownSendEnabled[se.Denom]; !known { rv = append(rv, *se) } } + return rv } diff --git a/x/bank/types/keys.go b/x/bank/types/keys.go index 86033a51f3ef..a7ba7114d8f0 100644 --- a/x/bank/types/keys.go +++ b/x/bank/types/keys.go @@ -37,13 +37,6 @@ var ( ParamsKey = []byte{0x05} ) -const ( - // TrueB is a byte with value 1 that represents true. - TrueB = byte(0x01) - // FalseB is a byte with value 0 that represents false. - FalseB = byte(0x00) -) - // AddressAndDenomFromBalancesStore returns an account address and denom from a balances prefix // store. The key must not contain the prefix BalancesPrefix as the prefix store // iterator discards the actual prefix. @@ -94,16 +87,3 @@ func CreateSendEnabledKey(denom string) []byte { copy(key[len(SendEnabledPrefix):], denom) return key } - -// IsTrueB returns true if the provided byte slice has exactly one byte, and it is equal to TrueB. -func IsTrueB(bz []byte) bool { - return len(bz) == 1 && bz[0] == TrueB -} - -// ToBoolB returns TrueB if v is true, and FalseB if it's false. -func ToBoolB(v bool) byte { - if v { - return TrueB - } - return FalseB -} diff --git a/x/bank/types/keys_test.go b/x/bank/types/keys_test.go index e4d3d06aff35..bfffdc2f6270 100644 --- a/x/bank/types/keys_test.go +++ b/x/bank/types/keys_test.go @@ -77,57 +77,3 @@ func TestCreateSendEnabledKey(t *testing.T) { assert.Equal(t, types.SendEnabledPrefix, actual[:len(types.SendEnabledPrefix)], "prefix") assert.Equal(t, []byte(denom), actual[len(types.SendEnabledPrefix):], "denom part") } - -func TestIsTrueB(t *testing.T) { - tests := []struct { - name string - bz []byte - exp bool - }{ - { - name: "empty bz", - bz: []byte{}, - exp: false, - }, - { - name: "nil bz", - bz: nil, - exp: false, - }, - { - name: "one byte zero", - bz: []byte{0x00}, - exp: false, - }, - { - name: "one byte one", - bz: []byte{0x01}, - exp: true, - }, - { - name: "one byte two", - bz: []byte{0x02}, - exp: false, - }, - { - name: "two bytes one zero", - bz: []byte{0x01, 0x00}, - exp: false, - }, - } - for _, tc := range tests { - t.Run(tc.name, func(tt *testing.T) { - actual := types.IsTrueB(tc.bz) - assert.Equal(t, tc.exp, actual) - }) - } -} - -func TestToBoolB(t *testing.T) { - t.Run("true", func(t *testing.T) { - assert.Equal(t, types.TrueB, types.ToBoolB(true)) - }) - t.Run("false", func(t *testing.T) { - assert.Equal(t, types.FalseB, types.ToBoolB(false)) - }) -} diff --git a/x/bank/types/msgs.go b/x/bank/types/msgs.go index 988d0964861b..81be11f5ac67 100644 --- a/x/bank/types/msgs.go +++ b/x/bank/types/msgs.go @@ -9,7 +9,7 @@ import ( const ( TypeMsgSend = "send" TypeMsgMultiSend = "multisend" - TypeMsgSetSendEnabled = "set-send-enabled" + TypeMsgSetSendEnabled = "set_send_enabled" TypeMsgUpdateParams = "update_params" ) @@ -235,25 +235,29 @@ func (msg MsgSetSendEnabled) GetSigners() []sdk.AccAddress { // ValidateBasic runs basic validation on this MsgSetSendEnabled. func (msg MsgSetSendEnabled) ValidateBasic() error { if len(msg.Authority) > 0 { - _, err := sdk.AccAddressFromBech32(msg.Authority) - if err != nil { + 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/send_authorization.go b/x/bank/types/send_authorization.go index 91b37def1c55..2e8ee91e9ec8 100644 --- a/x/bank/types/send_authorization.go +++ b/x/bank/types/send_authorization.go @@ -6,21 +6,19 @@ import ( "github.com/cosmos/cosmos-sdk/x/authz" ) -// TODO: Revisit this once we have propoer gas fee framework. -// Tracking issues https://github.com/cosmos/cosmos-sdk/issues/9054, https://github.com/cosmos/cosmos-sdk/discussions/9072 +// TODO: Revisit this once we have proper gas fee framework. +// Ref: https://github.com/cosmos/cosmos-sdk/issues/9054 +// Ref: https://github.com/cosmos/cosmos-sdk/discussions/9072 const gasCostPerIteration = uint64(10) var _ authz.Authorization = &SendAuthorization{} // NewSendAuthorization creates a new SendAuthorization object. func NewSendAuthorization(spendLimit sdk.Coins, allowed []sdk.AccAddress) *SendAuthorization { - allowedAddrs := toBech32Addresses(allowed) - - a := SendAuthorization{} - a.AllowList = allowedAddrs - a.SpendLimit = spendLimit - - return &a + return &SendAuthorization{ + AllowList: toBech32Addresses(allowed), + SpendLimit: spendLimit, + } } // MsgTypeURL implements Authorization.MsgTypeURL. @@ -34,7 +32,9 @@ func (a SendAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.AcceptRes if !ok { return authz.AcceptResponse{}, sdkerrors.ErrInvalidType.Wrap("type mismatch") } + toAddr := mSend.ToAddress + limitLeft, isNegative := a.SpendLimit.SafeSub(mSend.Amount...) if isNegative { return authz.AcceptResponse{}, sdkerrors.ErrInsufficientFunds.Wrapf("requested amount is more than spend limit") @@ -75,8 +75,10 @@ func (a SendAuthorization) ValidateBasic() error { if found[a.AllowList[i]] { return ErrDuplicateEntry } + found[a.AllowList[i]] = true } + return nil } @@ -89,5 +91,6 @@ func toBech32Addresses(allowed []sdk.AccAddress) []string { for i, addr := range allowed { allowedAddrs[i] = addr.String() } + return allowedAddrs }