Include BeforeSendCoins and AfterSendCoins Hooks in the bank module

[neverDefined]

Summary

Similar to the staking keeper, the bank keeper can benefit a lot by having hooks that other modules can register on send coins events. For developers that don't need it, its optional and doest require much overhead, and for developers that need this feature there is no better option than having it in the sdk itself instead of having to maintain a fork.

Problem Definition

What problems may be addressed by introducing this feature?

Allows developers to still use the SendKeeper to invoke transactions before and after a sendCoins event. Similar to staking modules AfterValidatorCreated and BeforeValidatorModified.

What benefits does the SDK stand to gain by including this feature?

Are there any disadvantages of including this feature? -->

Proposal

• update the bank send keeper and module to include BeforeSendCoins and AfterSendCoins events

[alexanderbez]

@ValarDragon did you guys end up using something similar in your SDK fork? Is that still useful?

[alexanderbez]

I have no opposition to this personally and am on board to include it!

But I must say that this and what staking does with hooks is sort of duct tape and doesn't scale well. Ideally, in the future, we can have a more robust and flexible system where modules subscribe and listen for certain typed events and act on them accordingly.

[itsdevbear]

@alexanderbez +1 on the more modular interface. We should engineer a notification based system.

i.e the Bank module emits a "CoinsSent" notification that other modules can subscribe to.

From my past life: https://developer.apple.com/documentation/DISPATCH

[itsdevbear]

@neverDefined is working to built transfer hooks for our tokenized vaults on Berachain, hence the motivation for this SDK change.

[SpicyLemon]

Edit: Sorry. This ended up being longer than I expected. I made a few shorter comments below for specific concerns discussed in this comment.

---

Related: #14224

That one goes one step further though, allowing the hook to alter the destination address.

At Provenance, in our fork of the SDK, we utilized something similar for the x/quarantine and x/sanction modules. We are also working on another enhancement that further restricts sends based on denom, sender, receiver, and other available on-chain information.

In all these cases, any time funds are transferred, we want some checks made first. That includes (but isn't limited to) use of the Send and MultiSend endpoints as well as other modules calling the keeper functions SendCoins or InputOutputCoins. In most cases, if the check fails, the transfer must not happen. With the x/quarantine module, the transfer can still happen, but to a holding account instead of the requested receiver (and it makes a record of that).

So far, the info needed for these checks is the sender, receiver, and amount being transferred. And the output is the receiver and an error:

// A SendRestrictionFn can restrict sends and/or provide a new receiver address.
type SendRestrictionFn func(ctx sdk.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (newToAddr sdk.AccAddress, err error)

I previously created an issue to discuss the quarantine and sanction modules: #14124. It's not exactly what we ended up going with at Provenance though. Our fork is based off v0.46 which still allows multiple inputs with MultiSend. That makes generic restriction functions/hooks more difficult because there's now two different sets of arguments to provide. We're currently injecting new keepers into the bank module, and having the bank module make the specific needed calls. But that's not an option without forking the SDK. We'd much rather just inject restriction functions into the bank keeper and use the standard SDK's bank module.

#12601 (not in v0.46) restricted MultiSend to a single input , but not InputOutputCoins. I feel like the PRs for it weren't handled well, and got messed up a bit. An initial PR was merged that had a breaking change in the proto. So it was fully reverted, then it looks like parts of it were cherry-picked and re-PRd. The final changes failed to fulfill the goal of the issue: simplification. Primarily, InputOutputCoins still allows for multiple inputs (contrary to the original PR), so none of the logic actually got simplified.

If InputOutputCoins is limited to a single input, the restriction functions (of #14224) can also be used in there, called once for each output. Without that, there would either need to be two hooks, or the one hook would need to take in either varargs senders (hook called in the outputs loop) or varargs receivers (hook called in the inputs loop). In the case of the quarantine module, it needs to know all senders and treat each receiver individually. I'm not aware of a use case that either requires knowing all receivers and treating each sender individually, or one that requires knowing all inputs and outputs at once. But with InputOutputCoins as is, that's something that would need to be addressed.

If there's a need to have different hooks before and after a transfer, InputOutputCoins becomes even more of a problem. I don't feel that's an actual need though. They might be useful if the hook needs to react to a balance, but with a single hook, the other balance can be looked up and calculated, so it's not a need. The only other time (that I can think of) where separate before and after hooks might matter is for sends that happen outside a tx; a failure on either would roll a tx back, preventing the transfer/tx, so it's no different than having just one hook. I'm not fully aware of what transfers happen outside a tx, though.

[SpicyLemon]

Why have separate before/after hooks?

One hook is simpler than two.

The only state difference is balances. With one hook, the other balances can easily be calculated, though. So separate hooks aren't actually needed in that case.

Most transfers happen inside a tx. An error from either hook would roll back the entire tx. So separate hooks aren't actually needed in this case either.

What transfers happen outside a tx that might benefit from having separate before/after hooks?

[SpicyLemon]

The InputOutputCoins function needs to be considered here as well.

[SpicyLemon]

One of our use cases needs the BeforeSendCoins hook to possibly change the destination address. I.e. we'd need that function to return (sdk.AccAddress, error), where the address it returns becomes the actual receiver.

A no-op for it would look like this:

func BeforeSendCoins(ctx sdk.Context, sender, receiver sdk.AccAddress, amt sdk.Coins) (newToAddr sdk.AccAddress, err error) {
    return receiver, nil
}

[Kingjames52-bot]

Okay I am really enjoying this
Development 👍👀🇺🇸

[chandiniv1]

Is this issue still open? Can I work on it @neverDefined

[itsdevbear]

@BrickBera @neverDefined

[SpicyLemon]

There's already this PR: #14224 which effectively adds a BeforeSendCoins hook, but with the added ability to change the destination address. And again, there isn't any reason to have separate before/after hooks.