Extend status enum

[PaulRBerg]

This is a big PR. Its primary goal is to implement #459 by extending the status enum, but it also closes a number of other issues:

• Refactor the default constant and params in the tests #438
• Dedup _withdrawableAmountOf #455
• Settled streams should not be renounceable #457
• Shorten name of WithdrawAmountGreaterThanWithdrawableAmount error #458
• Fuzz tests for cancelMultiple are not run #463

See the diagram in #459 for a visual guide to the changes introduced by this PR.

[andreivladbrg]

SRC

In the create function from linear contract you explicitly set the isCanceled and isDepleted flags to false, but in the dynamic contract you didn't. What version should we choose?

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupDynamic.sol#L544-L550

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupLinear.sol#L459-L460

I noticed this pattern in both cancel and withdraw function. At the start of these functions, statusOf is called to perform checks to determine if the function can be called. This thing is very inefficient because it will calculate the streamed amount twice if the status is STREAMING or SETTLED. Imagine a dynamic stream with multiple segments and
a timestamp in the last segment.

Once here:
https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupLinear.sol#L226

Once here:

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupLinear.sol#L378

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupLinear.sol#L530

Cancel

To address the issue, we should make the check on streamedAmount variable.

We can even eliminate the CANCELED and DEPLETED status checks altogether. This is because we have an invariant where if either of the isCanceled or isDepleted flags are true, then the isCancelable flag is false.

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupLinear.sol#L383

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupLinear.sol#L548

Although, if we do want to make additionals checks on the isCanceled isDepleted flags we should add getters and use them instead of calling statusOf.

Withdraw

To address the issue, we should use a getter function for isDepleted flag.

We can eliminate the PENDING check due to SablierV2Lockup_WithdrawAmountZero or SablierV2Lockup_Overdraw.

Let's rename the isCold and isWarm functions to isStatusCold and isStatusWarm respectively (and anything else related to the status). This way, we will indicate that we are not referring to storage properties.

Based on what I mentioned above, I would recommend to:

• add a functions that returns a boolean on a specific status
• move anything related to the status in the lockup contract
• make checks in the _cancel and _withdraw functions only on storage properties
• remove the notCold modifier

No need to declare streamId here, we can use streamIds[i]:

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/abstracts/SablierV2Lockup.sol#L226

Given that the _calculateStreamedAmount function is called only in the context when not depleted I think we can remove these lines from:

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupLinear.sol#L304-L308

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupDynamic.sol#L314-L317

Is the _checkParamsAndWithdraw function really necessary? We can move its logic in the withdraw function and change the withdrawMultiple as we did to cancelMultiple.

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/abstracts/SablierV2Lockup.sol#L269

Code snip

    /// @inheritdoc ISablierV2Lockup
    function withdrawMultiple(
        uint256[] calldata streamIds,
        address to,
        uint128[] calldata amounts
    )
        external
        override
        noDelegateCall
    {
        // Checks: there is an equal number of `streamIds` and `amounts`.
        uint256 streamIdsCount = streamIds.length;
        uint256 amountsCount = amounts.length;
        if (streamIdsCount != amountsCount) {
            revert Errors.SablierV2Lockup_WithdrawArrayCountsNotEqual(streamIdsCount, amountsCount);
        }

        // Iterate over the provided array of stream ids and withdraw from each stream.
        for (uint256 i = 0; i < streamIdsCount;) {
            // Checks, Effects, and Interactions: check the parameters and make the withdrawal.
            withdraw(streamIds[i], to, amounts[i]);

            // Increment the loop iterator.
            unchecked {
                i += 1;
            }
        }
    }

Regarding the new isCanceled flag, I think it would be more appropriate to name it wasCanceled instead, and we should not set it to false when the stream becomes depleted. This is because the stream can be canceled once and (as you said before) not to rely on the amount variable, we need to have a historical component which shows that.

I noticed that you moved the withdrawableAmountOf function in the abstract contract SablierV2Lockup because we marked the internal function as virtual.

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/abstracts/SablierV2Lockup.sol#L103

I think it would be a good idea if we also move the refundableAmountOf functions, ofc this would require to make _calculateStreamedAmount virtual.

In the _withdraw function you loaded the amounts struct in memory, I suggest we do the same in the _cancel function:

https://github.com/sablierhq/v2-core/blob/7b4d237e99ecbaf15848ab15a4122be40e8fe4e6/src/SablierV2LockupDynamic.sol#LL637C31-L637C38

Test

I don't have much to say other than it looks cleaner now. Although, I think it would be better if we change Defaults to a library (as I did initially), especially for these reasons:

• feat(forge): exclude certain contracts from being eavesdropped on by vm.expectRevert foundry-rs/foundry#4762
• Copying of type struct LockupDynamic.SegmentWithDelta memory[] memory to storage not yet supported.

The only downside is that you need pass the users and the asset in the default create functions in the shared dir, which IMO is not a big deal. Design wise a library is just superior.

[PaulRBerg]

Thanks for the extensive review, Andrei. I have responded to all of your specific comments above, and implemented the ones I have agreed with in my latest commit.

Explicit Flags

In the create function from linear contract you explicitly set the isCanceled and isDepleted flags to false, but in the dynamic contract you didn't. What version should we choose?

In the linear contract's case, those flags must be provided because otherwise, Solidity would throw a syntax error (all struct parameters must be provided when initializing a struct).

We can explicitly set isCanceled and isDepleted to false in the dynamic contract if you want, even if there is no technical requirement for doing so. Update: I changed my mind. It might cost to include these statements, so let's KISS this point by not including them.

Side note - "version" wasn't a fitting word in this context. It would have been much clearer to say "approach".

StatusOf Pattern

I noticed this pattern in both cancel and withdraw function. At the start of these functions, statusOf is called to perform checks to determine if the function can be called. This thing is very inefficient because it will calculate the streamed amount twice if the status is STREAMING or SETTLED

This pattern might be inefficient from a gas cost point of view, but it is clear, concise, and understandable.

Cancel Function

To address the issue, we should make the check on streamedAmount variable.

Despite my comment above, I'm happy to see how this approach works in practice.

We can even eliminate the CANCELED and DEPLETED status checks altogether. This is because we have an invariant where if either of the isCanceled or isDepleted flags are true, then the isCancelable flag is false.

I am not in favor of this approach because of two reasons:

1. Reverting on DEPLETED and CANCELED provides context and targeted information for end users (see my comment here).
2. All else being equal, relying on a first-tier invariant rather than a second-tier invariant (or an indirect invariant) is safer.

if we do want to make additional checks on the isCanceled isDepleted flags we should add getters and use them instead of calling statusOf

Again, no, due to the reasons given in the comment replies.

Withdraw Function

To address the issue, we should use a getter function for isDepleted flag.

We can just call the isDepleted storage property.

We can eliminate the PENDING check due to SablierV2Lockup_WithdrawAmountZero or SablierV2Lockup_Overdraw.

No, due to the reasons given in the comment replies.

Let's rename the isCold and isWarm functions to isStatusCold and isStatusWarm respectively (and anything else related to the status). This way, we will indicate that we are not referring to storage properties.

Let's keep it as is (isCold and isWarm) due to the following two reasons:

1. The English language is sufficiently rich for both the expressions "the stream is cold" and "the stream's status is cold" to make sense.
2. The is function cannot refer just to storage properties - see the explanation in the comment replies.

No need to declare streamId here, we can use streamIds[i]:

Yup.

Given that the _calculateStreamedAmount function is called only in the context when not depleted I think we can remove these lines from:

I'm afraid we can't remove those lines because of the existence of the SETTLED status (see this).

Is the _checkParamsAndWithdraw function really necessary? We can move its logic in the withdraw function

No, it isn't necessary (though, again, I insist on not using this as a criterion; "helpful" and "unhelpful" are better). We could move the logic to withdraw as you suggest.

Regarding the new isCanceled flag, I think it would be more appropriate to name it wasCanceled instead, and we should not set it to false when the stream becomes depleted. This is because the stream can be canceled once and (as you said before) not to rely on the amount variable, we need to have a historical component which shows that.

💯 agree

I think it would be a good idea if we also move the refundableAmountOf functions, ofc this would require to make _calculateStreamedAmount virtual.

Overkill. So many things need to be done to de-dup refundableAmountOf that it is just simpler to keep the code duplicated. Besides _calculateStreamedAmount, there is also the need to access isDepleted.

In the _withdraw function you loaded the amounts struct in memory, I suggest we do the same in the _cancel function.

In _withdraw, we need to read all amounts. In _cancel, we need to read only the deposited and the withdrawn amount. I did not check, but I guess it's cheaper not to load them in memory, as per the feedback I received in this Twitter thread.

I don't have much to say other than it looks cleaner now.

TY!

I think it would be better if we change Defaults to a library

I agree that a library would be nicer, but it doesn't work because of the need to initialize the users and the default ERC-20 asset.

foundry-rs/foundry#4762

They started working on a fix for this.

Copying of type struct LockupDynamic.SegmentWithDelta memory[] memory to storage not yet supported

What do you mean by this? It seems like a moot point because we would bump into this problem in any case.

The only downside is that you need to pass the users and the asset in the default create functions in the shared dir, which IMO is not a big deal.

It is a big deal considering the many places where those default functions are used + the issue of constantly bumping into this limitation.

Let's keep it as a contract, please.

[andreivladbrg]

This pattern might be inefficient from a gas cost point of view, but it is clear, concise, and understandable.

It's not just about efficiency from the gas cost point of view.

but it is clear, concise, and understandable.

This is where we disagree. You keep saying that adding getters for isDepleted and wasCanceled would overcomplicate things, I really don't understand why is that? IMO this is simply not true, the statusOf is the overcomplicated thing, this function underneath queries the isDepleted variable. Please keep in mind that the status, as you said, is just a dynamic thing that is computed with values we store. The source of truth are the storage properties and the getters are a way to read them, and are not an overkill.

Please double check what I am trying to achieve with my branch, I strongly believe that is the right design decision we should make. Ofc the work is not finished and it can be improved.

The English language is sufficiently rich for both the expressions "the stream is cold" and "the stream's status is cold" to make sense.

I don't think so, the stream is not cold, the status is.

The is function cannot refer just to storage properties

Yes, it isn't. We also use it for other things, but we are very precise about them:

https://github.com/sablierhq/v2-core/blob/021362b4953e19d227611278a3e0c225d458e170/src/abstracts/SablierV2Lockup.sol#L253

https://github.com/sablierhq/v2-core/blob/021362b4953e19d227611278a3e0c225d458e170/src/abstracts/SablierV2Lockup.sol#L243

Let's rename it.

to de-dup refundableAmountOf

Don't have a strong opinion here, so I give up on this.

we need to read only the deposited and the withdrawn amount.
it's cheaper not to load them in memory

Yes, I know, but the difference is just too small. I think readability outweighs the gas cost.

need to initialize the users and the default ERC-20 asset.

We can just pass them in the function parameters, we only need the returned values in memory.

It is a big deal considering the many places

Actually there are not that many places. Plus, it's only about passing arguments; this is just not a big deal.

What do you mean by this? It seems like a moot point because we would bump into this problem in any case.

It is a problem because we still need to have these storage variables here:

https://github.com/sablierhq/v2-core/blob/021362b4953e19d227611278a3e0c225d458e170/test/shared/lockup-linear/Linear.t.sol#L13-L26

https://github.com/sablierhq/v2-core/blob/021362b4953e19d227611278a3e0c225d458e170/test/shared/lockup-dynamic/Dynamic.t.sol#L13-L47

We can't do this:

_params = defaults.linearParams();

It is a big deal considering the many places where those default functions are used + the issue of constantly bumping into this limitation

The places where we would need to call the defaults function, with the users and the asset, are in the createDefaultStream functions.

[PaulRBerg]

It's not just about efficiency from the gas cost point of view.
...
the statusOf is the overcomplicated thing
...
Please double check what I am trying to achieve with my branch, I strongly believe that is the right design decision we should make.

I'll make a compromise and concede this point even if I disagree. To be productive, I won't respond to the other assertions you made. I will follow the design laid out on your branch.

the stream is not cold, the status is.

Hairsplitting. See Hyrum's Law - users will refer to streams being "cold" or "warm" regardless of how we present the nomenclature. Again, the English language is sufficiently rich to permit these expressions.

The is function cannot refer just to storage properties

Yes, it isn't. We also use it for other things, but we are very precise about them:

My point was the isCancelable function cannot just read from storage; we need to call statusOf to check that the stream is not settled. Did you understand this?

Let's rename it.

I'm sorry. Rename what?

Yes, I know, but the difference is just too small. I think readability outweighs the gas cost.

I don't think it's small, but there's also the potential risk of StackTooDeep, too. I'll have a look.

It is a problem because we still need to have these storage variables here:

Ah, yes, you're right. However, this issue will go away once Foundry fixes foundry-rs/foundry#4762.

The places where we would need to call the defaults function, with the users and the asset, are in the createDefaultStream functions.

For now* (you missed my point about the maintenance cost).

Let's keep it as a contract, please. I have a strong opinion.

[PaulRBerg]

@andreivladbrg, the PR is ready for your re-review.

I refactored the implementation according to your suggestions, with one exception: I did not refactor the renounce as you did on your branch because the streamed amount is not re-calculated in _renounce. In this case, it's fine to call statusOf.

[andreivladbrg]

My point was the isCancelable function cannot just read from storage; we need to call statusOf to check that the
stream is not settled. Did you understand this?

Yes, I did understand, my point was that the getter should match exactly the value from storage. Actually I think this
is a tough situation what is the right naming of the variable. For example, consider a stream created with
isCancelable set to true, which neither gets canceled nor depleted. It could lead to confusion if a variable and a
function share the same name but would return different values.

I don't see why we would keep both external and internal functions for isDepleted and wasCanceled. I think one is
enough(public), even though it would mean that the notNull would be called twice in the cancel function.

https://github.com/sablierhq/v2-core/blob/ac6f04af4d0cfbd56b91094c09d42f0cfdb4ab87/src/SablierV2LockupLinear.sol#L177

https://github.com/sablierhq/v2-core/blob/ac6f04af4d0cfbd56b91094c09d42f0cfdb4ab87/src/SablierV2LockupLinear.sol#L355

Plus, not alphabetically ordered:

https://github.com/sablierhq/v2-core/blob/ac6f04af4d0cfbd56b91094c09d42f0cfdb4ab87/src/SablierV2LockupLinear.sol#L360-L365

https://github.com/sablierhq/v2-core/blob/ac6f04af4d0cfbd56b91094c09d42f0cfdb4ab87/src/SablierV2LockupDynamic.sol#L449-L454

Did you leave it like this on purpose? It would be easier if we remove the statusOf from the SablierV2Lockup and add
isCold instead. Do you want to have an explicit revert reason?

https://github.com/sablierhq/v2-core/blob/ac6f04af4d0cfbd56b91094c09d42f0cfdb4ab87/src/abstracts/SablierV2Lockup.sol#L162-L170

The notNull modifier is implicitly used in the statusOf function.

https://github.com/sablierhq/v2-core/blob/ac6f04af4d0cfbd56b91094c09d42f0cfdb4ab87/src/abstracts/SablierV2Lockup.sol#L161

[PaulRBerg]

It looks like GitHub hadn't updated the diff in this PR, so you reviewed the branch directly instead of leaving comments in this PR.

my point was that the getter should match exactly the value from storage. Actually I think this
is a tough situation what is the right naming of the variable

I see. I agree that it is not clear what is the best solution, but I would be inclined to lean toward the solution I implemented in my latest commits, namely, returning false when the status is SETTLED.

Imagine a smart contract that depends upon the value returned by isCancelable to allow cancellations. If this value is true but the stream is SETTLED, the contract would allow a state transition that leads to a surefire revert, which can have all sorts of bad consequences.

It could lead to confusion if a variable and a function share the same name but would return different values.

The only place where the isCancelable storage variable is exposed is the getStream getter, which returns the entire struct, so I don't think there is that much potential for confusion. However, I agree that this is an issue, so I created a separate discussion to explore the possibility of renaming the storage variable: #486

In this PR, let's follow my implementation to merge this quickly.

don't see why we would keep both external and internal functions for isDepleted and wasCanceled. I think one is
enough(public), even though it would mean that the notNull would be called twice in the cancel function.

Fair enough, I see where you're coming from. Having only one getter for each would be more KISS, so I agree that we should do it.

Plus, not alphabetically ordered:

Good catch.

Did you leave it like this on purpose? It would be easier if we remove the statusOf from the SablierV2Lockup and add
isCold instead. Do you want to have an explicit revert reason?

Yes, I did that on purpose. I want to revert with an explicit error rather than _StreamCold (an error which, by the way, doesn't exist anymore in Errors.sol).

The notNull modifier is implicitly used in the statusOf function.

Good catch.

[PaulRBerg]

I implemented the changes as per the discussion above. Ready for re-review, @andreivladbrg!

FYI: I ended up removing the onlySenderOrRecipient modifier due to the following reasons:

1. After removing notNull (which is now checked implicitly in isDepleted), the order of checks was changed, which made the tests fail. And we want to check null-ness before access control.
2. It was only needed in one place anyway (the cancel function).

[andreivladbrg]

I ended up removing the onlySenderOrRecipient modifier due to the following reasons:

That's ok.

Looks like it's ready to be merged now. I hope I haven't missed anything😅.

[PaulRBerg]

Awesome!! Thanks, Andrei. Let's wait for the CI to finish, and we will merge.

[PaulRBerg]

Looks like it's ready to be merged now.

Can you approve it?