Refactor lockup contracts to move all common logic in SablierV2Lockup

[andreivladbrg]

Motivation

Because I've been working on implementing the new Sablier streaming model, i.e. LockupTranched, it reminded my self how much duplicated logic exists between LockupLinear and LockupDynamic contracts.

This thing is caused by the different Stream (LD,LL) data structures stored in the contracts:

v2-core/src/SablierV2LockupDynamic.sol

Line 62 in ea6f097

mapping(uint256 id => LockupDynamic.Stream stream) private _streams;

v2-core/src/SablierV2LockupLinear.sol

Line 50 in ea6f097

mapping(uint256 id => LockupLinear.Stream stream) private _streams;

As we plan to implement multiple models of Lockup contracts in the future, this issue will increasingly cause more harm, in terms of dev work time, maintenance, and ofc more prone to errors (as we discussed here #771 - also, we would not need to implement that "code identity" test) and I don't think we should leave it as it is.

Solution

Note: I've already implemented the changes on this branch.

Given that the structs share more than 90% of the variables, it was inevitable to find a solution by adding a new data structure within the Lockup namespace. This structure will be used in all child contracts of SablierV2Lockup.

So the design I am proposing is:

• All lockup contracts share a common Stream entity and it is going to be placed in SablierV2Lockup
• Each model has the ability to add new data structures to store in their unique implemention
  • For linear model the new type is going to be cliffs: mapping(uint256 id => uint40 cliff) internal _cliffs;
  • For dynamic model the new type is going to be the segments: mapping(uint256 id => LockupDynamic.Segment[] segments) internal _segments;
• All streams will still share the same id (similar to how we get the recipient - it is stored in the owners mapping from ERC721)
• The child contracts will only implement the create functions and the calculation function: _calculateStreamedAmount
• The API remains the same, meaning that getStream function will return the same data structure as the current version, but this one is going to be built in the view function

The only downside I can think of is the gas implications in the create function, this design will require one more mapping.

I've performed a benchmark and this is the result:

Later edit: the gas costs are increased in the linear contract only when there is a cliff linear curve stream created. when there is a simple linear stream, no gas increases (benchmarked at 116052).

Contract	Current gas cost	Proposed design gas cost	Increased by
LockupLinear	116184	138073	21889
LockupDynamic	182841	182967	126

Note: the gas costs are for the createWithTimestamps function and in dynamic contract 2 segments were used.

In the linear contract, there is indeed a significant difference, due to the introduction of a new storage slot. In the dynamic contract, there is not a significant difference because the segments were already located at the end of the stream struct.

Despite the increase in gas, I believe that this refactor is the right choice to make. The code reduction amounts to almost 300 lines lines in the src dir. If we were to add Tranched, we would have even greater improvement.
It is a more robust and a much cleaner design.

Other consideration

Later edit: as per my comment below, this consideration is not needed anymore because we would leave cliff = 0 in simple linear sterams.

Since the only unique thing in the linear stream is the cliff time variable, and we can achieve the same feature in the dynamic contract with a 2-segment stream, we can consider removing the cliff streaming curve for linear streams. In this case, the only curve achievable by this contract would be:

This approach would allow us to maintain the same gas cost as the current version or even slightly reduce it due to the removal of the cliffTime. However, before making this decision, we should make a query on Dune for lockup linear contracts to see how frequently this curve is used. Also, we need to see what's the impact at the app level.

I am very curious about your opinion @PaulRBerg @smol-ninja.
You can view the work here.

[andreivladbrg]

I want to mention that I would like to make a decision on this proposal first and then create a PR on this repo for the new Tranched contract.

[PaulRBerg]

OK I will try to review during the weekend or early next week.

[PaulRBerg]

Thanks for the thoughtful proposal, @andreivladbrg. It seems hat you've put a lot of effort in this.

Motivation

As we plan to implement multiple models of Lockup contracts in the future, this issue will increasingly cause more harm, in terms of dev work time, maintenance, and ofc more prone to errors

The development work time and the maintenance cost are a valid concern, but:

1. I don't expect us to implement that many more Lockup models in the future.
2. Even if we did, Test the code identity between the Lockup contracts #771 would catch the vast majority of potential errors.

---

Solution

It is an interesting proposal.

The child contracts will only implement the create functions and the calculation function

Not correct. All child contracts will have to also implement getStream and other bespoke functionalities, e.g., getRange.

It is a more robust and a much cleaner design.

I agree.

---

Other considerations

This approach would allow us to maintain the same gas cost as the current version or even slightly reduce it due to the removal of the cliffTime

I did not understand this part. When you say "the same gas cost as the current version", do you mean that if we removed cliffTime from your refactored version of LockupLinear, then the gas cost of that refactored reimplementation would match the current implementation on main?

we need to see what's the impact at the app level

If we keep the API unchanged, I expect the impact to be minimal.

we should make a query on Dune for lockup linear contracts to see how frequently this curve is used

I've made the calculations myself using this query.

• In Sablier V2.0, 28% of all LL streams are Linear-Cliff, and the rest (72%) are Linear
• Because LL streams make up ~94% of all streams, that means that 26% of all streams are Linear-Cliff, and 64% are Linear
• For easier calculations, let's approximate those figures to a quarter ($1/4$) and three quarters ($3/4$)
• The dilemma boils down to this: is it worth it to increase the gas cost from 116k to 182k only for a quarter of users, or would it be better to increase it only to 138k, but do so for_all users_? (@andreivladbrg please correct me if I'm wrong, my understanding is that the gas cost would increase for all users of LL, not just for Linear-Cliff users)
• This is an economic question, and we can quantify it like this: divide the new gas cost by the initial gas cost, then multiply by the percentage of users affected

Harm 1

$$\begin{align*} \frac{182}{116} &\approx 1.59 \\\ 1.59 \times 0.25 &= 0.3975 \end{align*}$$

Harm 2

$$\begin{align*} \frac{138}{116} &\approx 1.19 \\\ 1.19 \times 1 &= 1.19 \end{align*}$$

Comparison

It thus seems that the harm factor is greater when all users pay 138k gas for the cliff feature compared to when only a portion pay 182k. Combined with the additional development and audit cost of implementing the _cliffs mapping, it looks like the best course of action would be to use LockupDynamic for Linear-Cliff streams.

---

Decision

I'm tentatively in favor of this proposal, but I have some concerns and I want to mention them:

1. This refactor will lead to a higher audit cost, but I think we should be prepared for an expensive audit in any case (because of https://github.com/sablier-labs/company-discussions/discussions/25).
2. Isn't there a risk that some future Lockup contract will not need all properties in the base Stream type?
3. There is a theoretical risk that the stream ids entries in _streams and _segments (and other mappings in other contracts) will not remain isomorphic. We have to ensure that the segments saved in storage correspond to the segments passed in the create function parameters (I think that our tests are already doing this, just want to raise awareness about it)

[andreivladbrg]

Motivation & Solution

I don't expect us to implement that many more Lockup models in the future.

we can't know it precisely in advance

Even if we did, #771 would catch the vast majority of potential errors

probably, but why build a system that requires additional components on top to catch these types of errors (identity code check) when you can design it from the beginning to not be concerned with them?

Not correct. All child contracts will have to also implement getStream

yeah, forgot about getStream

Other considerations

then the gas cost of that refactored reimplementation would match the current implementation on main?

yes, that's what i meant

If we keep the API unchanged, I expect the impact to be minimal.

the tests were not modified, and none failed, so i don't think there is even a change in the API

The dilemma boils down to this: is it worth it to increase the gas cost from 116k to 182k only for a quarter of users, or would it be better to increase it only to 138k, but do so for_all users_?

yes, this is the dilemma

please correct me if I'm wrong, my understanding is that the gas cost would increase for all users of LL, not just for Linear-Cliff users)

yes, this is correct for the current version of the refactor.

but an idea came to me (see the code snippet below) what if we:

Scenario 1

LL user wants to create a linear curve stream:

• pass cliffTime = 0 in the function
• in create function, check cliff time is 0, then don't store anything in the _cliffs mapping
• in _calculateStreamedAmount function, check cliff time is 0, then ignore it in the calculation

Scenario 2

LL user wants to create a cliff curve stream:

• pass the correct cliff time
• in create function, check cliff time > 0, then store it in the _cliffs mapping
• in _calculateStreamedAmount function, cliff time > 0, then perform the current version of the calculation

in this way, we would minimize the costs for all users: linear curve users would not pay the gas for the storage of the cliff time variable, linear cliff curve users would pay the extra costs that dynamic contract would have.

code snip

    /// create function
    function _createWithTimestamps(LockupLinear.CreateWithTimestamps memory params)
        internal
        returns (uint256 streamId)
    {
        // -- rest of the code --

        // this function needs to be updated
        Helpers.checkCreateWithTimestamps(createAmounts.deposit, params.range);

        if (params.range.cliff > 0) {
            _cliffs[streamId] = params.range.cliff;
        }

        // -- rest of the code --
    }

    function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
        uint256 cliffTime = uint256(_cliffs[streamId]);
        uint256 currentTime = block.timestamp;
        if (cliffTime > 0 && cliffTime > currentTime) {
            return 0;
        }
       // -- rest of the code --`
   }

so far, this version looks the best for me. we would have the robust design without the gas implications for the majority of the linear users, 72% as per your finding, (Linear-Cliff user would still have to pay the extra 21k gas).

Decision

I'm tentatively in favor of this proposal

i am glad

This refactor will lead to a higher audit cost

yes, this would increase the audit costs, but i doubt it would be by a lot, since the API remains the same. also, we already have a lot of things on the line (tranched + campaigns for them), so this would be a small thing, imo.

Isn't there a risk that some future Lockup contract will not need all properties in the base Stream type?

yeah, that was my concern as well. I have looked at the storage properties a lot, and all of them simply make sense in a lockup design.

There is a theoretical risk that the stream ids entries in _streams and _segments (and other mappings in other contracts) will not remain isomorphic.

yeah, i want to add that this concern can be applied to the current version as well: ERC721._owners[id] and _streams[id]

[PaulRBerg]

when you can design it from the beginning to not be concerned with them

From a strictly technical sense, you're right.

the tests were not modified, and none failed, so i don't think there is even a change in the API

Nice

in create function, check cliff time is 0, then don't store anything in the _cliffs mapping
...
this version looks the best for me. we would have the robust design without the gas implications for the majority of the linear users

Wait, I thought that storing 0 wouldn't cost anything. Why is it a gas optimization to add an if check before storing 0?

this would increase the audit costs, but i doubt it would be by a lot, since the API remains the same

It's a major refactor in the storage layout. These things are expensive.

The API doesn't matter that much for auditors.

I have looked at the storage properties a lot, and all of them simply make sense in a lockup design.

Ok

this concern can be applied to the current version as well: ERC721._owners[id] and _streams[id]

You're right, though with owners, we're not managing the storage writing ourselves. But anyway.

[andreivladbrg]

Why is it a gas optimization to add an if check before storing 0?

yes, it would be more efficient (also tested on remix) because it attempts to perform an assignment operation regardless of the value. it would not cost 21k gas in addition, as it would if the state were modified, but it still attempts to perform it.

but that's not the whole idea.

the problem is that the current system doesn't allow to create streams with startime > cliffTime which would be the case for cliffTime = 0:

v2-core/src/libraries/Helpers.sol

Lines 95 to 98 in b5b884d

	// Checks: the start time is less than or equal to the cliff time.
	if (range.start > range.cliff) {
	revert Errors.SablierV2LockupLinear_StartTimeGreaterThanCliffTime(range.start, range.cliff);
	}

and, in the UI we pass cliff = start for linear streams which is going to be changed

It's a major refactor in the storage layout.

fair

so, the conclusion is that we should proceed with this change?

@smol-ninja do you have other opinions?

[PaulRBerg]

it attempts to perform an assignment operation regardless of the value. it would not cost 21k gas in addition, as it would if the state were modified, but it still attempts to perform it.

I'm skeptical of this, but anyway. I will defer to your judgement.

the problem is that the current system doesn't allow to create streams with startime > cliffTime

right. In this case, let's proceed by implementing the new LockupLinear without cliffs.

in the UI we pass cliff = start for linear streams which is going to be changed

Surely that's something easy to change in the UI. Right, @razgraf?

the conclusion is that we should proceed with this change?

Yes.

[andreivladbrg]

right. In this case, let's proceed by implementing the new LockupLinear without cliffs.

wait, no 😅

the idea is to leave the cliffs in linear, but they are ignored when they are not set

[PaulRBerg]

Oh, ok. Whatever. You will make a PR and I can review then.

[smol-ninja]

The problem that you have identified @andreivladbrg is really good. I am very glad that you started this thoughtful discussion and worked on the changes.

Clearly, there is a trade-off between gas cost optimization and maintenance cost. But this looks like a balance between the two (except for the extra storage slot in LL). So, I am in favour of it. However, I haven't reviewed the code yet but could see optimizations such as

mapping(uint256 id => uint256 cliff) internal _cliffs

instead of

mapping(uint256 id => uint40 cliff) internal _cliffs

Regardless, I will review it in detail when you raise a PR for it.

[PaulRBerg]

@smol-ninja I don't think that using uint40 would help because of the way storage is laid out in the EVM.

Mapping keys are pseudo-random - the (i+1)th element would be stored in a different location on disk compared to the (i)th element. So the fact that each is a uint40 doesn't help.

[smol-ninja]

Yes. That's what I said. It doesn't help to use uint40 because each slot can only have one cliff value when we store it using the map

[PaulRBerg]

oh haha @smol-ninja, sorry! I've misunderstood your suggestion.

[andreivladbrg]

yes, it doesn't help, gas wise, but for consistency purpose i would leave the cliffTime as uint40. all the other time variables use this type

[PaulRBerg]

@andreivladbrg I think Shub's point is that using uint40 may actually increase the gas cost

If the gas cost is not increased, however, I agree that keeping uint40 is fine.