From 3716098d8fac47e783119ffc492249646c117990 Mon Sep 17 00:00:00 2001 From: mattverse Date: Tue, 7 Mar 2023 16:47:47 +0900 Subject: [PATCH 1/2] Add separated hooks for bank send --- x/bank/keeper/hooks.go | 13 ++++-- x/bank/keeper/hooks_test.go | 72 ++++++++++++++++++++++++++------ x/bank/keeper/keeper.go | 15 ++++--- x/bank/keeper/send.go | 24 +++++++++-- x/bank/types/expected_keepers.go | 3 +- x/bank/types/hooks.go | 13 ++++-- 6 files changed, 111 insertions(+), 29 deletions(-) diff --git a/x/bank/keeper/hooks.go b/x/bank/keeper/hooks.go index 8876fa6e7f51..83d1e722cf71 100644 --- a/x/bank/keeper/hooks.go +++ b/x/bank/keeper/hooks.go @@ -8,10 +8,17 @@ import ( // 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 { +// TrackbeforeSend executes the TrackBeforeSend hook if registered. +func (k BaseSendKeeper) TrackBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) { if k.hooks != nil { - return k.hooks.BeforeSend(ctx, from, to, amount) + k.hooks.TrackBeforeSend(ctx, from, to, amount) + } +} + +// BlockBeforeSend executes the BlockBeforeSend hook if registered. +func (k BaseSendKeeper) BlockBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) error { + if k.hooks != nil { + return k.hooks.BlockBeforeSend(ctx, from, to, amount) } return nil } diff --git a/x/bank/keeper/hooks_test.go b/x/bank/keeper/hooks_test.go index 5fe63288b6c8..22ec05d7ba44 100644 --- a/x/bank/keeper/hooks_test.go +++ b/x/bank/keeper/hooks_test.go @@ -20,17 +20,31 @@ 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 { +// Mock BlockBeforeSend bank hook that doesn't allow the sending of exactly 100 coins of any denom. +func (h *MockBankHooksReceiver) BlockBeforeSend(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 } +// variable for counting `TrackBeforeSend` +var ( + countTrackBeforeSend = 0 + expNextCount = 1 +) + +// Mock TrackBeforeSend bank hook that simply tracks the sending of exactly 50 coins of any denom. +func (h *MockBankHooksReceiver) TrackBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) { + for _, coin := range amount { + if coin.Amount.Equal(sdk.NewInt(50)) { + countTrackBeforeSend += 1 + } + } +} + func TestHooks(t *testing.T) { app := simapp.Setup(false) ctx := app.BaseApp.NewContext(false, tmproto.Header{}) @@ -40,7 +54,8 @@ func TestHooks(t *testing.T) { // 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))) + triggerTrackSendAmount := sdk.NewCoins(sdk.NewCoin(app.StakingKeeper.BondDenom(ctx), sdk.NewInt(50))) + invalidBlockSendAmount := 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{} @@ -55,47 +70,78 @@ func TestHooks(t *testing.T) { err := app.BankKeeper.SendCoins(ctx, addrs[0], addrs[1], validSendAmount) require.NoError(t, err) + // try sending an trigger track send amount and it should work + err = app.BankKeeper.SendCoins(ctx, addrs[0], addrs[1], triggerTrackSendAmount) + require.NoError(t, err) + + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ + // try sending an invalidSendAmount and it should not work - err = app.BankKeeper.SendCoins(ctx, addrs[0], addrs[1], invalidSendAmount) + err = app.BankKeeper.SendCoins(ctx, addrs[0], addrs[1], invalidBlockSendAmount) 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}) + err = app.BankKeeper.SendManyCoins(ctx, addrs[0], []sdk.AccAddress{addrs[0], addrs[1]}, []sdk.Coins{invalidBlockSendAmount, validSendAmount}) require.Error(t, err) + err = app.BankKeeper.SendManyCoins(ctx, addrs[0], []sdk.AccAddress{addrs[0], addrs[1]}, []sdk.Coins{triggerTrackSendAmount, validSendAmount}) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ + // 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) + err = app.BankKeeper.SendCoinsFromAccountToModule(ctx, addrs[0], stakingtypes.BondedPoolName, invalidBlockSendAmount) require.Error(t, err) + err = app.BankKeeper.SendCoinsFromAccountToModule(ctx, addrs[0], stakingtypes.BondedPoolName, triggerTrackSendAmount) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ // 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) + err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, stakingtypes.BondedPoolName, addrs[0], invalidBlockSendAmount) require.Error(t, err) + err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, stakingtypes.BondedPoolName, addrs[0], triggerTrackSendAmount) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ // 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) + err = app.BankKeeper.SendCoinsFromModuleToModule(ctx, stakingtypes.BondedPoolName, stakingtypes.NotBondedPoolName, invalidBlockSendAmount) + // there should be no error since module to module does not call block before send hooks + require.NoError(t, err) + err = app.BankKeeper.SendCoinsFromModuleToModule(ctx, stakingtypes.BondedPoolName, stakingtypes.NotBondedPoolName, triggerTrackSendAmount) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ // 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}) + err = app.BankKeeper.SendCoinsFromModuleToManyAccounts(ctx, stakingtypes.BondedPoolName, []sdk.AccAddress{addrs[0], addrs[1]}, []sdk.Coins{validSendAmount, invalidBlockSendAmount}) require.Error(t, err) + err = app.BankKeeper.SendCoinsFromModuleToManyAccounts(ctx, stakingtypes.BondedPoolName, []sdk.AccAddress{addrs[0], addrs[1]}, []sdk.Coins{validSendAmount, triggerTrackSendAmount}) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ // 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) + err = app.BankKeeper.DelegateCoins(ctx, addrs[0], app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), invalidBlockSendAmount) require.Error(t, err) + err = app.BankKeeper.DelegateCoins(ctx, addrs[0], app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), triggerTrackSendAmount) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ // 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) + err = app.BankKeeper.UndelegateCoins(ctx, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), addrs[0], invalidBlockSendAmount) require.Error(t, err) + + err = app.BankKeeper.UndelegateCoins(ctx, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), addrs[0], triggerTrackSendAmount) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ } diff --git a/x/bank/keeper/keeper.go b/x/bank/keeper/keeper.go index 776fcdcdbfb2..8e98a6975c45 100644 --- a/x/bank/keeper/keeper.go +++ b/x/bank/keeper/keeper.go @@ -183,11 +183,12 @@ 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) + err := k.BlockBeforeSend(ctx, delegatorAddr, moduleAccAddr, amt) if err != nil { return err } + // call the TrackBeforeSend hooks and the BlockBeforeSend hooks + k.TrackBeforeSend(ctx, delegatorAddr, moduleAccAddr, amt) balances := sdk.NewCoins() @@ -237,12 +238,14 @@ 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) + // call the TrackBeforeSend hooks and the BlockBeforeSend hooks + err := k.BlockBeforeSend(ctx, moduleAccAddr, delegatorAddr, amt) if err != nil { return err } + k.TrackBeforeSend(ctx, moduleAccAddr, delegatorAddr, amt) + err = k.subUnlockedCoins(ctx, moduleAccAddr, amt) if err != nil { return err @@ -441,6 +444,8 @@ func (k BaseKeeper) SendCoinsFromModuleToManyAccounts( // SendCoinsFromModuleToModule transfers coins from a ModuleAccount to another. // It will panic if either module account does not exist. +// SendCoinsFromModuleToModule is the only send method that does not call both BlockBeforeSend and TrackBeforeSend hook. +// It only calls the TrackBeforeSend hook. func (k BaseKeeper) SendCoinsFromModuleToModule( ctx sdk.Context, senderModule, recipientModule string, amt sdk.Coins, ) error { @@ -455,7 +460,7 @@ func (k BaseKeeper) SendCoinsFromModuleToModule( panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule)) } - return k.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt) + return k.SendCoinsWithoutBlockHook(ctx, senderAddr, recipientAcc.GetAddress(), amt) } // SendCoinsFromAccountToModule transfers coins from an AccAddress to a ModuleAccount. diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index 1cf2c48d7a45..9317bcb31f7b 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -143,16 +143,29 @@ func (k BaseSendKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input, return nil } +// SendCoinsWithoutBlockHook calls sendCoins without calling the `BlockBeforeSend` hook. +func (k BaseSendKeeper) SendCoinsWithoutBlockHook(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error { + return k.sendCoins(ctx, fromAddr, toAddr, amt) +} + // 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) + // BlockBeforeSend hook should always be called before the TrackBeforeSend hook. + err := k.BlockBeforeSend(ctx, fromAddr, toAddr, amt) if err != nil { return err } - err = k.subUnlockedCoins(ctx, fromAddr, amt) + return k.sendCoins(ctx, fromAddr, toAddr, amt) +} + +// sendCoins has the internal logic for sending coins. +func (k BaseSendKeeper) sendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error { + // call the TrackBeforeSend hooks + k.TrackBeforeSend(ctx, fromAddr, toAddr, amt) + + err := k.subUnlockedCoins(ctx, fromAddr, amt) if err != nil { return err } @@ -198,10 +211,13 @@ 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]) + k.TrackBeforeSend(ctx, fromAddr, toAddrs[i], amts[i]) + + err := k.BlockBeforeSend(ctx, fromAddr, toAddrs[i], amts[i]) if err != nil { return err } + totalAmt = sdk.Coins.Add(totalAmt, amt...) } diff --git a/x/bank/types/expected_keepers.go b/x/bank/types/expected_keepers.go index 270ff42115d2..858b2d2162a9 100644 --- a/x/bank/types/expected_keepers.go +++ b/x/bank/types/expected_keepers.go @@ -35,5 +35,6 @@ type AccountKeeper interface { // 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 + TrackBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) // Must be before any send is executed + BlockBeforeSend(ctx sdk.Context, from, 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 index 09d4296fdda2..2fa3b35a0bc5 100644 --- a/x/bank/types/hooks.go +++ b/x/bank/types/hooks.go @@ -12,10 +12,17 @@ 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 { +// TrackBeforeSend runs the TrackBeforeSend hooks in order for each BankHook in a MultiBankHooks struct +func (h MultiBankHooks) TrackBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) { for i := range h { - err := h[i].BeforeSend(ctx, from, to, amount) + h[i].TrackBeforeSend(ctx, from, to, amount) + } +} + +// BlockBeforeSend runs the BlockBeforeSend hooks in order for each BankHook in a MultiBankHooks struct +func (h MultiBankHooks) BlockBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) error { + for i := range h { + err := h[i].BlockBeforeSend(ctx, from, to, amount) if err != nil { return err } From 27bd206e3481c48122768cc55ab7dae1fff3fca5 Mon Sep 17 00:00:00 2001 From: "Matt, Park" <45252226+mattverse@users.noreply.github.com> Date: Wed, 8 Mar 2023 20:14:32 +0900 Subject: [PATCH 2/2] Update x/bank/keeper/hooks.go Co-authored-by: Nicolas Lara --- x/bank/keeper/hooks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/bank/keeper/hooks.go b/x/bank/keeper/hooks.go index 83d1e722cf71..028a9d01ce26 100644 --- a/x/bank/keeper/hooks.go +++ b/x/bank/keeper/hooks.go @@ -8,7 +8,7 @@ import ( // Implements StakingHooks interface var _ types.BankHooks = BaseSendKeeper{} -// TrackbeforeSend executes the TrackBeforeSend hook if registered. +// TrackBeforeSend executes the TrackBeforeSend hook if registered. func (k BaseSendKeeper) TrackBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) { if k.hooks != nil { k.hooks.TrackBeforeSend(ctx, from, to, amount)