Should we use SD59x18 in SablierV2Linear, too?

[PaulRBerg]

The SablierV2Pro contract need to use SD59x18 because all quotients end up taking a value between 0 and 1, and that value is passed as a base to the exp function, which uses log2 under the hood, which spits out a negative value when the input is smaller than 1.

The SablierV2Linear contract doesn't use the exp function though, so that's why we implemented the calculations in the getWithdrawableAmount function with UD60x18.

Question is: should we switch to use SD59x18 everywhere?

The main pro would be consistency. The main con would be the slightly higher gas of working with signed numbers.

[PaulRBerg]

Consistency is not worth it in this case. We should stick with UD60x18 in the linear contract.

[PaulRBerg]

It turns out that using SD59x18 is necessary in the Lockup Dynamic contract. Let me copy-paste my response to Cantina (during their first audit):

We agree that it would be nice to be able to use UD60x18 in SablierV2LockupDynamic. However, despite our best efforts to find a solution, it turns out that this cannot be done without significantly limiting the scope of the dynamic contract.

I have rewritten the UD60x18.pow function to apply your recommendation when the base input is less than 1e18. The modified implementation performs well within the scope of PRBMath, but not when applied to Sablier, because of the following mathematical constraint:

• The base, elapsedTimePercentage, is frequently a very small value.
• Consequently, the variable i becomes exceedingly large
• As a result, the mul operation that computes the variable w reverts, as the exp2 function does not support inputs greater than 192e18

In practice, this is what happened after switching from SD59x18 to UD60x18 - we started getting the following error when running the fuzz tests:

PRBMath_UD60x18_Exp2_InputTooBig

Thus, UD60x18 doesn't scale well for the mathematical complexity demanded by SablierV2LockupDynamic. We have to use SD59x18 due to the need to take the binary logarithm of values below 1e18, which requires in-flight signed numbers.

FYI, here are some examples of inputs that triggered reverts in testFuzz_Withdraw:

// args=[(0, [(0, 0, 0), (2475880078570760549798248447, 18446744073709551613, 436333)], 2,
// 0xF3408fb94475994988Ad732cC395EfEA04F124f1)]]
params.deposit = 0;
params.segments = new LockupDynamic.Segment[](2);
params.segments[0] = LockupDynamic.Segment({ amount: 0, exponent: ud2x18(0), milestone: 0 });
params.segments[1] = LockupDynamic.Segment({
    amount: 2_475_880_078_570_760_549_798_248_447,
    exponent: ud2x18(18_446_744_073_709_551_613),
    milestone: 436_333
});
params.timeWarp = 2;
params.to = 0xF3408fb94475994988Ad732cC395EfEA04F124f1;

These values are relatively large (the exponent is nearly MAX_UD2x18), but this doesn't change the fact that multiplications are hard to control, and switching to UD60x18 would have required us to severely constrain the design space of dynamic streams. Some limitations would have been awkward, such as prohibiting long-dated streams because they lead to tiny elapsedTimePercentage values.