From be61cb554dbd8cd993cc8b9de28d8c461e1ad75d Mon Sep 17 00:00:00 2001 From: Roman Date: Tue, 13 Sep 2022 00:01:23 -0400 Subject: [PATCH] Revert "feat: add a BeforeSend hook to the bank module (#278)" (#326) This reverts commit 6fcd25f8abb82ac4db337d72510429568362d348. --- x/bank/keeper/hooks.go | 17 ------ x/bank/keeper/hooks_test.go | 101 ------------------------------- x/bank/keeper/internal_test.go | 11 ---- x/bank/keeper/keeper.go | 16 +---- x/bank/keeper/send.go | 27 +-------- x/bank/types/expected_keepers.go | 11 ---- x/bank/types/hooks.go | 24 -------- 7 files changed, 4 insertions(+), 203 deletions(-) delete mode 100644 x/bank/keeper/hooks.go delete mode 100644 x/bank/keeper/hooks_test.go delete mode 100644 x/bank/keeper/internal_test.go delete mode 100644 x/bank/types/hooks.go diff --git a/x/bank/keeper/hooks.go b/x/bank/keeper/hooks.go deleted file mode 100644 index 8876fa6e7f51..000000000000 --- a/x/bank/keeper/hooks.go +++ /dev/null @@ -1,17 +0,0 @@ -package keeper - -import ( - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/bank/types" -) - -// Implements StakingHooks interface -var _ types.BankHooks = BaseSendKeeper{} - -// BeforeSend executes the BeforeSend hook if registered. -func (k BaseSendKeeper) BeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) error { - if k.hooks != nil { - return k.hooks.BeforeSend(ctx, from, to, amount) - } - return nil -} diff --git a/x/bank/keeper/hooks_test.go b/x/bank/keeper/hooks_test.go deleted file mode 100644 index 5fe63288b6c8..000000000000 --- a/x/bank/keeper/hooks_test.go +++ /dev/null @@ -1,101 +0,0 @@ -package keeper_test - -import ( - "fmt" - "testing" - - "github.com/stretchr/testify/require" - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" - - "github.com/cosmos/cosmos-sdk/simapp" - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/bank/keeper" - "github.com/cosmos/cosmos-sdk/x/bank/types" - - stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" -) - -var _ types.BankHooks = &MockBankHooksReceiver{} - -// BankHooks event hooks for bank (noalias) -type MockBankHooksReceiver struct{} - -// Mock BeforeSend bank hook that doesn't allow the sending of exactly 100 coins of any denom. -func (h *MockBankHooksReceiver) BeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) error { - for _, coin := range amount { - if coin.Amount.Equal(sdk.NewInt(100)) { - return fmt.Errorf("not allowed; expected %v, got: %v", 100, coin.Amount) - } - } - - return nil -} - -func TestHooks(t *testing.T) { - app := simapp.Setup(false) - ctx := app.BaseApp.NewContext(false, tmproto.Header{}) - - addrs := simapp.AddTestAddrs(app, ctx, 2, sdk.NewInt(1000)) - simapp.FundModuleAccount(app.BankKeeper, ctx, stakingtypes.BondedPoolName, sdk.NewCoins(sdk.NewCoin(app.StakingKeeper.BondDenom(ctx), sdk.NewInt(1000)))) - - // create a valid send amount which is 1 coin, and an invalidSendAmount which is 100 coins - validSendAmount := sdk.NewCoins(sdk.NewCoin(app.StakingKeeper.BondDenom(ctx), sdk.NewInt(1))) - invalidSendAmount := sdk.NewCoins(sdk.NewCoin(app.StakingKeeper.BondDenom(ctx), sdk.NewInt(100))) - - // setup our mock bank hooks receiver that prevents the send of 100 coins - bankHooksReceiver := MockBankHooksReceiver{} - baseBankKeeper, ok := app.BankKeeper.(keeper.BaseKeeper) - require.True(t, ok) - keeper.UnsafeSetHooks( - &baseBankKeeper, types.NewMultiBankHooks(&bankHooksReceiver), - ) - app.BankKeeper = baseBankKeeper - - // try sending a validSendAmount and it should work - err := app.BankKeeper.SendCoins(ctx, addrs[0], addrs[1], validSendAmount) - require.NoError(t, err) - - // try sending an invalidSendAmount and it should not work - err = app.BankKeeper.SendCoins(ctx, addrs[0], addrs[1], invalidSendAmount) - require.Error(t, err) - - // try doing SendManyCoins and make sure if even a single subsend is invalid, the entire function fails - err = app.BankKeeper.SendManyCoins(ctx, addrs[0], []sdk.AccAddress{addrs[0], addrs[1]}, []sdk.Coins{invalidSendAmount, validSendAmount}) - require.Error(t, err) - - // make sure that account to module doesn't bypass hook - err = app.BankKeeper.SendCoinsFromAccountToModule(ctx, addrs[0], stakingtypes.BondedPoolName, validSendAmount) - require.NoError(t, err) - err = app.BankKeeper.SendCoinsFromAccountToModule(ctx, addrs[0], stakingtypes.BondedPoolName, invalidSendAmount) - require.Error(t, err) - - // make sure that module to account doesn't bypass hook - err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, stakingtypes.BondedPoolName, addrs[0], validSendAmount) - require.NoError(t, err) - err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, stakingtypes.BondedPoolName, addrs[0], invalidSendAmount) - require.Error(t, err) - - // make sure that module to module doesn't bypass hook - err = app.BankKeeper.SendCoinsFromModuleToModule(ctx, stakingtypes.BondedPoolName, stakingtypes.NotBondedPoolName, validSendAmount) - require.NoError(t, err) - err = app.BankKeeper.SendCoinsFromModuleToModule(ctx, stakingtypes.BondedPoolName, stakingtypes.NotBondedPoolName, invalidSendAmount) - require.Error(t, err) - - // make sure that module to many accounts doesn't bypass hook - err = app.BankKeeper.SendCoinsFromModuleToManyAccounts(ctx, stakingtypes.BondedPoolName, []sdk.AccAddress{addrs[0], addrs[1]}, []sdk.Coins{validSendAmount, validSendAmount}) - require.NoError(t, err) - err = app.BankKeeper.SendCoinsFromModuleToManyAccounts(ctx, stakingtypes.BondedPoolName, []sdk.AccAddress{addrs[0], addrs[1]}, []sdk.Coins{validSendAmount, invalidSendAmount}) - require.Error(t, err) - - // make sure that DelegateCoins doesn't bypass the hook - err = app.BankKeeper.DelegateCoins(ctx, addrs[0], app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), validSendAmount) - require.NoError(t, err) - err = app.BankKeeper.DelegateCoins(ctx, addrs[0], app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), invalidSendAmount) - require.Error(t, err) - - // make sure that UndelegateCoins doesn't bypass the hook - err = app.BankKeeper.UndelegateCoins(ctx, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), addrs[0], validSendAmount) - require.NoError(t, err) - err = app.BankKeeper.UndelegateCoins(ctx, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), addrs[0], invalidSendAmount) - require.Error(t, err) -} diff --git a/x/bank/keeper/internal_test.go b/x/bank/keeper/internal_test.go deleted file mode 100644 index 3408248f1a98..000000000000 --- a/x/bank/keeper/internal_test.go +++ /dev/null @@ -1,11 +0,0 @@ -package keeper - -import "github.com/cosmos/cosmos-sdk/x/bank/types" - -// UnsafeSetHooks updates the x/bank keeper's hooks, overriding any potential -// pre-existing hooks. -// -// WARNING: this function should only be used in tests. -func UnsafeSetHooks(k *BaseKeeper, h types.BankHooks) { - k.hooks = h -} diff --git a/x/bank/keeper/keeper.go b/x/bank/keeper/keeper.go index 776fcdcdbfb2..a85d0a0536fd 100644 --- a/x/bank/keeper/keeper.go +++ b/x/bank/keeper/keeper.go @@ -183,12 +183,6 @@ func (k BaseKeeper) DelegateCoins(ctx sdk.Context, delegatorAddr, moduleAccAddr return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, amt.String()) } - // call the BeforeSend hooks - err := k.BeforeSend(ctx, delegatorAddr, moduleAccAddr, amt) - if err != nil { - return err - } - balances := sdk.NewCoins() for _, coin := range amt { @@ -214,7 +208,7 @@ func (k BaseKeeper) DelegateCoins(ctx sdk.Context, delegatorAddr, moduleAccAddr types.NewCoinSpentEvent(delegatorAddr, amt), ) - err = k.addCoins(ctx, moduleAccAddr, amt) + err := k.addCoins(ctx, moduleAccAddr, amt) if err != nil { return err } @@ -237,13 +231,7 @@ func (k BaseKeeper) UndelegateCoins(ctx sdk.Context, moduleAccAddr, delegatorAdd return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, amt.String()) } - // call the BeforeSend hooks - err := k.BeforeSend(ctx, moduleAccAddr, delegatorAddr, amt) - if err != nil { - return err - } - - err = k.subUnlockedCoins(ctx, moduleAccAddr, amt) + err := k.subUnlockedCoins(ctx, moduleAccAddr, amt) if err != nil { return err } diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index e94bcc631500..f2c90f92a4bb 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -39,7 +39,6 @@ type BaseSendKeeper struct { ak types.AccountKeeper storeKey sdk.StoreKey paramSpace paramtypes.Subspace - hooks types.BankHooks // list of addresses that are restricted from receiving transactions blockedAddrs map[string]bool @@ -59,17 +58,6 @@ func NewBaseSendKeeper( } } -// Set the bank hooks -func (k *BaseSendKeeper) SetHooks(bh types.BankHooks) *BaseSendKeeper { - if k.hooks != nil { - panic("cannot set bank hooks twice") - } - - k.hooks = bh - - return k -} - // GetParams returns the total set of bank parameters. func (k BaseSendKeeper) GetParams(ctx sdk.Context) (params types.Params) { k.paramSpace.GetParamSet(ctx, ¶ms) @@ -84,13 +72,7 @@ func (k BaseSendKeeper) SetParams(ctx sdk.Context, params types.Params) { // SendCoins transfers amt coins from a sending account to a receiving account. // An error is returned upon failure. func (k BaseSendKeeper) SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error { - // call the BeforeSend hooks - err := k.BeforeSend(ctx, fromAddr, toAddr, amt) - if err != nil { - return err - } - - err = k.subUnlockedCoins(ctx, fromAddr, amt) + err := k.subUnlockedCoins(ctx, fromAddr, amt) if err != nil { return err } @@ -134,12 +116,7 @@ func (k BaseSendKeeper) SendManyCoins(ctx sdk.Context, fromAddr sdk.AccAddress, } totalAmt := sdk.Coins{} - for i, amt := range amts { - // make sure to trigger the BeforeSend hooks for all the sends that are about to occur - err := k.BeforeSend(ctx, fromAddr, toAddrs[i], amts[i]) - if err != nil { - return err - } + for _, amt := range amts { totalAmt = sdk.Coins.Add(totalAmt, amt...) } diff --git a/x/bank/types/expected_keepers.go b/x/bank/types/expected_keepers.go index 270ff42115d2..23ca020a34b0 100644 --- a/x/bank/types/expected_keepers.go +++ b/x/bank/types/expected_keepers.go @@ -26,14 +26,3 @@ type AccountKeeper interface { GetModuleAccount(ctx sdk.Context, moduleName string) types.ModuleAccountI SetModuleAccount(ctx sdk.Context, macc types.ModuleAccountI) } - -// Event Hooks -// These can be utilized to communicate between a bank keeper and another -// keeper which must take particular actions when sends happen. -// The second keeper must implement this interface, which then the -// bank keeper can call. - -// BankHooks event hooks for bank sends -type BankHooks interface { - BeforeSend(ctx sdk.Context, from sdk.AccAddress, to sdk.AccAddress, amount sdk.Coins) error // Must be before any send is executed -} diff --git a/x/bank/types/hooks.go b/x/bank/types/hooks.go deleted file mode 100644 index 09d4296fdda2..000000000000 --- a/x/bank/types/hooks.go +++ /dev/null @@ -1,24 +0,0 @@ -package types - -import ( - sdk "github.com/cosmos/cosmos-sdk/types" -) - -// MultiBankHooks combine multiple bank hooks, all hook functions are run in array sequence -type MultiBankHooks []BankHooks - -// NewMultiBankHooks takes a list of BankHooks and returns a MultiBankHooks -func NewMultiBankHooks(hooks ...BankHooks) MultiBankHooks { - return hooks -} - -// BeforeSend runs the BeforeSend hooks in order for each BankHook in a MultiBankHooks struct -func (h MultiBankHooks) BeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) error { - for i := range h { - err := h[i].BeforeSend(ctx, from, to, amount) - if err != nil { - return err - } - } - return nil -}