Move all duplicated logic in the abstract contract SablierV2Lockup (#…

…813) * build: update openzeppelin to 5.0.0 refactor: remove _after and _before hooks and implement _update function refactor: use newly added _requireOwned function refactor: use the new named parameter in ERC20 functions test: add mocks for ERC20 and ERC721 test: use openeppelin's custom errors in cheatcode chore: provide a more precise explanation for _update chore: define "_update" alphabetically docs: document "_update" function test: check IERC20Errors custom error revert * feat: add Lockup.Stream struct feat: add mapping for cliffs in linear feat: add mapping for segments in dynamic refactor: move all common functions in SablierV2Lockup * perf: use the storages instead of the getters * feat: check if the start time is zero feat: check if the start time is greater than end time test: update tests accordingly * docs: re-add natspec for _calculateStreamedAmountForMultipleSegments * refactor: make start time strictly less than cliff time * chore: update branch from staging * docs: use plural * refactor: order constant functions alphabetically refactor: make constant functions external * refactor: make cliff time strictly greater than start time * docs: correct natspec chore: use "override" specifier on nftDescriptor * test: update comment for using unchecked to allow overflow * docs: improve natspec chore: order functions alphabetically in SablierV2Lockup * refactor: rename Stream structs within LockupLinear and LockupDynamic namespaces --------- Co-authored-by: smol-ninja <shubhamy2015@gmail.com>
sablier-labs · Feb 15, 2024 · c47eebd · c47eebd
1 parent 3f36df4
commit c47eebd
Show file tree

Hide file tree

Showing 27 changed files with 571 additions and 803 deletions.
diff --git a/src/SablierV2LockupDynamic.sol b/src/SablierV2LockupDynamic.sol
diff --git a/src/SablierV2LockupLinear.sol b/src/SablierV2LockupLinear.sol
diff --git a/src/abstracts/SablierV2Lockup.sol b/src/abstracts/SablierV2Lockup.sol
diff --git a/src/interfaces/ISablierV2LockupDynamic.sol b/src/interfaces/ISablierV2LockupDynamic.sol
@@ -59,10 +59,10 @@ interface ISablierV2LockupDynamic is ISablierV2Lockup {
     /// @param streamId The stream id for the query.
     function getSegments(uint256 streamId) external view returns (LockupDynamic.Segment[] memory segments);
 
-    /// @notice Retrieves the stream entity.
+    /// @notice Retrieves the stream details, which is a struct documented in {DataTypes}.
     /// @dev Reverts if `streamId` references a null stream.
     /// @param streamId The stream id for the query.
-    function getStream(uint256 streamId) external view returns (LockupDynamic.Stream memory stream);
+    function getStream(uint256 streamId) external view returns (LockupDynamic.StreamLD memory stream);
 
     /// @notice Calculates the amount streamed to the recipient, denoted in units of the asset's decimals.
     ///

diff --git a/src/interfaces/ISablierV2LockupLinear.sol b/src/interfaces/ISablierV2LockupLinear.sol
@@ -43,7 +43,8 @@ interface ISablierV2LockupLinear is ISablierV2Lockup {
                                  CONSTANT FUNCTIONS
     //////////////////////////////////////////////////////////////////////////*/
 
-    /// @notice Retrieves the stream's cliff time, which is a Unix timestamp.
+    /// @notice Retrieves the stream's cliff time, which is a Unix timestamp.  A value of zero means there
+    /// is no cliff.
     /// @dev Reverts if `streamId` references a null stream.
     /// @param streamId The stream id for the query.
     function getCliffTime(uint256 streamId) external view returns (uint40 cliffTime);
@@ -54,10 +55,10 @@ interface ISablierV2LockupLinear is ISablierV2Lockup {
     /// @param streamId The stream id for the query.
     function getRange(uint256 streamId) external view returns (LockupLinear.Range memory range);
 
-    /// @notice Retrieves the stream entity.
+    /// @notice Retrieves the stream details, which is a struct documented in {DataTypes}.
     /// @dev Reverts if `streamId` references a null stream.
     /// @param streamId The stream id for the query.
-    function getStream(uint256 streamId) external view returns (LockupLinear.Stream memory stream);
+    function getStream(uint256 streamId) external view returns (LockupLinear.StreamLL memory stream);
 
     /// @notice Calculates the amount streamed to the recipient, denoted in units of the asset's decimals.
     ///
@@ -106,14 +107,17 @@ interface ISablierV2LockupLinear is ISablierV2Lockup {
     /// @dev Emits a {Transfer} and {CreateLockupLinearStream} event.
     ///
     /// Notes:
+    /// - A cliff time of zero means there is no cliff.
     /// - As long as the times are ordered, it is not an error for the start or the cliff time to be in the past.
     ///
     /// Requirements:
     /// - Must not be delegate called.
     /// - `params.totalAmount` must be greater than zero.
     /// - If set, `params.broker.fee` must not be greater than `MAX_FEE`.
-    /// - `params.range.start` must be less than or equal to `params.range.cliff`.
-    /// - `params.range.cliff` must be less than `params.range.end`.
+    /// - `params.range.start` must be greater than zero.
+    /// - `params.range.start` must be less than `params.range.end`.
+    /// - If set, `params.range.cliff` must be greater than `params.range.start`.
+    /// - If set, `params.range.cliff` must be less than `params.range.end`.
     /// - `params.range.end` must be in the future.
     /// - `params.recipient` must not be the zero address.
     /// - `msg.sender` must have allowed this contract to spend at least `params.totalAmount` assets.

diff --git a/src/libraries/Errors.sol b/src/libraries/Errors.sol
@@ -116,7 +116,13 @@ library Errors {
     error SablierV2LockupLinear_CliffTimeNotLessThanEndTime(uint40 cliffTime, uint40 endTime);
 
     /// @notice Thrown when trying to create a stream with a start time greater than the cliff time.
-    error SablierV2LockupLinear_StartTimeGreaterThanCliffTime(uint40 startTime, uint40 cliffTime);
+    error SablierV2LockupLinear_StartTimeNotLessThanCliffTime(uint40 startTime, uint40 cliffTime);
+
+    /// @notice Thrown when trying to create a stream with a start time greater than the end time.
+    error SablierV2LockupLinear_StartTimeNotLessThanEndTime(uint40 startTime, uint40 endTime);
+
+    /// @notice Thrown when trying to create a stream with a start time equal to zero.
+    error SablierV2LockupLinear_StartTimeZero();
 
     /*//////////////////////////////////////////////////////////////////////////
                              SABLIER-V2-NFT-DESCRIPTOR

diff --git a/src/libraries/Helpers.sol b/src/libraries/Helpers.sol
@@ -92,9 +92,19 @@ library Helpers {
             revert Errors.SablierV2Lockup_DepositAmountZero();
         }
 
-        // 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);
+        // Checks: the start time is not zero.
+        if (range.start == 0) {
+            revert Errors.SablierV2LockupLinear_StartTimeZero();
+        }
+
+        // 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.

diff --git a/src/types/DataTypes.sol b/src/types/DataTypes.sol
@@ -66,6 +66,35 @@ library Lockup {
         CANCELED,
         DEPLETED
     }
+
+    /// @notice A common data structure to be stored in all child contracts of {SablierV2Lockup}.
+    /// @dev The fields are arranged like this to save gas via tight variable packing.
+    /// @param sender The address streaming the assets, with the ability to cancel the stream.
+    /// @param startTime The Unix timestamp indicating the stream's start.
+    /// @param endTime The Unix timestamp indicating the stream's end.
+    /// @param isCancelable Boolean indicating if the stream is cancelable.
+    /// @param wasCanceled Boolean indicating if the stream was canceled.
+    /// @param asset The contract address of the ERC-20 asset used for streaming.
+    /// @param isDepleted Boolean indicating if the stream is depleted.
+    /// @param isStream Boolean indicating if the struct entity exists.
+    /// @param isTransferable Boolean indicating if the stream NFT is transferable.
+    /// @param amounts Struct containing the deposit, withdrawn, and refunded amounts, all denoted in units of the
+    /// asset's decimals.
+    struct Stream {
+        // slot 0
+        address sender;
+        uint40 startTime;
+        uint40 endTime;
+        bool isCancelable;
+        bool wasCanceled;
+        // slot 1
+        IERC20 asset;
+        bool isDepleted;
+        bool isStream;
+        bool isTransferable;
+        // slot 2 and 3
+        Lockup.Amounts amounts;
+    }
 }
 
 /// @notice Namespace for the structs used in {SablierV2LockupDynamic}.
@@ -149,35 +178,20 @@ library LockupDynamic {
         uint40 duration;
     }
 
-    /// @notice Lockup Dynamic stream.
-    /// @dev The fields are arranged like this to save gas via tight variable packing.
-    /// @param sender The address streaming the assets, with the ability to cancel the stream.
-    /// @param startTime The Unix timestamp indicating the stream's start.
-    /// @param endTime The Unix timestamp indicating the stream's end.
-    /// @param isCancelable Boolean indicating if the stream is cancelable.
-    /// @param wasCanceled Boolean indicating if the stream was canceled.
-    /// @param asset The contract address of the ERC-20 asset used for streaming.
-    /// @param isDepleted Boolean indicating if the stream is depleted.
-    /// @param isStream Boolean indicating if the struct entity exists.
-    /// @param isTransferable Boolean indicating if the stream NFT is transferable.
-    /// @param amounts Struct containing the deposit, withdrawn, and refunded amounts, all denoted in units of the
-    /// asset's decimals.
-    /// @param segments Segments used to compose the custom streaming curve.
-    struct Stream {
-        // slot 0
+    /// @notice Struct encapsulating all the data for a specific id, allowing anyone to retrieve all information within
+    /// one call to the contract.
+    /// @dev It contains the same data as the `Lockup.Stream` struct, plus the segments.
+    struct StreamLD {
         address sender;
         uint40 startTime;
         uint40 endTime;
         bool isCancelable;
         bool wasCanceled;
-        // slot 1
         IERC20 asset;
         bool isDepleted;
         bool isStream;
         bool isTransferable;
-        // slot 2 and 3
         Lockup.Amounts amounts;
-        // slots [4..n]
         Segment[] segments;
     }
 }
@@ -241,42 +255,28 @@ library LockupLinear {
 
     /// @notice Struct encapsulating the time range.
     /// @param start The Unix timestamp for the stream's start.
-    /// @param cliff The Unix timestamp for the cliff period's end.
+    /// @param cliff The Unix timestamp for the cliff period's end. A value of zero means there is no cliff.
     /// @param end The Unix timestamp for the stream's end.
     struct Range {
         uint40 start;
         uint40 cliff;
         uint40 end;
     }
 
-    /// @notice Lockup Linear stream.
-    /// @dev The fields are arranged like this to save gas via tight variable packing.
-    /// @param sender The address streaming the assets, with the ability to cancel the stream.
-    /// @param startTime The Unix timestamp indicating the stream's start.
-    /// @param cliffTime The Unix timestamp indicating the cliff period's end.
-    /// @param isCancelable Boolean indicating if the stream is cancelable.
-    /// @param wasCanceled Boolean indicating if the stream was canceled.
-    /// @param asset The contract address of the ERC-20 asset used for streaming.
-    /// @param endTime The Unix timestamp indicating the stream's end.
-    /// @param isDepleted Boolean indicating if the stream is depleted.
-    /// @param isStream Boolean indicating if the struct entity exists.
-    /// @param isTransferable Boolean indicating if the stream NFT is transferable.
-    /// @param amounts Struct containing the deposit, withdrawn, and refunded amounts, all denoted in units of the
-    /// asset's decimals.
-    struct Stream {
-        // slot 0
+    /// @notice Struct encapsulating all the data for a specific id, allowing anyone to retrieve all information within
+    /// one call to the contract.
+    /// @dev It contains the same data as the `Lockup.Stream` struct, plus the cliff value.
+    struct StreamLL {
         address sender;
         uint40 startTime;
-        uint40 cliffTime;
         bool isCancelable;
         bool wasCanceled;
-        // slot 1
         IERC20 asset;
         uint40 endTime;
         bool isDepleted;
         bool isStream;
         bool isTransferable;
-        // slot 2 and 3
         Lockup.Amounts amounts;
+        uint40 cliffTime;
     }
 }
diff --git a/test/fork/LockupDynamic.t.sol b/test/fork/LockupDynamic.t.sol
@@ -197,7 +197,7 @@ abstract contract LockupDynamic_Fork_Test is Fork_Test {
         vars.isCancelable = vars.isSettled ? false : true;
 
         // Assert that the stream has been created.
-        LockupDynamic.Stream memory actualStream = lockupDynamic.getStream(vars.streamId);
+        LockupDynamic.StreamLD memory actualStream = lockupDynamic.getStream(vars.streamId);
         assertEq(actualStream.amounts, Lockup.Amounts(vars.createAmounts.deposit, 0, 0));
         assertEq(actualStream.asset, ASSET, "asset");
         assertEq(actualStream.endTime, vars.range.end, "endTime");

diff --git a/test/fork/LockupLinear.t.sol b/test/fork/LockupLinear.t.sol
@@ -121,7 +121,7 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
         params.broker.fee = _bound(params.broker.fee, 0, MAX_FEE);
         params.protocolFee = _bound(params.protocolFee, 0, MAX_FEE);
         params.range.start = boundUint40(params.range.start, currentTime - 1000 seconds, currentTime + 10_000 seconds);
-        params.range.cliff = boundUint40(params.range.cliff, params.range.start, params.range.start + 52 weeks);
+        params.range.cliff = boundUint40(params.range.cliff, params.range.start + 1, params.range.start + 52 weeks);
         params.totalAmount = boundUint128(params.totalAmount, 1, uint128(initialHolderBalance));
         params.transferable = true;
 
@@ -193,7 +193,7 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
         );
 
         // Assert that the stream has been created.
-        LockupLinear.Stream memory actualStream = lockupLinear.getStream(vars.streamId);
+        LockupLinear.StreamLL memory actualStream = lockupLinear.getStream(vars.streamId);
         assertEq(actualStream.amounts, Lockup.Amounts(vars.createAmounts.deposit, 0, 0));
         assertEq(actualStream.asset, ASSET, "asset");
         assertEq(actualStream.cliffTime, params.range.cliff, "cliffTime");

diff --git a/test/integration/concrete/lockup-dynamic/create-with-durations/createWithDurations.t.sol b/test/integration/concrete/lockup-dynamic/create-with-durations/createWithDurations.t.sol
@@ -171,8 +171,8 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
         createDefaultStreamWithDurations();
 
         // Assert that the stream has been created.
-        LockupDynamic.Stream memory actualStream = lockupDynamic.getStream(streamId);
-        LockupDynamic.Stream memory expectedStream = defaults.lockupDynamicStream();
+        LockupDynamic.StreamLD memory actualStream = lockupDynamic.getStream(streamId);
+        LockupDynamic.StreamLD memory expectedStream = defaults.lockupDynamicStream();
         expectedStream.endTime = range.end;
         expectedStream.segments = segments;
         expectedStream.startTime = range.start;

diff --git a/test/integration/concrete/lockup-dynamic/create-with-timestamps/createWithTimestamps.t.sol b/test/integration/concrete/lockup-dynamic/create-with-timestamps/createWithTimestamps.t.sol
@@ -383,8 +383,8 @@ contract CreateWithTimestamps_LockupDynamic_Integration_Concrete_Test is
         streamId = createDefaultStreamWithAsset(IERC20(asset));
 
         // Assert that the stream has been created.
-        LockupDynamic.Stream memory actualStream = lockupDynamic.getStream(streamId);
-        LockupDynamic.Stream memory expectedStream = defaults.lockupDynamicStream();
+        LockupDynamic.StreamLD memory actualStream = lockupDynamic.getStream(streamId);
+        LockupDynamic.StreamLD memory expectedStream = defaults.lockupDynamicStream();
         expectedStream.asset = IERC20(asset);
         assertEq(actualStream, expectedStream);
 

diff --git a/test/integration/concrete/lockup-dynamic/get-stream/getStream.t.sol b/test/integration/concrete/lockup-dynamic/get-stream/getStream.t.sol
@@ -26,8 +26,8 @@ contract GetStream_LockupDynamic_Integration_Concrete_Test is LockupDynamic_Inte
 
     function test_GetStream_StatusSettled() external givenNotNull {
         vm.warp({ timestamp: defaults.END_TIME() });
-        LockupDynamic.Stream memory actualStream = lockupDynamic.getStream(defaultStreamId);
-        LockupDynamic.Stream memory expectedStream = defaults.lockupDynamicStream();
+        LockupDynamic.StreamLD memory actualStream = lockupDynamic.getStream(defaultStreamId);
+        LockupDynamic.StreamLD memory expectedStream = defaults.lockupDynamicStream();
         expectedStream.isCancelable = false;
         assertEq(actualStream, expectedStream);
     }
@@ -38,8 +38,8 @@ contract GetStream_LockupDynamic_Integration_Concrete_Test is LockupDynamic_Inte
 
     function test_GetStream() external givenNotNull givenStatusNotSettled {
         uint256 streamId = createDefaultStream();
-        LockupDynamic.Stream memory actualStream = lockupDynamic.getStream(streamId);
-        LockupDynamic.Stream memory expectedStream = defaults.lockupDynamicStream();
+        LockupDynamic.StreamLD memory actualStream = lockupDynamic.getStream(streamId);
+        LockupDynamic.StreamLD memory expectedStream = defaults.lockupDynamicStream();
         assertEq(actualStream, expectedStream);
     }
 }
diff --git a/test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.t.sol b/test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.t.sol
@@ -28,30 +28,6 @@ contract CreateWithDurations_LockupLinear_Integration_Concrete_Test is
         expectRevertDueToDelegateCall(success, returnData);
     }
 
-    function test_RevertWhen_CliffDurationCalculationOverflows() external whenNotDelegateCalled {
-        uint40 startTime = getBlockTimestamp();
-        uint40 cliffDuration = MAX_UINT40 - startTime + 1 seconds;
-
-        // Calculate the end time. Needs to be "unchecked" to avoid an overflow.
-        uint40 cliffTime;
-        unchecked {
-            cliffTime = startTime + cliffDuration;
-        }
-
-        // Expect the relevant error to be thrown.
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                Errors.SablierV2LockupLinear_StartTimeGreaterThanCliffTime.selector, startTime, cliffTime
-            )
-        );
-
-        // Set the total duration to be the same as the cliff duration.
-        uint40 totalDuration = cliffDuration;
-
-        // Create the stream.
-        createDefaultStreamWithDurations(LockupLinear.Durations({ cliff: cliffDuration, total: totalDuration }));
-    }
-
     function test_RevertWhen_TotalDurationCalculationOverflows()
         external
         whenNotDelegateCalled
@@ -61,7 +37,7 @@ contract CreateWithDurations_LockupLinear_Integration_Concrete_Test is
         LockupLinear.Durations memory durations =
             LockupLinear.Durations({ cliff: 0, total: MAX_UINT40 - startTime + 1 seconds });
 
-        // Calculate the cliff time and the end time. Needs to be "unchecked" to avoid an overflow.
+        // Calculate the cliff time and the end time. Needs to be "unchecked" to allow an overflow.
         uint40 cliffTime;
         uint40 endTime;
         unchecked {
@@ -72,7 +48,7 @@ contract CreateWithDurations_LockupLinear_Integration_Concrete_Test is
         // Expect the relevant error to be thrown.
         vm.expectRevert(
             abi.encodeWithSelector(
-                Errors.SablierV2LockupLinear_CliffTimeNotLessThanEndTime.selector, cliffTime, endTime
+                Errors.SablierV2LockupLinear_StartTimeNotLessThanEndTime.selector, startTime, endTime
             )
         );
 
@@ -131,8 +107,8 @@ contract CreateWithDurations_LockupLinear_Integration_Concrete_Test is
         createDefaultStreamWithDurations();
 
         // Assert that the stream has been created.
-        LockupLinear.Stream memory actualStream = lockupLinear.getStream(streamId);
-        LockupLinear.Stream memory expectedStream = defaults.lockupLinearStream();
+        LockupLinear.StreamLL memory actualStream = lockupLinear.getStream(streamId);
+        LockupLinear.StreamLL memory expectedStream = defaults.lockupLinearStream();
         expectedStream.startTime = range.start;
         expectedStream.cliffTime = range.cliff;
         expectedStream.endTime = range.end;

diff --git a/test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.tree b/test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.tree
@@ -2,9 +2,6 @@ createWithDurations.t.sol
 ├── when delegate called
 │  └── it should revert
 └── when not delegate called
-   ├── when the cliff duration calculation overflows uint256
-   │  └── it should revert due to the start time being greater than the cliff time
-   └── when the cliff duration calculation does not overflow uint256
       ├── when the total duration calculation overflows uint256
       │   └── it should revert
       └── when the total duration calculation does not overflow uint256