Segment[] array, comptroller, protocol and operator fees, structified parameters, via IR, PRBMath types, major tests refactor

[PaulRBerg]

Description

Implements #11, #22, #117, #202, #203, #206, #207, #208, #209, #210, #211, #214, #217, #218, #219, #223, #225, #226, #230, #231, #236, #237, and #238.

This is the biggest PR that I have ever created in this repository. Documenting the minutiae of all new features and changes I made would be impractical, so I listed only the most significant stuff below.

New Features

Comptroller

We now have a SablierV2Comptroller contract. See #206.

Protocol Fees

We now have protocol fees (#207).

Operator Fees

We now have operator fees in all create functions (#208).

I opted for not implementing separate special functions that have operator fees.

PRBMath Types

New file types/MathTypes that re-exports PRBMath types (#202).

Coverage in CI

See #210 and #217.

Changes

Production

• Bundle the cliffDuration and the totalDuration parameters into a Durations struct (Add a "Durations" struct and use it in "createWithDuration" #236)
• Cluster common arguments in the create functions (Cluster common parameters in the create functions #203)
• Get rid of the stopTime storage property (Get rid of the "stopTime" storage property in the pro contract #225)
• Move DataTypes to dedicated types directory, and rename it to Structs, and also delete the library (import structs as is)
• Move hook interfaces to dedicated hooks directory
• Rename the cancelable property to isCancelable (Rename "cancelable" to "isCancelable" in the "Stream" struct #214)
• Rename cancelAll to cancelMultiple and withdrawAll to withdrawMultiple (Rename "cancelAll" to "cancelMultiple" and "withdrawAll" to "withdrawMultiple" #219)
• Rename createWithDuration to createWithDurations (Add a "Durations" struct and use it in "createWithDuration" #236)
• Rename withdrawAmount to recipientAmount, and returnAmount to senderAmount (Rename "withdrawAmount" to "recipientAmount", and "returnAmount" to "senderAmount" #226)
• Rename the create functions
  • Rename create to createWithRange in SablierV2Linear
  • Rename create to createWithMilestones in SablierV2Pro
  • Rename createWithDuration to createWithDeltas in SablierV2Pro
• Structify function arguments
  • Bundle protocolFeeAmount, operatorFeeAmount, and netDepositAmount in CreateAmounts struct
  • Bundle startTime, cliffTime, and stopTime in Range struct
  • Bundle segmentAmounts, segmentExponents, and segmentMilestones into a single array of Segment[] (Bundle "segmentAmounts", "segmentExponents" and "segmentMilestones" into a single array of structs #223)
• Use UD60x18.wrap instead of toUD60x18, and SD59x18.wrap instead of toSD59x18 (Use "UD60x18.wrap" instead of "toUD60x18" in "getWithdrawableAmount" #230 and Use "SD59x18.wrap" instead of "toSD59x18" in "getWithdrawableAmount" #231)

Tests

• I added CreateStreamArgs and DefaultArgs structs to improve the readability of the arguments passed in the create function tests.
• I added tests for expecting calls to the ERC-20 contracts (Write tests that expect calls to ERC-20 contracts #209).
• I dryified all tests (DRY-ify the tests #218), which means that we now have test coverage for the _cancel and _withdraw functions of the SablierV2Pro contract (Cover the "_cancel" and "_withdraw" internal functions of "SablierV2Pro" #117)
• I fuzzed all function arguments that I could fuzz (Write fuzz tests #22). The only feature that proved intractable for fuzzing were the segment arrays in the pro contract - I may open an issue or a discussion to document my findings later on.
  • By fuzzing arguments, I was able to significantly simplify the complexity of the testing trees. Basically, multiple branches merged into a single fuzzing branch.
• I merged many test leaves into one leaf, in order to avoid repeating myself during the initialization of the test. A case in point example is the leaf for the createWithRange test.
• I removed the unit tests that were creating streams with USDC, because I realized that we don't actually need to test tokens with different decimals. We are never actually pulling the token decimals in our code base - what we need to test is simply different deposit amounts, which are testing now.
  • A corollary of this (and the fuzzing) is that we can consider issue Check that "getWithdrawableAmount" works with non-18-decimal tokens #11 closed now, because we are now fuzzing the deposit amounts.
• Our CI set up now compiles and tests the production code with --via-ir enabled (Follow Seaport's Foundry set-up to enable "--via-ir" but avoid it when running tests #217)
• The test contracts are all deployed from the base test contract BaseTest. There is no UnitTest contract anymore.
• The test contracts are all labeled now.
• The test contracts are named just linear instead of sablierV2Linear and pro instead of sablierV2Pro.
• Using the solarray library instead of our custom createDynamicArray helper functions (Ditch the "createDynamicArray" helpers in favor of the "solarray" library #211). I ended up forking solarray and using my fork in our repo, because the canonical repo was lacking many features that we needed (uint40, PRBMath types, etc.)

[PaulRBerg]

Regarding params - TIL! Thanks for the link. I agree that we should refactor args to params in the internal create functions.

For better understanding the only arguments we have are

Not sure what you meant by this, but you can explain offline.

[andreivladbrg]

To be more efficient I am going to submit this review without getting in detail on tests, but I liked the shared pattern to get rid of the duplicated tests.

Regarding structs I suggest to keep them in their dedicated file and separate them with headers, memory / storage

To solve the size issue in the pro contract is mandatory to move the _calculateWithdrawableAmountForOneSegment and the _calculateWithdrawableAmountForMultipleSegments functions in Helpers lib.

[andreivladbrg]

Lets merge this with Math.sol and also add the hooks interfaces, and ofc we have to rename the file.

[PaulRBerg]

No, I am against merging these types with the hooks interfaces. The goal with the Math.sol and the Tokens.sol files is to re-export external types so that users don't have to manually install third-party repos for using Sablier. Whereas the hooks are internal.

At a minimum, I may be open to merging the Math.sol and the Tokens.sol files in one file - but I can't think of any good name for this merged file. Types.sol would be ugly, because of the types/Types.sol path.

[andreivladbrg]

Create a dir called re-exports and add all of them there.

[PaulRBerg]

No, I don't like how that sounds, not at all. End users shouldn't know that these are re-exported types.

Let's sleep on this idea for a while, we will come back to it later. If you have a strong opinion, feel free to create a discussion for tracking.

[andreivladbrg]

The main point is if a user wants to import all of them they must have 3 separeted paths instead of one:

import { X } from "path/File1.sol";
import { Y } from "path/File2.sol";
import { Z } from "path/File3.sol";

vs

import { X, Y, Y } from "path/File.sol";

[andreivladbrg]

Exports.sol

// SPDX-License-Identifier: LGPL-3.0
pragma solidity >=0.8.13;

/*//////////////////////////////////////////////////////////////////////////
                                    MATH
//////////////////////////////////////////////////////////////////////////*/

/// All PRBMath types needed in v2-core. It is provided for convenience to
/// users so that they don't have to install PRBMath separately.

import { SD1x18 } from "@prb/math/SD1x18.sol";
import { SD59x18 } from "@prb/math/SD59x18.sol";
import { UD60x18 } from "@prb/math/UD60x18.sol";

/*//////////////////////////////////////////////////////////////////////////
                                    TOKENS
//////////////////////////////////////////////////////////////////////////*/

/// All token interfaces needed in v2-core. It is provided for convenience to users so that they don't
/// have to install openzeppelin-contracts and prb-contracts separately.

import { IERC20 } from "@prb/contracts/token/erc20/IERC20.sol";
import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol";

/*//////////////////////////////////////////////////////////////////////////
                                    HOOKS
//////////////////////////////////////////////////////////////////////////*/

import { ISablierV2Recipient } from "../hooks/ISablierV2Recipient.sol";
import { ISablierV2Sender } from "../hooks/ISablierV2Sender.sol";

[PaulRBerg]

Regarding merging Math and Tokens - I see your point, and as I said above, I am sympathetic to it.

But I do not agree regarding the hooks - it's fine to leave them separate, because the hooks are not "types" per se. They have nothing to do with the user-defined value types of PRBMath, or the structs. They are interface files, which serve a specific purpose.

FYI, even forge-std has gone down this path of modularization and selective importing - see the discussion here about the rationale for removing the Components.sol file, which was re-exporting all of the Forge Std components. The TLDR; is that selectively importing files leads to faster compile times, because when you re-export everything in one file, Solidity compiles all of those files even if the user imports just one file.

Also, I don't like any name that contains the term "exports" - let's sleep on this for a while.

[PaulRBerg]

Thanks @andreivladbrg, your feedback is excellent.

I liked the shared pattern to get rid of the duplicated tests.

Glad you like it.

Regarding structs I suggest to keep them in their dedicated file and separate them with headers, memory / storage

I don't like this separation:

1. It would lead to a worse UX for integrators, because they would have to know the import path for each struct.
2. The imports would grow bigger as a result.
3. It would become more difficult to find the structs. Now, they are aggregated in one file, which makes them easy to search.

To solve the size issue in the pro contract is mandatory to move

I'm in favor of this proposal, but let's merge this PR as is and we will later come back to this idea in case we don't manage to reduce the size of the contracts in other ways.

Another reason why I like this separation is that it makes it possible to perform the pro calculations from outside of the pro contracts - this might be helpful for integrators, or for our periphery contracts.

[andreivladbrg]

1. It would lead to a worse UX for integrators, because they would have to know the import path for each struct.
2. The imports would grow bigger as a result.
3. It would become more difficult to find the structs. Now, they are aggregated in one file, which makes them easy to search

You really didn't get my point

Separation by headers

// SPDX-License-Identifier: LGPL-3.0
pragma solidity >=0.8.13;

import { IERC20 } from "@prb/contracts/token/erc20/IERC20.sol";
import { SD1x18 } from "@prb/math/SD1x18.sol";

/*//////////////////////////////////////////////////////////////////////////
                                MEMORY STRUCTS
//////////////////////////////////////////////////////////////////////////*/

/// @notice Simple struct that encapsulates the net deposit amount, the protocol fee amount, and the operator fee
/// amount.
/// @custom:field netDeposit The deposit amount net of fees, in units of the token's decimals.
/// @custom:field protocolFee The protocol fee amount, in units of the token's decimals.
/// @custom:field operatorFee The operator fee amount, in units of the token's decimals.
struct CreateAmounts {
    uint128 netDeposit; // ──┐
    uint128 protocolFee; // ─┘
    uint128 operatorFee;
}

/// @dev This struct is needed to avoid the "Stack Too Deep" error.
struct CreateWithMilestonesArgs {
    CreateAmounts amounts;
    Segment[] segments;
    address sender; // ──┐
    uint40 startTime; // │
    bool cancelable; // ─┘
    address recipient;
    address operator;
    IERC20 token;
}

/// @dev This struct is needed to avoid the "Stack Too Deep" error.
struct CreateWithRangeArgs {
    CreateAmounts amounts;
    Range range;
    address sender; // ──┐
    bool cancelable; // ─┘
    address recipient;
    address operator;
    IERC20 token;
}

/*//////////////////////////////////////////////////////////////////////////
                                STORAGE STRUCTS
//////////////////////////////////////////////////////////////////////////*/

/// @notice Simple struct that encapsulates the deposit and the withdrawn amounts.
/// @custom:field deposit The amount of tokens that have been originally deposited in the stream, net of fees and
/// in units of the token's decimals.
/// @custom:field withdrawn The amount of tokens that have been withdrawn from the stream, in units of the token's
/// decimals.
struct Amounts {
    uint128 deposit; // ───┐
    uint128 withdrawn; // ─┘
}

/// @notice Linear stream struct used in the SablierV2Linear contract.
/// @dev The fields are arranged like this to save gas via tight variable packing.
/// @custom:field amounts Simple struct with the deposit and withdrawn amounts.
/// @custom:field segments The arrays of segments used to compose the custom streaming curve.
/// @custom:field sender The address of the sender of the stream.
/// @custom:field isCancelable A boolean that indicates whether the stream is cancelable or not.
/// @custom:field isEntity A boolean that signals the existence of the instance of the struct.
/// @custom:field token The address of the ERC-20 token used for streaming.
struct LinearStream {
    Amounts amounts;
    Range range;
    address sender; // ───┐
    bool isCancelable; // │
    bool isEntity; // ────┘
    IERC20 token;
}

/// @notice Pro stream struct used in the SablierV2Pro contract.
/// @dev The fields are arranged like this to save gas via tight variable packing.
/// @custom:field amounts Simple struct with the deposit and withdrawn amounts.
/// @custom:field segments The arrays of segments used to compose the custom streaming curve.
/// @custom:field sender The address of the sender of the stream.
/// @custom:field isCancelable A boolean that indicates whether the stream is cancelable or not.
/// @custom:field isEntity A boolean that signals the existence of the instance of the struct.
/// @custom:field token The address of the ERC-20 token used for streaming.
struct ProStream {
    Amounts amounts;
    Segment[] segments;
    address sender; // ───┐
    uint40 startTime; //  │
    bool isCancelable; // │
    bool isEntity; // ────┘
    IERC20 token;
}

/// @notice Range struct used as a field in the linear stream.
/// @custom:field cliff The Unix timestamp for when the cliff period will end.
/// @custom:field start The Unix timestamp for when the stream will start.
/// @custom:field stop The Unix timestamp for when the stream will stop.
struct Range {
    uint40 cliff;
    uint40 start;
    uint40 stop;
}

/// @notice Segment struct used in the SablierV2Pro contract.
/// @custom:field amount The amounts of tokens to be streamed in this segment, in units of the token's decimals.
/// @custom:field exponent The exponent in this segment, whose base is the elapsed time as a percentage.
/// @custom:field milestone The Unix timestamp for when this segment ends.
struct Segment {
    uint128 amount;
    SD1x18 exponent;
    uint40 milestone;
}

[PaulRBerg]

I'm sorry, @andreivladbrg, I might have misunderstood you. But the way you have worded your initial suggestion was a bit unclear - you said "I suggest to keep them in their dedicated file" - this is interpretable but I would say it's more easy to interpret as meaning that you meant to create one file for each struct. If you wanted to say to keep all structs in the same file, it would have been better to use the terms "all" and "current file", to communicate the fact that the way they are structured currently is fine.

Anyway. As I said in one of the comments above, I wish to keep the CreateWithRangeArgs and CreateWithMilestones outside of the Structs file, because they are not meant to be imported by end users. Therefore, the memory <> storage dichotomy doesn't help much. Also, the CreateAmounts struct is now emitted in the create events, which might create a bit of confusion with respect to the in-memory classification.

[andreivladbrg]

Therefore, the memory <> storage dichotomy doesn't help much

I think is very helpful

the CreateAmounts struct is now emitted in the create events, which might create a bit of confusion with respect to the in-memory classification.

There is nothing wrong with this (nothing to be confused by), the params are still in memory and not stored in our contracts.

For me this separation of classifications is just logical.

[andreivladbrg]

As I said in one of the comments above, I wish to keep the CreateWithRangeArgs and CreateWithMilestones outside of the Structs file, because they are not meant to be imported by end users.

Why is one struct meant to be imported more than another?

They can just import one if they don't want all of them.

[PaulRBerg]

I think is very helpful

It wouldn't be if the only in-memory struct is CreateAmounts.

For me this separation of classifications is just logical.

It might be logical but it would add a lot of comments and headers for separating just one struct from the rest. Looks odd.

Why is one struct meant to be imported more than another?

The CreateWithRangeArgs and the CreateWithMilestonesArgs are NOT meant to be used by end users - we should not export something that users should never touch. This can easily create confusion, e.g. if end user imports the Structs.sol file wholesale (without using specific symbols).

[andreivladbrg]

It might be logical but it would add a lot of comments and headers for separating just one struct from the rest. Looks odd.

Ofc, the idea is to add the CreateWithRangeArgs and CreateWithMilestonesArgs structs.

This can easily create confusion, e.g. if end user imports the Structs.sol file wholesale (without using specific symbols).

To import all file is a problem of a uneducated user/integrator, we will make sure to specify this exactly in our documentation on how to interact with sablier. For a newbie user would be more confusing having a dedicated struct file and also to see a struct in the core contracts.

[PaulRBerg]

To import all file is a problem of a uneducated user/integrator

You would be surprised by how often Solidity files are imported in the global scope - specific symbols have only become popular in the recent years. This was due, in part, to the fact that Solidity contracts used to be tested in JavaScript/ Python.

we will make sure to specify this exactly in our documentation

So why give homework to ourselves and have to document something when we can just hide it away from the public API in the first place?

For a newbie user would be more confusing having a dedicated struct file and also to see a struct in the core contracts.

Uhm, no? A third-party integrator doesn't even have to look at the implementation details. They can just pick up the interface files and the types - and off they go.

The actual user-facing create function don't expect a struct as a parameter - if they did, I would have welcomed your suggestion to move the said structs in the Structs.sol file. But that is simply not the case - the CreateWithRangeArgs and CreateWithMilestonesArgs structs are concerned with the internal business logic of the protocol, not with end users.