Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add TrackBeforeSend , BlockBeforeSend hooks. Deprecate BeforeSend hook #421

Merged
merged 2 commits into from
Mar 9, 2023
Merged

feat: Add TrackBeforeSend , BlockBeforeSend hooks. Deprecate BeforeSend hook #421

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3716098
Add separated hooks for bank send
mattverse Mar 7, 2023
27bd206
Update x/bank/keeper/hooks.go
mattverse Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions x/bank/keeper/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
mattverse marked this conversation as resolved.
Show resolved Hide resolved
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
}
72 changes: 59 additions & 13 deletions x/bank/keeper/hooks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand All @@ -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{}
Expand All @@ -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++
}
15 changes: 10 additions & 5 deletions x/bank/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q unrelated to this PR: Do we know why Delegate/UndelegateCoins manages the coins directly instead of using SendCoins under the hood? Seems like it would be a better abstraction and the hooks could just be in SendCoins

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentially because of security issue, I have no ide. Good point tho

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 @@ -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 {
Expand All @@ -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.
Expand Down
24 changes: 20 additions & 4 deletions x/bank/keeper/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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...)
}

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