feat(bank): send-hooks #14660

neverDefined · 2023-01-17T22:45:48Z

Description

This PR implements before/after send hooks to the BaseSendKeeper. Other modules will be able to register hooks to this module.

Closes: #XXXX

x/bank/send.go
x/bank/types/hooks.go

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...

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)

alexanderbez · 2023-01-18T16:55:49Z

Is there an issue that describes the who/what/why?

neverDefined · 2023-01-19T15:43:58Z

Hey Alexander,

Who:
Im a developer on Berachain and some other defi DAOS.

Why:

Hooks at the bank level are the only way that App-chain developers can listen in on sendCoins actions and preform actions. By using the same light implementations as the staking-module its a no-op for developers that don't need them and makes the development of ones that need them much easier.

How:

Devs can implement the SendHooks interface in any other module, and register it using the bank.SetHooks method. The slice of sendHooks then will be ran after every sendCoins call.

julienrbrt · 2023-01-19T16:44:22Z

simapp/app.go

@@ -301,6 +301,11 @@ func NewSimApp(
 		BlockedAddresses(),
 		authtypes.NewModuleAddress(govtypes.ModuleName).String(),
 	)
+	app.BankKeeper.SetHooks(


If this proposal is accepted, the app wiring configuration should be done for bank hooks.
You can look at staking for guidance:

https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/x/staking/module.go#L199

https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/x/staking/module.go#L245-L282

https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/staking/module/v1/module.proto#L13-L16

sounds good wanted to get the teams opinion on the changes first. implementing that now.

Of course, this is why I commented:

If this proposal is accepted

Don't do it already, just in case :)

lmk when and if the team is looking to move forward with this ser. Have a good weekend sers

alexanderbez · 2023-01-19T18:17:49Z

@neverDefined can you please open that in an issue :p

neverDefined · 2023-01-19T19:00:11Z

@neverDefined can you please open that in an issue :p

made an issue: #14701

SpicyLemon · 2023-01-24T17:35:33Z

This looks related: #14224

neverDefined · 2023-01-30T12:40:28Z

This looks related: #14224

Would it make sense to merge the features, and run them right after the restrictions.

SpicyLemon · 2023-01-30T16:39:42Z

This looks related: #14224

Would it make sense to merge the features, and run them right after the restrictions.

I haven't really looked into this PR, but on the surface, this one and that one heavily overlap and have the same goal. The SDK team is having some discussions related to these things, but I'm not privy to them. #14224 is awaiting some decisions from those discussion.

What's a use case for needing separate pre and post send hooks? A failure from either would prevent the send. #14224 is analogous to the pre-send hooks in here.

I know that a lot of optimization has gone into bank sends since it's a crucial and often-used blockchain feature; keeping gas costs (and what they represent) low is desirable. It'd be good to keep things a simple as possible.

SpicyLemon · 2023-01-30T16:51:07Z

x/bank/keeper/send.go

@@ -141,6 +163,11 @@ func (k BaseSendKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input,
 			return err
 		}

+		// Run send hooks if present.
+		if err := k.Hooks().BeforeSendCoins(ctx, inAddress, inAddress, in.Coins); err != nil {


Providing the same to and from addresses like this, is not desirable. Same goes for the outputs loop below.

At best, the hook will think nothing is happening, but these calls are also telling lies. This call is telling the hook that in.Coins is going to be added to inAddress. In the outputs loop, it tells the hook that out.Coins is being removed from the outAddress. Neither are true.

yup first implem was to get the conversation going, probably need to sync the loop from the same index so you have n instead of 2n

tac0turtle · 2023-01-31T19:35:47Z

talked with @ValarDragon and there are edge cases to think through here. Osmosis did an audit on this work but never merged it

itsdevbear · 2023-02-13T01:51:03Z

@neverDefined follow up Q's

neverDefined · 2023-02-17T14:41:02Z

This looks related: #14224

Would it make sense to merge the features, and run them right after the restrictions.

I haven't really looked into this PR, but on the surface, this one and that one heavily overlap and have the same goal. The SDK team is having some discussions related to these things, but I'm not privy to them. #14224 is awaiting some decisions from those discussion.

What's a use case for needing separate pre and post send hooks? A failure from either would prevent the send. #14224 is analogous to the pre-send hooks in here.

I know that a lot of optimization has gone into bank sends since it's a crucial and often-used blockchain feature; keeping gas costs (and what they represent) low is desirable. It'd be good to keep things a simple as possible.

The real reason for send hooks is as you said, sending coins is one of the most used methods in the sdk, hence having a simple api to run logic that is closely tied with that feature could make sense.

re the sepiration of the hooks, for sure in most use cases, post send hooks make the most sense, but could be some niche use cases, such as a module needing to "deregister" from a service, creating a better binding from sender to logic.

neverDefined · 2023-02-17T14:51:52Z

l ,

talked with @ValarDragon and there are edge cases to think through here. Osmosis did an audit on this work but never merged it

yea definitely touches the core the of the sdk. But think the use for other app chains like perp-exchanges, dex's can really help with integrating some interesting features cleanly.

itsdevbear · 2023-02-20T17:43:54Z

Not having an "ontransfer" makes it difficult for using native coins for defi type composability, and forces chain developers to use non native solutions to implement this, causing additional complexity and eventually token / liquidity fragmentation.

Our main use case is to allow for rewards to be streamed to the tokenized vaults, in which our receipt tokens are actually native denoms. (We have a vault module that effectively wraps a token factory).

For passive accounting to work, if a user transfers the receipt token to another address, we need to trigger them claiming any pending rewards from the vault.

This is a very common use case in Ethereum DeFi.

tac0turtle · 2023-03-02T14:14:12Z

we need more discussion on this and most likely a design doc. I talked with osmosis about this change and they said there some gotchas with adding this feature. We should document and make sure the trade offs are something we are willing to accept.

Lets continue the discussion in the issue. #14701. Also would love to bring this discussion to the next community call.

send-hooks

18d32c2

github-actions bot added the C:x/bank label Jan 17, 2023

neverDefined changed the title ~~send-hooks~~ feat(bank): send-hooks Jan 17, 2023

julienrbrt reviewed Jan 19, 2023

View reviewed changes

neverDefined added 4 commits January 19, 2023 14:32

update proto

176f2d5

ref-types

ec95e17

update-bank-module.go

2978d0a

fix:replace-go-work

9116ba7

SpicyLemon reviewed Jan 30, 2023

View reviewed changes

tac0turtle closed this Mar 2, 2023

itsdevbear mentioned this pull request Mar 2, 2023

feat: Add PrepareCheckState and Precommit callbacks #14860

Merged

19 tasks

BrickBera mentioned this pull request May 19, 2023

feat(bank): AfterSendCoins Hooks in the bank module #16234

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bank): send-hooks #14660

feat(bank): send-hooks #14660

neverDefined commented Jan 17, 2023

alexanderbez commented Jan 18, 2023

neverDefined commented Jan 19, 2023 •

edited

Loading

julienrbrt Jan 19, 2023 •

edited

Loading

neverDefined Jan 19, 2023

julienrbrt Jan 19, 2023 •

edited

Loading

neverDefined Jan 20, 2023

alexanderbez commented Jan 19, 2023

neverDefined commented Jan 19, 2023

SpicyLemon commented Jan 24, 2023

neverDefined commented Jan 30, 2023

SpicyLemon commented Jan 30, 2023

SpicyLemon Jan 30, 2023

neverDefined Feb 17, 2023

tac0turtle commented Jan 31, 2023

itsdevbear commented Feb 13, 2023

neverDefined commented Feb 17, 2023

neverDefined commented Feb 17, 2023

itsdevbear commented Feb 20, 2023

tac0turtle commented Mar 2, 2023

feat(bank): send-hooks #14660

feat(bank): send-hooks #14660

Conversation

neverDefined commented Jan 17, 2023

Description

Author Checklist

Reviewers Checklist

alexanderbez commented Jan 18, 2023

neverDefined commented Jan 19, 2023 • edited Loading

julienrbrt Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

neverDefined Jan 19, 2023

Choose a reason for hiding this comment

julienrbrt Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

neverDefined Jan 20, 2023

Choose a reason for hiding this comment

alexanderbez commented Jan 19, 2023

neverDefined commented Jan 19, 2023

SpicyLemon commented Jan 24, 2023

neverDefined commented Jan 30, 2023

SpicyLemon commented Jan 30, 2023

SpicyLemon Jan 30, 2023

Choose a reason for hiding this comment

neverDefined Feb 17, 2023

Choose a reason for hiding this comment

tac0turtle commented Jan 31, 2023

itsdevbear commented Feb 13, 2023

neverDefined commented Feb 17, 2023

neverDefined commented Feb 17, 2023

itsdevbear commented Feb 20, 2023

tac0turtle commented Mar 2, 2023

neverDefined commented Jan 19, 2023 •

edited

Loading

julienrbrt Jan 19, 2023 •

edited

Loading

julienrbrt Jan 19, 2023 •

edited

Loading