Hooks and reactivity

[PaulRBerg]

While writing my feedback to #150, it dawned on me what is the solution to the problems raised by Astaria.

It's to make it possible for third-party contracts to "react" to Sablier stream interactions, i.e. cancelations and withdrawals, via cross-contract hooks.

How could this work, in practice? There are a couple of ways I can think of; both would require the drafting of an IERCSablierResponder interface with two functions onCancelHook and onWithdrawHook.

1. We attempt to call onCancelHook at the bottom of the _cancel function on the recipient itself, but use something like try/catch to not revert if the recipient does not implement the hook.
2. We add a responder field in the stream struct and check if this is defined and if it is, we call the onCancelHook function on the responder.

Both should cost the same in terms of gas, on average, though the second approach would cost more when setting the responder field due to the storage gas overhead.

Needless to say that by doing this, Astaria could easily react to whatever happens to the streams, and update their internal state accordingly (e.g. account for the fact that ERC-20 tokens have been transferred in).

[PaulRBerg]

I wrote an issue that implements this idea in #164.

Two questions that came to mind:

1. Should we pass a specific gas stipend to the recipient? (e.g. see what I do in PRBProxy)
2. Should we add a simple enum with the "type" or "class" of streams available in Sablier V2 (currently only linear and pro, but who knows what in future), and pass this as an argument to the recipient hooks? The recipient smart contract might want to know if it is interacting with what kind of stream is interacting.

[razgraf]

I dig this proposal mostly due to callbacks simplifying how our contracts can be extended, thus providing means to scale our systems.

1. Question: Will we support callbacks for ERC721 as well (onTransfer)?
2. Note: As you mentioned in ISablierV2Recipient: sender and recipient hooks #164, one has to be extremely careful with callbacks. They are an opportunity for an outside system to mess with ours (example of reentrancy attack) so we should definitely employ some guards for extra security. Follow-up question: will these guards play well with the flash loan logic?

---

Bit of nitpicking but this is not exactly a solution to address the problems raised by Astaria. Those discussions revolved around limiting what the funds from an NFT can do and when, thus lowering the work required to support complex behavior. This solution keeps the dynamics of funds intact, while adding some quality UX on top to allow integrators to react to changes. It improves the current system without replacing it.

[PaulRBerg]

Thanks, glad you like it. I think you meant to say "integrated"? I don't see how hooks make inheritance any easier.

1. I don't see why we would. The point with the _beforeTokenTransfer and the _afterTokenTransfer hooks is to react to a token transfer as an ERC-721 contract developer in case you need to perform some actions (we don't), not as an integrator of the contract thereof. Lending protocols like Astaria would have ownership of the NFT, so they control how and when the NFT is transferred.
2. Given that we are following the Checks, Effects, Interactions pattern quite stringently, there is no attack vector introduced by callbacks (I added them at the end of the functions in my reference implementation). However, it's possible that a callback makes it possible to call withdraw recursively - a behavior that is not a bug, but which should be accounted for in the tests explicitly, proving that it is not a bug.

Bit of nitpicking but this is not exactly a solution to address the problems raised by Astaria

This is true but, as you said, with callbacks we will provide integrators with a superior DX. Instead of having to add a bunch of if statements to compare the previous withdrawable balances to the current one, an integrator can just perform the state changes atomically in the hook functions.

As I mentioned in my first comment in #150, we have gotten to a point where optimizing for one use case brings a trade-off for some other use case. Skipping the withdrawal part in the cancel function would have been a huge pain in the ass for a great many users.

[razgraf]

By the way, why would calling withdraw recursively be something we allow?

If we're not implementing features such as Flash Loans, shouldn't we try to enforce Reentrency Guards as much as possible?

[PaulRBerg]

@razgraf the simple answer is "to save gas".

I don't see the point in adding the reentrancy guard if reentrancy cannot do any harm (there are other situations in which reentrancy simply must be added).

If we write a test in which recursive withdrawals can be made successfully, and there is no malicious exploit that comes with it, then I would be in favor of not adding the guard.

[PaulRBerg]

@andreivladbrg asked an interesting question in #164 (comment), which has led me to consider a new feature - what if we attempted to call the hook on the sender, but only when cancelations are made?

The sender could be a smart contract (e.g. a Safe multisig or some sort of protocol that creates streams on behalf of users), so they may be interested to know when cancelations are made (though I don't think they would care about withdrawals, since it's not their money anymore).

Cc @razgraf, would love your two cents on this.

[PaulRBerg]

Cc @razgraf we would like to start working on this next week, do you think you can give us the green light no later than tomorrow?

[razgraf]

Just to be clear, you're saying we would have 3 hooks:

1. recipient.onWithdraw
2. recipient.onCancel
3. sender.onCancel

I don't yet see a clear use-case for the sender onCancel hook, but it might be worth having it. Smart contract wallets could make use of this, as well as any custom contracts that might wrap cancelable streams. Question: what's the order for execution, recipient hook then sender hook or the other way around?

Another question: what's the gas impact of these hooks. 95% of streams will probably not execute the hook branches. Cancellation is a one-time deal, but withdraw happens quite often. Do we anticipate any meaningful impact for that external call especially on withdraw?

I'm in favor of adding these hooks (even all 3 of them), as long as they don't impact our performance or security long term (at least not in an obvious way).

[PaulRBerg]

Yes, that's right, our proposal is to have the three hooks that you have enumerated.

The order of execution doesn't really matter, because there is no data passed between the hooks, so it is not possible for the callees to respond to one another. I suggest we keep the same order as for withdrawals and call the recipient hook first, for consistency.

Regarding gas impact, it shouldn't be high for non-compliant recipients. As per the evm.codes website, the cost of a vanilla CALL is 100 gas. But this is something that @andreivladbrg should confirm when we creates the PR for this feature.

Regarding security. Because we are adding these hooks at the very bottom of the cancel and withdraw functions, there are no potential recursion issues. Of course, @andreivladbrg will have to write tests to validate this assumption, but as far as I can tell, we're good to go from this perspective.

[PaulRBerg]

Locking, since it's been implemented via #193.