Skip to content

Commit

Permalink
feat: Add TrackBeforeSend , BlockBeforeSend hooks. Deprecate `Bef…
Browse files Browse the repository at this point in the history 
…oreSend` hook (#421)

* Add separated hooks for bank send

* Update x/bank/keeper/hooks.go

Co-authored-by: Nicolas Lara <[email protected]>

---------

Co-authored-by: Nicolas Lara <[email protected]>
  • Loading branch information
@mattverse @nicolaslara @czarcas7ic
2 people authored and czarcas7ic committed May 9, 2024
1 parent 058afdd commit 937c133
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 29 deletions.
71 changes: 59 additions & 12 deletions x/bank/hooks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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(),
Expand All @@ -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{}
Expand All @@ -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++
}
13 changes: 10 additions & 3 deletions x/bank/keeper/hooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
15 changes: 10 additions & 5 deletions x/bank/keeper/keeper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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.
Expand Down
26 changes: 21 additions & 5 deletions x/bank/keeper/send.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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...)
}

Expand Down
3 changes: 2 additions & 1 deletion x/bank/types/expected_keepers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
13 changes: 10 additions & 3 deletions x/bank/types/hooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down

0 comments on commit 937c133

Please sign in to comment.