feat: add a BeforeSend hook to the bank module

[sunnya97]

Description

This PR adds a hook point to before all sends in the bank module.

It makes sure to trigger the hook in the following Bank keeper functions:

• SendCoins
• SendManyCoins
• SendCoinsFromAccountToModule
• SendCoinsFromModuleToAccount
• SendCoinsFromModuleToModule
• SendCoinsFromModuleToManyAccounts
• DelegateCoins
• UndelegateCoins

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[mattverse]

I'm wondering if we don;t need to call SetHooks in app.go?

[sunnya97]

Don't we only need to call SetHooks if we're setting some hooks? There's nothing in the cosmos-sdk repo that makes use of these hooks. In the upstream osmosis, we will add some

[ValarDragon]

Lets please add these hooks != nil check in the hooks struct itself, and not in this logic.

[ValarDragon]

Unless this is the standard for the rest of bank. If so, lets make a github issue for it to be fixed

[ValarDragon]

Yeah no need to set hooks, as long as theres nil guards in the hooks structs

[sunnya97]

How can we add nil guards to the hook structs if what we're checking if the hook struct itself is nil?

Other modules solve it by adding keeper functions for each hook, wrapping them and checking if hooks exist

https://github.com/osmosis-labs/cosmos-sdk/blob/osmosis-main/x/staking/keeper/hooks.go

Imo, this is uglier and "more boilerplate-y", but I can update to that to be standard with the rest of the sdk.

[p0mvn]

Looks great overall, I like the approach for testing with the mock.

I think having a separate case with a valid amount for all methods would be good

[sunnya97]

Why would we package the calling of the before send hook with subUnlockedCoins? These seem to be two very different functions. The BeforeSend hook needs to have a recipient address, and feels weird to be passing that into a function focused on subtracting coins

[p0mvn]

Why would we package the calling of the before send hook with subUnlockedCoins? These seem to be two very different functions. The BeforeSend hook needs to have a recipient address, and feels weird to be passing that into a function focused on subtracting coins

#278 (comment)

[p0mvn]

LGTM once linter and tests are happy