Move all duplicated logic in the abstract contract SablierV2Lockup

[andreivladbrg]

Addresses #805.

Depends on #806.

Changes

• Introduce a new Datatype Stream within Lockup namespace
• Move _streams mapping to SablierV2Lockup and use the new data type
• Move all duplicated logic in the abstract contract SablierV2Lockup. This change introduced the need of:
  • _calculateStreamedAmount virtual function
  • Additional logic in the getStream function
• Introduce _cliffs and _segments mappings in linear contract, and dynamic respectively
• Allow cliff time to be zero, which means there is a simple linear stream created. This change introduced the need of:
  • Check if start time is zero
  • Check if start time is strictly less than the end time

Also, it would be recommended to read the 805 discussion for the rational behind.

Other decisions

It no longer made sense to allow the start time to equal the cliff time, since now it can be zero (cliff).

I could have DRY-FY the _createWithTimestamps function since there is common logic between linear and dynamic. But the reasons I did not were:

• Checks, Effects, Interactions pattern was not respected anymore
• Less flexibility for the child contract inheriting SablierV2Lockup
• Introduces the "stack too deep error" in dynamic contract
  • Which is also the reason I did not use the linear approach when storing the Stream struct

Gas comparison

Contract	Type of stream	Staging branch gas	This branch gas cost	Increased by
LockupLinear	Simple linear stream	116184	116052	-132
LockupLinear	Cliff linear stream	116184	138073	21889
LockupDynamic	Two segments stream	182841	182967	126

Notes and questions

After this PR the app would need to change how it works when creating a simple linear stream (passing 0 as the cliff time).

Should we rename the Stream structs within LockupLinear and LockupDynamic namespaces (which are used only for getStream function)?
Should we re-order the variables in these structs?

[andreivladbrg]

The Codecov report is now failing. After debugging, I can see that the lines marked as "Uncovered" are the same as in other PRs.

Current PR: https://app.codecov.io/gh/sablier-labs/v2-core/pull/813/blob/src/SablierV2LockupDynamic.sol
Other PR: https://app.codecov.io/gh/sablier-labs/v2-core/pull/814/blob/src/SablierV2LockupDynamic.sol

The issue may be from the fact that the contracts (Linear and Dynamic) now have fewer lines, thereby increasing the percentage of uncovered lines. This is why the report's coverage has decreased.

Atm, I am not sure how to address this issue, other than by reducing the coverage threshold on Codecov.

What do you say @PaulRBerg ?

[PaulRBerg]

Sorry no time to advise on this issue now. I recommend asking for help on StackOverflow or the Codecov forum.

[smol-ninja]

Great work @andreivladbrg. I have left some comments below but I haven't finished the review yet.

[andreivladbrg]

Regarding the codecov coverage mention in this comment

I did some research, and I was correct. The reason why the coverage fails is because there are fewer lines of code in the targeted files (SablierV2LockupLinear and SablierV2LockupDynamic).

GPT answer

"
This usually occurs because code coverage is calculated as the ratio of the number of lines of code executed by tests to the total number of lines of code. When you refactor code to be more efficient (i.e., reducing the total number of lines), even if the number of lines tested remains the same, the percentage of code covered can decrease because there's a smaller total number of lines of code.
"

One potential solution, is to temporarily add a threshold of 0.3% on codecov.yml file, and after we merge this PR to remove it.

What do you say? @PaulRBerg @smol-ninja

[PaulRBerg]

Thanks for investigating, @andreivladbrg.

One potential solution, is to temporarily add a threshold of 0.3%

That shouldn't be needed - as an admin, I have the power to merge the PRs even when the checks fail. Let me know when you want to merge it (and how - squashing or not), and I'll do it.

[andreivladbrg]

That shouldn't be needed - as an admin, I have the power to merge the PRs even when the checks fail. Let me know when you want to merge it (and how - squashing or not), and I'll do it.

I can do that as well. I was just saying this to avoid having the workflow fail. But I guess it is not a problem since the test- utils is also failing

[PaulRBerg]

Oh ok, got it. Letting the workflow fail it's fine in this case.

[smol-ninja]

I have reviewed the PR. Please see comments below.

[smol-ninja]

LGTM now.

[smol-ninja]

Should we rename the Stream structs within LockupLinear and LockupDynamic namespaces (which are used only for getStream function)?

In my opinion, it is better to rename them to avoid mentioning namespaces everytime we are referring to Stream structs. (Later: it might not be a problem as the default is the one defined in Lockup namespace but from the documentation pov, it looks better to use a different name for them).

Suggestions: StreamLL, StreamLD?

Should we re-order the variables in these structs?

Yes. I am in favour of this (comment).

[andreivladbrg]

In my opinion, it is better to rename them to avoid mentioning namespaces everytime we are referring to Stream structs. (Later: it might not be a problem as the default is the one defined in Lockup namespace but from the documentation pov, it looks better to use a different name for them).

Agree

Suggestions: StreamLL, StreamLD?

i like these

Yes. I am in favour of this (#813 (comment)).

I was ok with that, but I now changed my mind.
I would like to be consistent here, so I suggest we either:

• keep the current version, which has the order from Lockup.Stream and the segments/cliffs at the end
• change it to the order from CreateWithTimestamps struct

[smol-ninja]

keep the current version, which has the order from Lockup.Stream and at the segments/cliffs at the

We can do this. I am good with this as well.

[smol-ninja]

looks good now.

[PaulRBerg]

This tree is not aligned with the order of the checks in {Helpers-checkCreateWithTimestamps}.

It is critical that we align them because one of the goals of BTT is to build a circuit-like representation of the function's execution paths.

Specifically, we need to reverse the cliff time checks with the end time check here:

v2-core/src/libraries/Helpers.sol

Lines 95 to 108 in 76e29fe

	// Checks: the start time is strictly less than the end time.
	if (range.start >= range.end) {
	revert Errors.SablierV2LockupLinear_StartTimeNotLessThanEndTime(range.start, range.end);
	}
	// Checks: the start time is strictly less than the cliff time when cliff time is not zero.
	if (range.cliff > 0 && range.start >= range.cliff) {
	revert Errors.SablierV2LockupLinear_StartTimeNotLessThanCliffTime(range.start, range.cliff);
	}
	// Checks: the cliff time is strictly less than the end time.
	if (range.cliff >= range.end) {
	revert Errors.SablierV2LockupLinear_CliffTimeNotLessThanEndTime(range.cliff, range.end);
	}