ERC-3156 flash loans

[PaulRBerg]

Implements #246 by strictly following ERC-3156 and the advice I have given in this comment.

For context - the author of the EIP still recommends using it, and I am in very much in favor of that.

The benefits of complying with the EIP are:

1. Instant compatibility with all existing ERC-3156 implementations
2. Reduced documentation and marketing costs

[andreivladbrg]

Why didn't you check if the {SablierV2Comptroller-flashFee} is greater than the MAX_FEE in the {SablierV2FlashLoan-flashLoan}?

If you created a dedicated dir for the flash loan functions tests, why not move the SablierV2 tests (claimProtocolRevenues, getProtocolRevenues, setComptroller) from shared dir to a dedicated one aswell?

For context - the author of the EIP still recommends using it, and I am in very much in favor of that.

Why wouldn't he? It doesn't mean that it is the best version that can be implemented of the flash loan functionality.

The benefits of complying with the EIP are:

I am going to name some cons too:

• Larger contract size: CALLBACK_SUCCESS constant flashFee function and maxFlashLoan function, they are not mandatory to be able to make the functionality work.
• Contradictories regarding on how we write code in our contracts: address instead of IERC20, uint256 instead of uint128.
• Close doors to customizations / personalizations: implementing the idea on my comment would not follow the EIP.

But let's not debate this anymore, we are gonna follow the EIP.

[PaulRBerg]

Thanks for the review, @andreivladbrg.

Why didn't you check if the {SablierV2Comptroller-flashFee} is greater than the MAX_FEE?

Good question. My rationale was based on the following reasons:

1. No damage to the user experience. We want an upper limit to the fees charged on the streams because that is a critical part of the protocol. We refer to Sablier as a "streaming protocol" - if streams can't be created because the fee is too high, then we're in trouble. On the flip side, flash loans are an economic "add-on" for the Sablier protocol, something that is plugged in as a nice-to-have feature that may or may not generate revenues. Charging an exorbitantly high fee in this context wouldn't hurt the user experience of the core protocol - it wouldn't only hurt us, financially, because nobody would use Sablier for flash loaning, and thus we wouldn't earn any fees.
2. Purpose overuse - we are already using the MAX_FEE as an upper limit for the protocol and the broker fees, which are both applied to the stream's grossDepositAmount. It just didn't feel right to me to assign yet another purpose to this constant.

If you created a dedicated dir for the flash loan functions tests, why not move the SablierV2 tests (claimProtocolRevenues, getProtocolRevenues, setComptroller) from shared dir to a dedicated one, as well?

Again, good question. The answer is simple, but technical. The mechanism by which the protocol revenues are generated is stream-dependent - by simply inheriting the abstract contract SablierV2 (or rather a mock thereof), no protocol revenues can be generated, ever. Therefore, it only makes sense to test the revenues function in the context of the SablierV2LockupLinear and SablierV2LockupPro contracts.

Nonetheless, what might be useful is to create a dedicated directory for the setComptroller function, and maybe also the constructor of SablierV2 (which would address your point here). We would have to reintroduce a SablierV2Mock contract, though, to do this.

Why wouldn't he?

I know Alberto for 3+ years already. He's working at Yield, which is (was?) Hifi's main competitor.

Alberto is a top-notch software engineer, with a sense of intellectual honesty. If the ERC standard wasn't useful anymore, he would have said it.

name some cons too:

I don't think "cons" is a good word to use in this context - instead I would call of those points the "costs" for implementing the ERC. And they are fine to me. A 32 bytes constant doesn't cost much bytecode. The lack of consistency is easily handled by separation of concerns (the separate SablierV2FlashLoan contract, and thorough explanations - since the NatSpec documentation for the flashFee function!). Regarding consistency, what matters is to be internally (in the core protocol logic) consistent and also cohesive - which we are.

[andreivladbrg]

No damage to the user experience. We want an upper limit to the fees charged on the streams because that is a critical part of the protocol. We refer to Sablier as a "streaming protocol" - if streams can't be created because the fee is too high, then we're in trouble. On the flip side, flash loans are an economic "add-on" for the Sablier protocol, something that is plugged in as a nice-to-have feature that may or may not generate revenues. Charging an exorbitantly high fee in this context wouldn't hurt the user experience of the core protocol - it wouldn't only hurt us, financially, because nobody would use Sablier for flash loaning, and thus we wouldn't earn any fees.
Purpose overuse - we are already using the MAX_FEE as an upper limit for the protocol and the broker fees, which are both applied to the stream's grossDepositAmount. It just didn't feel right to me to assign yet another purpose to this constant.

Fair enough, don't have anything to add here.

Again, good question. The answer is simple, but technical. The mechanism by which the protocol revenues are generated is stream-dependent - by simply inheriting the abstract contract SablierV2 (or rather a mock thereof), no protocol revenues can be generated, ever. Therefore, it only makes sense to test the revenues function in the context of the SablierV2LockupLinear and SablierV2LockupPro contracts.

Same for the flash loans, if there are no streams created, there is no liquidity in the contracts.

Nonetheless, what might be useful is to create a dedicated directory for the setComptroller function, and maybe also the constructor of SablierV2 (which would address your point #271 (review)). We would have to reintroduce a SablierV2Mock contract, though, to do this.

No, we shouldn't reintroduce the mock, the constructor must be tested in the Constructor_Linear_Test and Constructor_Pro_Test.

[PaulRBerg]

Same for the flash loans, if there are no streams created, there is no liquidity in the contracts.

Nope, but I see that my formulation was ambiguous. What I meant to say is that the values in the _protocolRevenus mapping can be increased only by calling some protocol function, such as createWithRange.

On the other hand, in the case of the flash loan contract, we can simply use the deal cheatcode to mint tokens to the contract:

https://github.com/sablierhq/v2-core/blob/632ef3a6f3f413bf21f0e1e6bca2a0ef6ba3209b/test/unit/flash-loan/flash-loan/flashLoan.t.sol#L168

the constructor must be tested in the Constructor_Linear_Test and Constructor_Pro_Test

Ah, so this is what you meant! Ok, thanks for clarifying. Let's do this later though, there's no rush to do in this PR (doing this would trigger a lot of git conflicts down the line).

I will create an issue about this because I realize that we haven't tested something super important in the constructor - that the initialAdmin is set correctly (a constructor arg introduced in #262).

[PaulRBerg]

@andreivladbrg, here are some concrete figures for the statement that you made in the first comment:

Larger contract size: CALLBACK_SUCCESS constant

I removed this constant from the SablierV2FlashLoan contract, and the bytecode saving is a whopping .. wait for it .. ~0.038kB 😅

[andreivladbrg]

On the other hand, in the case of the flash loan contract, we can simply use the deal cheatcode to mint tokens to the contract

Fair enough.

I realize that we haven't tested something super important in the constructor

All the args have to be tested: initialAdmin initialComptroller maxFee

I removed this constant from the SablierV2FlashLoan contract, and the bytecode saving is a whopping .. wait for it .. ~0.038kB

It may not be that much, but it is unnecessary, would've been much easier to check for a bool.

Looks like it can be merged

[PaulRBerg]

Thanks for your final review.

All the args have to be tested: initialAdmin initialComptroller `maxFee

Yup, tested all of them in #284.

would've been much easier to check for a bool

It would have even easier to just not return anything, but EIPs are not modifiable once finalized.