From 937c133936be12d57adc35aa1cd4d23a21405e48 Mon Sep 17 00:00:00 2001 From: "Matt, Park" <45252226+mattverse@users.noreply.github.com> Date: Thu, 9 Mar 2023 18:00:04 +0900 Subject: [PATCH] feat: Add `TrackBeforeSend` , `BlockBeforeSend` hooks. Deprecate `BeforeSend` hook (#421) * Add separated hooks for bank send * Update x/bank/keeper/hooks.go Co-authored-by: Nicolas Lara --------- Co-authored-by: Nicolas Lara --- x/bank/hooks_test.go | 71 ++++++++++++++++++++++++++------ x/bank/keeper/hooks.go | 13 ++++-- x/bank/keeper/keeper.go | 15 ++++--- x/bank/keeper/send.go | 26 +++++++++--- x/bank/types/expected_keepers.go | 3 +- x/bank/types/hooks.go | 13 ++++-- 6 files changed, 112 insertions(+), 29 deletions(-) diff --git a/x/bank/hooks_test.go b/x/bank/hooks_test.go index 6f0983bc7a1d..efb4aad722c7 100644 --- a/x/bank/hooks_test.go +++ b/x/bank/hooks_test.go @@ -23,8 +23,8 @@ 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 context.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 context.Context, from, to sdk.AccAddress, amount sdk.Coins) error { for _, coin := range amount { if coin.Amount.Equal(math.NewInt(100)) { return fmt.Errorf("not allowed; expected %v, got: %v", 100, coin.Amount) @@ -33,6 +33,21 @@ func (h *MockBankHooksReceiver) BeforeSend(ctx context.Context, from, to sdk.Acc 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 context.Context, from, to sdk.AccAddress, amount sdk.Coins) { + for _, coin := range amount { + if coin.Amount.Equal(math.NewInt(50)) { + countTrackBeforeSend += 1 + } + } +} + func TestHooks(t *testing.T) { acc := &authtypes.BaseAccount{ Address: addr1.String(), @@ -49,7 +64,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(sdk.DefaultBondDenom, math.NewInt(1))) - invalidSendAmount := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100))) + triggerTrackSendAmount := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(50))) + invalidBlockSendAmount := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100))) // setup our mock bank hooks receiver that prevents the send of 100 coins bankHooksReceiver := MockBankHooksReceiver{} @@ -64,47 +80,78 @@ func TestHooks(t *testing.T) { err := app.BankKeeper.SendCoins(ctx, addr1, addr2, validSendAmount) require.NoError(t, err) + // try sending an trigger track send amount and it should work + err = app.BankKeeper.SendCoins(ctx, addr1, addr2, 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, addr1, addr2, invalidSendAmount) + err = app.BankKeeper.SendCoins(ctx, addr1, addr2, 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, addr1, []sdk.AccAddress{addr1, addr2}, []sdk.Coins{invalidSendAmount, validSendAmount}) + err = app.BankKeeper.SendManyCoins(ctx, addr1, []sdk.AccAddress{addr1, addr2}, []sdk.Coins{invalidBlockSendAmount, validSendAmount}) require.Error(t, err) + err = app.BankKeeper.SendManyCoins(ctx, addr1, []sdk.AccAddress{addr1, addr2}, []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, addr1, stakingtypes.BondedPoolName, validSendAmount) require.NoError(t, err) - err = app.BankKeeper.SendCoinsFromAccountToModule(ctx, addr1, stakingtypes.BondedPoolName, invalidSendAmount) + err = app.BankKeeper.SendCoinsFromAccountToModule(ctx, addr1, stakingtypes.BondedPoolName, invalidBlockSendAmount) require.Error(t, err) + err = app.BankKeeper.SendCoinsFromAccountToModule(ctx, addr1, 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, addr1, validSendAmount) require.NoError(t, err) - err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, stakingtypes.BondedPoolName, addr1, invalidSendAmount) + err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, stakingtypes.BondedPoolName, addr1, invalidBlockSendAmount) require.Error(t, err) + err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, stakingtypes.BondedPoolName, addr1, 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{addr1, addr2}, []sdk.Coins{validSendAmount, validSendAmount}) require.NoError(t, err) - err = app.BankKeeper.SendCoinsFromModuleToManyAccounts(ctx, stakingtypes.BondedPoolName, []sdk.AccAddress{addr1, addr2}, []sdk.Coins{validSendAmount, invalidSendAmount}) + err = app.BankKeeper.SendCoinsFromModuleToManyAccounts(ctx, stakingtypes.BondedPoolName, []sdk.AccAddress{addr1, addr2}, []sdk.Coins{validSendAmount, invalidBlockSendAmount}) require.Error(t, err) + err = app.BankKeeper.SendCoinsFromModuleToManyAccounts(ctx, stakingtypes.BondedPoolName, []sdk.AccAddress{addr1, addr2}, []sdk.Coins{validSendAmount, triggerTrackSendAmount}) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ // make sure that DelegateCoins doesn't bypass the hook err = app.BankKeeper.DelegateCoins(ctx, addr1, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), validSendAmount) require.NoError(t, err) - err = app.BankKeeper.DelegateCoins(ctx, addr1, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), invalidSendAmount) + err = app.BankKeeper.DelegateCoins(ctx, addr1, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), invalidBlockSendAmount) require.Error(t, err) + err = app.BankKeeper.DelegateCoins(ctx, addr1, 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), addr1, validSendAmount) require.NoError(t, err) - err = app.BankKeeper.UndelegateCoins(ctx, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), addr1, invalidSendAmount) + err = app.BankKeeper.UndelegateCoins(ctx, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), addr1, invalidBlockSendAmount) require.Error(t, err) + + err = app.BankKeeper.UndelegateCoins(ctx, app.AccountKeeper.GetModuleAddress(stakingtypes.BondedPoolName), addr1, triggerTrackSendAmount) + require.Equal(t, countTrackBeforeSend, expNextCount) + expNextCount++ } diff --git a/x/bank/keeper/hooks.go b/x/bank/keeper/hooks.go index c0006c94da93..00f1d7ef9c84 100644 --- a/x/bank/keeper/hooks.go +++ b/x/bank/keeper/hooks.go @@ -10,10 +10,17 @@ import ( // Implements StakingHooks interface var _ types.BankHooks = BaseSendKeeper{} -// BeforeSend executes the BeforeSend hook if registered. -func (k BaseSendKeeper) BeforeSend(ctx context.Context, from, to sdk.AccAddress, amount sdk.Coins) error { +// TrackBeforeSend executes the TrackBeforeSend hook if registered. +func (k BaseSendKeeper) TrackBeforeSend(ctx context.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 context.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/keeper.go b/x/bank/keeper/keeper.go index b341416c2c59..6f6f5d5c16b8 100644 --- a/x/bank/keeper/keeper.go +++ b/x/bank/keeper/keeper.go @@ -144,11 +144,12 @@ func (k BaseKeeper) DelegateCoins(ctx context.Context, delegatorAddr, moduleAccA return errorsmod.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() @@ -199,12 +200,14 @@ func (k BaseKeeper) UndelegateCoins(ctx context.Context, moduleAccAddr, delegato return errorsmod.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 @@ -318,6 +321,8 @@ func (k BaseKeeper) SendCoinsFromModuleToManyAccounts(ctx context.Context, sende // 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 context.Context, senderModule, recipientModule string, amt sdk.Coins, ) error { @@ -331,7 +336,7 @@ func (k BaseKeeper) SendCoinsFromModuleToModule( panic(errorsmod.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 ecfe323d4799..235c081a0fbe 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -216,16 +216,29 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, return nil } +// SendCoinsWithoutBlockHook calls sendCoins without calling the `BlockBeforeSend` hook. +func (k BaseSendKeeper) SendCoinsWithoutBlockHook(ctx context.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 context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error { - // call the BeforeSend hooks - err := k.BeforeSend(ctx, fromAddr, toAddr, amt) +func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error { + // 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 context.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 } @@ -279,10 +292,13 @@ func (k BaseSendKeeper) SendManyCoins(ctx context.Context, fromAddr sdk.AccAddre 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 099b5edb704f..ef251be18aeb 100644 --- a/x/bank/types/expected_keepers.go +++ b/x/bank/types/expected_keepers.go @@ -42,5 +42,6 @@ type AccountKeeper interface { // BankHooks event hooks for bank sends type BankHooks interface { - BeforeSend(ctx context.Context, from sdk.AccAddress, to sdk.AccAddress, amount sdk.Coins) error // Must be before any send is executed + TrackBeforeSend(ctx context.Context, from, to sdk.AccAddress, amount sdk.Coins) // Must be before any send is executed + BlockBeforeSend(ctx context.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 05a269b00fbc..6f76208ef1d0 100644 --- a/x/bank/types/hooks.go +++ b/x/bank/types/hooks.go @@ -14,10 +14,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 context.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 context.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 context.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 }