From d1dc780e131e6cbb640b759647b484a4b1dc0786 Mon Sep 17 00:00:00 2001 From: andreivladbrg Date: Thu, 30 Mar 2023 15:10:41 +0300 Subject: [PATCH 1/9] fix: disallow setting the end time in the past docs: specify that the end time can't be in the past test: when the current time is not less than end time --- src/interfaces/ISablierV2LockupDynamic.sol | 1 + src/interfaces/ISablierV2LockupLinear.sol | 3 +- src/libraries/Errors.sol | 4 +++ src/libraries/Helpers.sol | 20 +++++++++-- .../createWithMilestones.t.sol | 33 +++++++++++++++++ .../createWithMilestones.tree | 31 ++++++++-------- .../create-with-range/createWithRange.t.sol | 35 +++++++++++++++++++ .../create-with-range/createWithRange.tree | 23 ++++++------ 8 files changed, 122 insertions(+), 28 deletions(-) diff --git a/src/interfaces/ISablierV2LockupDynamic.sol b/src/interfaces/ISablierV2LockupDynamic.sol index 108589a40..03d557bb0 100644 --- a/src/interfaces/ISablierV2LockupDynamic.sol +++ b/src/interfaces/ISablierV2LockupDynamic.sol @@ -111,6 +111,7 @@ interface ISablierV2LockupDynamic is ISablierV2Lockup { /// - The first segment's milestone must be greater than or equal to `params.startTime`. /// - The segment milestones must be arranged in ascending order. /// - `params.startTime` must not be greater than the last segment's milestone. + /// - The current time must not be greater than or equal to `params.range.end`. /// - The sum of the segment amounts must be equal to the deposit amount. /// - `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/interfaces/ISablierV2LockupLinear.sol b/src/interfaces/ISablierV2LockupLinear.sol index 3c9c26b70..bc45654f4 100644 --- a/src/interfaces/ISablierV2LockupLinear.sol +++ b/src/interfaces/ISablierV2LockupLinear.sol @@ -102,7 +102,8 @@ interface ISablierV2LockupLinear is ISablierV2Lockup { /// - `params.totalAmount` must not be zero. /// - If set, `params.broker.fee` must not be greater than `MAX_FEE`. /// - `params.range.start` must not be greater than `params.range.cliff`. - /// - `params.range.cliff` must not be greater than `params.range.end`. + /// - `params.range.cliff` must not be greater than or equal to `params.range.end`. + /// - The current time must not be greater than or equal to `params.range.end`. /// - `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 index 55588a91a..bbff9ba5e 100644 --- a/src/libraries/Errors.sol +++ b/src/libraries/Errors.sol @@ -50,6 +50,10 @@ library Errors { /// @notice Thrown when the broker fee is greater than the maximum fee permitted. error SablierV2Lockup_BrokerFeeTooHigh(UD60x18 brokerFee, UD60x18 maxFee); + /// @notice Thrown when attempting to create a stream with a current time that is not strictly less + /// than the end time. + error SablierV2Lockup_CurrentTimeNotLessThanEndTime(uint40 currentTime, uint40 endTime); + /// @notice Thrown when attempting to create a stream with a zero deposit amount. error SablierV2Lockup_DepositAmountZero(); diff --git a/src/libraries/Helpers.sol b/src/libraries/Helpers.sol index 8832fbc92..f50fb0618 100644 --- a/src/libraries/Helpers.sol +++ b/src/libraries/Helpers.sol @@ -64,7 +64,7 @@ library Helpers { uint40 startTime ) internal - pure + view { // Checks: the deposit amount is not zero. if (depositAmount == 0) { @@ -87,7 +87,7 @@ library Helpers { } /// @dev Checks the parameters of the {SablierV2LockupLinear-_createWithRange} function. - function checkCreateLinearParams(uint128 depositAmount, LockupLinear.Range memory range) internal pure { + function checkCreateLinearParams(uint128 depositAmount, LockupLinear.Range memory range) internal view { // Checks: the deposit amount is not zero. if (depositAmount == 0) { revert Errors.SablierV2Lockup_DepositAmountZero(); @@ -102,6 +102,13 @@ library Helpers { if (range.cliff >= range.end) { revert Errors.SablierV2LockupLinear_CliffTimeNotLessThanEndTime(range.cliff, range.end); } + + uint40 currentTime = uint40(block.timestamp); + + // Checks: the current time is strictly less than the end time. + if (currentTime >= range.end) { + revert Errors.SablierV2Lockup_CurrentTimeNotLessThanEndTime(currentTime, range.end); + } } /// @dev Checks that the segment array counts match, and then adjusts the segments by calculating the milestones. @@ -153,7 +160,7 @@ library Helpers { uint40 startTime ) private - pure + view { // Checks: the start time is strictly less than the first segment milestone. if (startTime >= segments[0].milestone) { @@ -191,6 +198,13 @@ library Helpers { } } + uint40 currentTime = uint40(block.timestamp); + + // Checks: the current time is strictly less than the end time. + if (currentTime >= currentMilestone) { + revert Errors.SablierV2Lockup_CurrentTimeNotLessThanEndTime(currentTime, currentMilestone); + } + // Check that the deposit amount is equal to the segment amounts sum. if (depositAmount != segmentAmountsSum) { revert Errors.SablierV2LockupDynamic_DepositAmountNotEqualToSegmentAmountsSum( diff --git a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol index b2968c767..56e1b549a 100644 --- a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol +++ b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol @@ -206,6 +206,33 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { _; } + /// @dev it should revert. + function test_RevertWhen_CurrentTimeNotLessThanEndTime() + external + whenNoDelegateCall + whenRecipientNonZeroAddress + whenDepositAmountNotZero + whenSegmentCountNotZero + whenSegmentCountNotTooHigh + whenSegmentAmountsSumDoesNotOverflow + whenStartTimeLessThanFirstSegmentMilestone + whenSegmentMilestonesOrdered + whenCurrentTimeLessThanEndTime + { + vm.warp(uint256(DEFAULT_END_TIME)); + + vm.expectRevert( + abi.encodeWithSelector( + Errors.SablierV2Lockup_CurrentTimeNotLessThanEndTime.selector, DEFAULT_END_TIME, DEFAULT_END_TIME + ) + ); + createDefaultStream(); + } + + modifier whenCurrentTimeLessThanEndTime() { + _; + } + /// @dev it should revert. function test_RevertWhen_DepositAmountNotEqualToSegmentAmountsSum() external @@ -217,6 +244,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered + whenCurrentTimeLessThanEndTime { // Disable both the protocol and the broker fee so that they don't interfere with the calculations. changePrank({ msgSender: users.admin }); @@ -258,6 +286,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered + whenCurrentTimeLessThanEndTime whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum { @@ -289,6 +318,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered + whenCurrentTimeLessThanEndTime whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum whenProtocolFeeNotTooHigh @@ -313,6 +343,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered + whenCurrentTimeLessThanEndTime whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum whenProtocolFeeNotTooHigh @@ -346,6 +377,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered + whenCurrentTimeLessThanEndTime whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum whenProtocolFeeNotTooHigh @@ -371,6 +403,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered + whenCurrentTimeLessThanEndTime whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum whenProtocolFeeNotTooHigh diff --git a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.tree b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.tree index cb37f90c8..9960d6278 100644 --- a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.tree +++ b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.tree @@ -28,21 +28,24 @@ createWithMilestones.t.sol ├── when the segment milestones are not ordered │ └── it should revert └── when the segment milestones are ordered - ├── when the deposit amount is not equal to the segment amounts sum + ├── when the current time is not less than the end time │ └── it should revert - └── when the deposit amount is equal to the segment amounts sum - ├── when the protocol fee is too high + └── when the current time is less than the end time + ├── when the deposit amount is not equal to the segment amounts sum │ └── it should revert - └── when the protocol fee is not too high - ├── when the broker fee is too high + └── when the deposit amount is equal to the segment amounts sum + ├── when the protocol fee is too high │ └── it should revert - └── when the broker fee is not too high - ├── when the asset is not a contract + └── when the protocol fee is not too high + ├── when the broker fee is too high │ └── it should revert - ├── when the asset is not a contract - │ └── it should revert - └── when the asset is a contract - ├── when the asset misses the return value - │ └── it should perform the ERC-20 transfers, create the stream, bump the next stream id, and mint the NFT - └── when the asset is ERC-20 compliant - └── it should perform the ERC-20 transfers, create the stream, bump the next stream id, record the protocol fee, mint the NFT, and emit a {CreateLockupDynamicStream} event + └── when the broker fee is not too high + ├── when the asset is not a contract + │ └── it should revert + ├── when the asset is not a contract + │ └── it should revert + └── when the asset is a contract + ├── when the asset misses the return value + │ └── it should perform the ERC-20 transfers, create the stream, bump the next stream id, and mint the NFT + └── when the asset is ERC-20 compliant + └── it should perform the ERC-20 transfers, create the stream, bump the next stream id, record the protocol fee, mint the NFT, and emit a {CreateLockupDynamicStream} event diff --git a/test/unit/lockup/linear/create-with-range/createWithRange.t.sol b/test/unit/lockup/linear/create-with-range/createWithRange.t.sol index 8abd203e7..1abf0fdab 100644 --- a/test/unit/lockup/linear/create-with-range/createWithRange.t.sol +++ b/test/unit/lockup/linear/create-with-range/createWithRange.t.sol @@ -78,6 +78,7 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { /// @dev it should revert. function test_RevertWhen_CliffTimeNotLessThanEndTime() external + whenNoDelegateCall whenRecipientNonZeroAddress whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime @@ -96,13 +97,39 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { _; } + /// @dev it should revert. + function test_RevertWhen_CurrentTimeNotLessThanEndTime() + external + whenNoDelegateCall + whenRecipientNonZeroAddress + whenDepositAmountNotZero + whenStartTimeNotGreaterThanCliffTime + whenCliffTimeLessThanEndTime + whenCurrentTimeLessThanEndTime + { + vm.warp(uint256(DEFAULT_END_TIME)); + + vm.expectRevert( + abi.encodeWithSelector( + Errors.SablierV2Lockup_CurrentTimeNotLessThanEndTime.selector, DEFAULT_END_TIME, DEFAULT_END_TIME + ) + ); + createDefaultStream(); + } + + modifier whenCurrentTimeLessThanEndTime() { + _; + } + /// @dev it should revert. function test_RevertWhen_ProtocolFeeTooHigh() external + whenNoDelegateCall whenRecipientNonZeroAddress whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime + whenCurrentTimeLessThanEndTime { UD60x18 protocolFee = MAX_FEE.add(ud(1)); @@ -124,10 +151,12 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { /// @dev it should revert. function test_RevertWhen_BrokerFeeTooHigh() external + whenNoDelegateCall whenRecipientNonZeroAddress whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime + whenCurrentTimeLessThanEndTime whenProtocolFeeNotTooHigh { UD60x18 brokerFee = MAX_FEE.add(ud(1)); @@ -142,10 +171,12 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { /// @dev it should revert. function test_RevertWhen_AssetNotContract() external + whenNoDelegateCall whenRecipientNonZeroAddress whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime + whenCurrentTimeLessThanEndTime whenProtocolFeeNotTooHigh whenBrokerFeeNotTooHigh { @@ -161,10 +192,12 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { /// @dev it should perform the ERC-20 transfers, create the stream, bump the next stream id, and mint the NFT. function test_CreateWithRange_AssetMissingReturnValue() external + whenNoDelegateCall whenRecipientNonZeroAddress whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime + whenCurrentTimeLessThanEndTime whenProtocolFeeNotTooHigh whenBrokerFeeNotTooHigh whenAssetContract @@ -186,9 +219,11 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { /// - Emit a {CreateLockupLinearStream} event. function test_CreateWithRange() external + whenNoDelegateCall whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime + whenCurrentTimeLessThanEndTime whenProtocolFeeNotTooHigh whenBrokerFeeNotTooHigh whenAssetContract diff --git a/test/unit/lockup/linear/create-with-range/createWithRange.tree b/test/unit/lockup/linear/create-with-range/createWithRange.tree index 292957196..67debe2e0 100644 --- a/test/unit/lockup/linear/create-with-range/createWithRange.tree +++ b/test/unit/lockup/linear/create-with-range/createWithRange.tree @@ -14,17 +14,20 @@ createWithRange.t.sol ├── when the cliff time is not less than the end time │ └── it should revert └── when the cliff time is less than the end time - ├── when the protocol fee is too high + ├── when the current time is not less than the end time │ └── it should revert - └── when the protocol fee is not too high - ├── when the broker fee is too high + └── when the current time is less than the end time + ├── when the protocol fee is too high │ └── it should revert - └── when the broker fee is not too high - ├── when the asset is not a contract + └── when the protocol fee is not too high + ├── when the broker fee is too high │ └── it should revert - └── when the asset is a contract - ├── when the asset misses the return value - │ └── it should perform the ERC-20 transfers, create the stream, bump the next stream id and mint the NFT - └── when the asset is ERC-20 compliant - └── it should perform the ERC-20 transfers, create the stream, bump the next stream id, record the protocol fee, mint the NFT, and emit a {CreateLockupLinearStream} event + └── when the broker fee is not too high + ├── when the asset is not a contract + │ └── it should revert + └── when the asset is a contract + ├── when the asset misses the return value + │ └── it should perform the ERC-20 transfers, create the stream, bump the next stream id and mint the NFT + └── when the asset is ERC-20 compliant + └── it should perform the ERC-20 transfers, create the stream, bump the next stream id, record the protocol fee, mint the NFT, and emit a {CreateLockupLinearStream} event From 4046093f35231f5356fad535c95e97b6bcaaed58 Mon Sep 17 00:00:00 2001 From: andreivladbrg Date: Thu, 30 Mar 2023 17:38:15 +0300 Subject: [PATCH 2/9] test: move warp cheatcode after the createDefault test: ensure the last milestone is greater than the current time in fuzzSegments test: bound end time between current time + 1 and MAX_UNIX_TIMESTAMP --- test/fork/lockup/linear/Linear.t.sol | 7 +++++++ .../streamed-amount-of/streamedAmountOf.sol | 14 +++++++------- test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol | 6 +++--- .../withdrawableAmountOf.t.sol | 13 +++++++------ .../linear/streamed-amount-of/streamedAmountOf.sol | 8 ++++---- .../withdrawableAmountOf.t.sol | 13 +++++++------ .../shared/cancel-multiple/cancelMultiple.t.sol | 12 ++++++------ test/fuzz/lockup/shared/cancel/cancel.t.sol | 12 ++++++------ .../withdraw-multiple/withdrawMultiple.t.sol | 6 +++--- test/fuzz/lockup/shared/withdraw/withdraw.t.sol | 6 +++--- .../handlers/LockupLinearCreateHandler.t.sol | 8 +++++++- test/invariant/lockup/linear/Linear.t.sol | 2 +- test/shared/Fuzzers.t.sol | 9 +++++++++ 13 files changed, 70 insertions(+), 46 deletions(-) diff --git a/test/fork/lockup/linear/Linear.t.sol b/test/fork/lockup/linear/Linear.t.sol index bf6e3d6b8..3a6ba697e 100644 --- a/test/fork/lockup/linear/Linear.t.sol +++ b/test/fork/lockup/linear/Linear.t.sol @@ -116,6 +116,13 @@ abstract contract Linear_Fork_Test is Fork_Test { params.protocolFee = bound(params.protocolFee, 0, MAX_FEE); params.totalAmount = boundUint128(params.totalAmount, 1, uint128(initialHolderBalance)); + // If the cliff time is in the past we want to make sure that the end time is not. + if (params.range.cliff >= getBlockTimestamp()) { + params.range.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP); + } else { + params.range.end = boundUint40(params.range.end, getBlockTimestamp() + 1, MAX_UNIX_TIMESTAMP); + } + // Set the fuzzed protocol fee. changePrank({ msgSender: users.admin }); comptroller.setProtocolFee({ asset: asset, newProtocolFee: params.protocolFee }); diff --git a/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol b/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol index 0b6a744fe..fdaf0d940 100644 --- a/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol +++ b/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol @@ -32,10 +32,6 @@ contract StreamedAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { { timeWarp = boundUint40(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); - // Warp into the future. - uint40 currentTime = DEFAULT_START_TIME + timeWarp; - vm.warp({ timestamp: currentTime }); - // Create a single-element segment array. LockupDynamic.Segment[] memory segments = new LockupDynamic.Segment[](1); segments[0] = LockupDynamic.Segment({ @@ -47,6 +43,10 @@ contract StreamedAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { // Create the stream with the one-segment array. uint256 streamId = createDefaultStreamWithSegments(segments); + // Warp into the future. + uint40 currentTime = DEFAULT_START_TIME + timeWarp; + vm.warp({ timestamp: currentTime }); + // Run the test. uint128 actualStreamedAmount = dynamic.streamedAmountOf(streamId); uint128 expectedStreamedAmount = @@ -78,13 +78,13 @@ contract StreamedAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { { timeWarp = boundUint40(timeWarp, MAX_SEGMENTS[0].milestone, DEFAULT_TOTAL_DURATION * 2); + // Create the stream with the multiple-segment array. + uint256 streamId = createDefaultStreamWithSegments(MAX_SEGMENTS); + // Warp into the future. uint40 currentTime = DEFAULT_START_TIME + timeWarp; vm.warp({ timestamp: currentTime }); - // Create the stream with the multiple-segment array. - uint256 streamId = createDefaultStreamWithSegments(MAX_SEGMENTS); - // Run the test. uint128 actualStreamedAmount = dynamic.streamedAmountOf(streamId); uint128 expectedStreamedAmount = diff --git a/test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol b/test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol index 89aa50e1a..aced47515 100644 --- a/test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol +++ b/test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol @@ -62,9 +62,6 @@ contract Withdraw_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test, Withdraw_Fuzz_Test { // Bound the time warp. params.timeWarp = bound(params.timeWarp, 1, params.segments[params.segments.length - 1].milestone); - // Warp into the future. - vm.warp({ timestamp: DEFAULT_START_TIME + params.timeWarp }); - // Mint enough ERC-20 assets to the sender. deal({ token: address(DEFAULT_ASSET), to: vars.funder, give: vars.totalAmount }); @@ -82,6 +79,9 @@ contract Withdraw_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test, Withdraw_Fuzz_Test { }) ); + // Warp into the future. + vm.warp({ timestamp: DEFAULT_START_TIME + params.timeWarp }); + // Bound the withdraw amount. vars.withdrawableAmount = dynamic.withdrawableAmountOf(vars.streamId); vars.withdrawAmount = boundUint128(vars.withdrawAmount, 1, vars.withdrawableAmount); diff --git a/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol index 4676c78c6..3eb34343a 100644 --- a/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -39,10 +39,6 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { { timeWarp = boundUint40(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); - // Warp into the future. - uint40 currentTime = DEFAULT_START_TIME + timeWarp; - vm.warp({ timestamp: currentTime }); - // Create the stream with a custom total amount. The broker fee is disabled so that it doesn't interfere with // the calculations. LockupDynamic.CreateWithMilestones memory params = defaultParams.createWithMilestones; @@ -50,6 +46,10 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { params.broker = Broker({ account: address(0), fee: ZERO }); uint256 streamId = dynamic.createWithMilestones(params); + // Warp into the future. + uint40 currentTime = DEFAULT_START_TIME + timeWarp; + vm.warp({ timestamp: currentTime }); + // Run the test. uint128 actualWithdrawableAmount = dynamic.withdrawableAmountOf(streamId); uint128 expectedWithdrawableAmount = @@ -80,9 +80,7 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { { timeWarp = boundUint40(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); - // Warp into the future. uint40 currentTime = DEFAULT_START_TIME + timeWarp; - vm.warp({ timestamp: currentTime }); // Bound the withdraw amount. uint128 streamedAmount = @@ -96,6 +94,9 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { params.broker = Broker({ account: address(0), fee: ZERO }); uint256 streamId = dynamic.createWithMilestones(params); + // Warp into the future. + vm.warp({ timestamp: currentTime }); + // Make the withdrawal. dynamic.withdraw({ streamId: streamId, to: users.recipient, amount: withdrawAmount }); diff --git a/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol b/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol index 3c37e5199..e0d3a11bb 100644 --- a/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol +++ b/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol @@ -52,10 +52,6 @@ contract StreamedAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { vm.assume(depositAmount != 0); timeWarp = boundUint40(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); - // Warp into the future. - uint40 currentTime = DEFAULT_START_TIME + timeWarp; - vm.warp({ timestamp: currentTime }); - // Mint enough ERC-20 assets to the sender. deal({ token: address(DEFAULT_ASSET), to: users.sender, give: depositAmount }); @@ -65,6 +61,10 @@ contract StreamedAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { params.broker = Broker({ account: address(0), fee: ZERO }); uint256 streamId = linear.createWithRange(params); + // Warp into the future. + uint40 currentTime = DEFAULT_START_TIME + timeWarp; + vm.warp({ timestamp: currentTime }); + // Run the test. uint128 actualStreamedAmount = linear.streamedAmountOf(streamId); uint128 expectedStreamedAmount = calculateStreamedAmount(currentTime, depositAmount); diff --git a/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol index 0eb848ee7..76de2b98f 100644 --- a/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -52,10 +52,6 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { vm.assume(depositAmount != 0); timeWarp = boundUint40(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); - // Warp into the future. - uint40 currentTime = DEFAULT_START_TIME + timeWarp; - vm.warp({ timestamp: currentTime }); - // Mint enough ERC-20 assets to the sender. deal({ token: address(DEFAULT_ASSET), to: users.sender, give: depositAmount }); @@ -65,6 +61,10 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { params.broker = Broker({ account: address(0), fee: ZERO }); uint256 streamId = linear.createWithRange(params); + // Warp into the future. + uint40 currentTime = DEFAULT_START_TIME + timeWarp; + vm.warp({ timestamp: currentTime }); + // Run the test. uint128 actualWithdrawableAmount = linear.withdrawableAmountOf(streamId); uint128 expectedWithdrawableAmount = calculateStreamedAmount(currentTime, depositAmount); @@ -92,9 +92,7 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { timeWarp = boundUint40(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); depositAmount = boundUint128(depositAmount, 10_000, UINT128_MAX); - // Warp into the future. uint40 currentTime = DEFAULT_START_TIME + timeWarp; - vm.warp({ timestamp: currentTime }); // Bound the withdraw amount. uint128 streamedAmount = calculateStreamedAmount(currentTime, depositAmount); @@ -109,6 +107,9 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { params.broker = Broker({ account: address(0), fee: ZERO }); uint256 streamId = linear.createWithRange(params); + // Warp into the future. + vm.warp({ timestamp: currentTime }); + // Make the withdrawal. linear.withdraw({ streamId: streamId, to: users.recipient, amount: withdrawAmount }); diff --git a/test/fuzz/lockup/shared/cancel-multiple/cancelMultiple.t.sol b/test/fuzz/lockup/shared/cancel-multiple/cancelMultiple.t.sol index ba3de832c..fe7a83fd9 100644 --- a/test/fuzz/lockup/shared/cancel-multiple/cancelMultiple.t.sol +++ b/test/fuzz/lockup/shared/cancel-multiple/cancelMultiple.t.sol @@ -62,15 +62,15 @@ abstract contract CancelMultiple_Fuzz_Test is Fuzz_Test, Lockup_Shared_Test { timeWarp = bound(timeWarp, 0 seconds, DEFAULT_TOTAL_DURATION * 2); endTime = boundUint40(endTime, DEFAULT_CLIFF_TIME + 1, DEFAULT_END_TIME + DEFAULT_TOTAL_DURATION / 2); - // Warp into the future. - vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); - // Make the sender the caller in this test. changePrank({ msgSender: users.sender }); // Create a new stream with a different end time. uint256 streamId = createDefaultStreamWithEndTime(endTime); + // Warp into the future. + vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); + // Create the stream ids array. uint256[] memory streamIds = Solarray.uint256s(defaultStreamIds[0], streamId); @@ -148,15 +148,15 @@ abstract contract CancelMultiple_Fuzz_Test is Fuzz_Test, Lockup_Shared_Test { timeWarp = bound(timeWarp, 0 seconds, DEFAULT_TOTAL_DURATION * 2); endTime = boundUint40(endTime, DEFAULT_CLIFF_TIME + 1, DEFAULT_END_TIME + DEFAULT_TOTAL_DURATION / 2); - // Warp into the future. - vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); - // Make the recipient the caller in this test. changePrank({ msgSender: users.recipient }); // Create a new stream with a different end time. uint256 streamId = createDefaultStreamWithEndTime(endTime); + // Warp into the future. + vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); + // Create the stream ids array. uint256[] memory streamIds = Solarray.uint256s(defaultStreamIds[0], streamId); diff --git a/test/fuzz/lockup/shared/cancel/cancel.t.sol b/test/fuzz/lockup/shared/cancel/cancel.t.sol index 87f81086d..309e5c98d 100644 --- a/test/fuzz/lockup/shared/cancel/cancel.t.sol +++ b/test/fuzz/lockup/shared/cancel/cancel.t.sol @@ -79,12 +79,12 @@ abstract contract Cancel_Fuzz_Test is Fuzz_Test, Lockup_Shared_Test { { timeWarp = bound(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); - // Warp into the future. - vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); - // Create the stream. uint256 streamId = createDefaultStreamWithRecipient(address(goodRecipient)); + // Warp into the future. + vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); + // Bound the withdraw amount. uint128 streamedAmount = lockup.streamedAmountOf(streamId); withdrawAmount = boundUint128(withdrawAmount, 0, streamedAmount - 1); @@ -173,12 +173,12 @@ abstract contract Cancel_Fuzz_Test is Fuzz_Test, Lockup_Shared_Test { { timeWarp = bound(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); - // Warp into the future. - vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); - // Create the stream. uint256 streamId = createDefaultStreamWithSender(address(goodSender)); + // Warp into the future. + vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); + // Bound the withdraw amount. uint128 streamedAmount = lockup.streamedAmountOf(streamId); withdrawAmount = boundUint128(withdrawAmount, 0, streamedAmount - 1); diff --git a/test/fuzz/lockup/shared/withdraw-multiple/withdrawMultiple.t.sol b/test/fuzz/lockup/shared/withdraw-multiple/withdrawMultiple.t.sol index 67a969744..538da0ce2 100644 --- a/test/fuzz/lockup/shared/withdraw-multiple/withdrawMultiple.t.sol +++ b/test/fuzz/lockup/shared/withdraw-multiple/withdrawMultiple.t.sol @@ -253,9 +253,6 @@ abstract contract WithdrawMultiple_Fuzz_Test is Fuzz_Test, Lockup_Shared_Test { vm.assume(params.to != address(0)); params.timeWarp = bound(params.timeWarp, DEFAULT_TOTAL_DURATION, DEFAULT_TOTAL_DURATION * 2 - 1); - // Warp into the future. - vm.warp({ timestamp: DEFAULT_START_TIME + params.timeWarp }); - // Use the first default stream as the ended stream. Vars memory vars; vars.endedStreamId = defaultStreamIds[0]; @@ -265,6 +262,9 @@ abstract contract WithdrawMultiple_Fuzz_Test is Fuzz_Test, Lockup_Shared_Test { vars.ongoingEndTime = DEFAULT_END_TIME + DEFAULT_TOTAL_DURATION; vars.ongoingStreamId = createDefaultStreamWithEndTime(vars.ongoingEndTime); + // Warp into the future. + vm.warp({ timestamp: DEFAULT_START_TIME + params.timeWarp }); + // Bound the ongoing withdraw amount. vars.ongoingWithdrawableAmount = lockup.withdrawableAmountOf(vars.ongoingStreamId); params.ongoingWithdrawAmount = boundUint128(params.ongoingWithdrawAmount, 1, vars.ongoingWithdrawableAmount); diff --git a/test/fuzz/lockup/shared/withdraw/withdraw.t.sol b/test/fuzz/lockup/shared/withdraw/withdraw.t.sol index 229219aa5..44ec66767 100644 --- a/test/fuzz/lockup/shared/withdraw/withdraw.t.sol +++ b/test/fuzz/lockup/shared/withdraw/withdraw.t.sol @@ -204,12 +204,12 @@ abstract contract Withdraw_Fuzz_Test is Fuzz_Test, Lockup_Shared_Test { { timeWarp = bound(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); - // Warp into the future. - vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); - // Create the stream with a contract as the recipient. uint256 streamId = createDefaultStreamWithRecipient(address(goodRecipient)); + // Warp into the future. + vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); + // Bound the withdraw amount. uint128 withdrawableAmount = lockup.withdrawableAmountOf(streamId); withdrawAmount = boundUint128(withdrawAmount, 1, withdrawableAmount); diff --git a/test/invariant/handlers/LockupLinearCreateHandler.t.sol b/test/invariant/handlers/LockupLinearCreateHandler.t.sol index e979090ab..d7578bdf0 100644 --- a/test/invariant/handlers/LockupLinearCreateHandler.t.sol +++ b/test/invariant/handlers/LockupLinearCreateHandler.t.sol @@ -97,9 +97,15 @@ contract LockupLinearCreateHandler is BaseHandler { params.broker.fee = bound(params.broker.fee, 0, MAX_FEE); params.range.start = boundUint40(params.range.start, 0, DEFAULT_START_TIME); params.range.cliff = boundUint40(params.range.cliff, params.range.start, 52 weeks); - params.range.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP); params.totalAmount = boundUint128(params.totalAmount, 1, 1_000_000_000e18); + // If the cliff time is in the past we want to make sure that the end time is not. + if (params.range.cliff >= getBlockTimestamp()) { + params.range.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP); + } else { + params.range.end = boundUint40(params.range.end, getBlockTimestamp() + 1, MAX_UNIX_TIMESTAMP); + } + // Mint enough ERC-20 assets to the sender. deal({ token: address(asset), to: params.sender, give: asset.balanceOf(params.sender) + params.totalAmount }); diff --git a/test/invariant/lockup/linear/Linear.t.sol b/test/invariant/lockup/linear/Linear.t.sol index 7aff25a09..a8835a140 100644 --- a/test/invariant/lockup/linear/Linear.t.sol +++ b/test/invariant/lockup/linear/Linear.t.sol @@ -120,7 +120,7 @@ contract Linear_Invariant_Test is Lockup_Invariant_Test { for (uint256 i = 0; i < lastStreamId; ++i) { uint256 streamId = lockupHandlerStorage.streamIds(i); assertGt( - linear.getEndTime(streamId), linear.getCliffTime(streamId), "Invariant violated: end time < cliff time" + linear.getEndTime(streamId), linear.getCliffTime(streamId), "Invariant violated: end time <= cliff time" ); } } diff --git a/test/shared/Fuzzers.t.sol b/test/shared/Fuzzers.t.sol index e5b393e64..98136b8dc 100644 --- a/test/shared/Fuzzers.t.sol +++ b/test/shared/Fuzzers.t.sol @@ -137,6 +137,10 @@ abstract contract Fuzzers is Constants, Utils { // Return here if there's only one segment to not run into division by zero. uint40 segmentCount = uint40(segments.length); if (segmentCount == 1) { + // Make sure that the milestone is strictly greater than the current time. + if (segments[0].milestone <= getBlockTimestamp()) { + segments[0].milestone = getBlockTimestamp() + 1; + } return; } @@ -151,5 +155,10 @@ abstract contract Fuzzers is Constants, Utils { milestone = bound(milestone, milestone - halfStep, milestone + halfStep); segments[i].milestone = uint40(milestone); } + + // Make sure that the last milestone is strictly greater than the current time. + if (segments[segmentCount - 1].milestone <= getBlockTimestamp()) { + segments[segmentCount - 1].milestone = getBlockTimestamp() + 1; + } } } From b225731161804a14dcad6bf61878d2e54a14e66f Mon Sep 17 00:00:00 2001 From: andreivladbrg Date: Fri, 31 Mar 2023 00:19:31 +0300 Subject: [PATCH 3/9] refactor: use better naming for error chore: improve wording in explanatory comments test: improve naming for tests test: remove unnecessary casting --- src/libraries/Errors.sol | 5 ++-- src/libraries/Helpers.sol | 13 +++++------ .../createWithMilestones.t.sol | 23 ++++++++----------- .../createWithMilestones.tree | 4 ++-- .../create-with-range/createWithRange.t.sol | 22 ++++++++---------- .../create-with-range/createWithRange.tree | 4 ++-- 6 files changed, 32 insertions(+), 39 deletions(-) diff --git a/src/libraries/Errors.sol b/src/libraries/Errors.sol index bbff9ba5e..091cdd054 100644 --- a/src/libraries/Errors.sol +++ b/src/libraries/Errors.sol @@ -50,9 +50,8 @@ library Errors { /// @notice Thrown when the broker fee is greater than the maximum fee permitted. error SablierV2Lockup_BrokerFeeTooHigh(UD60x18 brokerFee, UD60x18 maxFee); - /// @notice Thrown when attempting to create a stream with a current time that is not strictly less - /// than the end time. - error SablierV2Lockup_CurrentTimeNotLessThanEndTime(uint40 currentTime, uint40 endTime); + /// @notice Thrown when attempting to create a stream with the end time in the past. + error SablierV2Lockup_EndTimeInThePast(uint40 currentTime, uint40 endTime); /// @notice Thrown when attempting to create a stream with a zero deposit amount. error SablierV2Lockup_DepositAmountZero(); diff --git a/src/libraries/Helpers.sol b/src/libraries/Helpers.sol index f50fb0618..459528b70 100644 --- a/src/libraries/Helpers.sol +++ b/src/libraries/Helpers.sol @@ -98,16 +98,15 @@ library Helpers { revert Errors.SablierV2LockupLinear_StartTimeGreaterThanCliffTime(range.start, range.cliff); } - // Checks: the cliff time is strictly less than the end time. + // Checks: the end time is not in the past. if (range.cliff >= range.end) { revert Errors.SablierV2LockupLinear_CliffTimeNotLessThanEndTime(range.cliff, range.end); } - uint40 currentTime = uint40(block.timestamp); - // Checks: the current time is strictly less than the end time. + uint40 currentTime = uint40(block.timestamp); if (currentTime >= range.end) { - revert Errors.SablierV2Lockup_CurrentTimeNotLessThanEndTime(currentTime, range.end); + revert Errors.SablierV2Lockup_EndTimeInThePast(currentTime, range.end); } } @@ -198,11 +197,11 @@ library Helpers { } } + // Checks: the end time is not in the past. + // When the loop exits, current milestone is actually the last milestone, i.e. the end time of the stream. uint40 currentTime = uint40(block.timestamp); - - // Checks: the current time is strictly less than the end time. if (currentTime >= currentMilestone) { - revert Errors.SablierV2Lockup_CurrentTimeNotLessThanEndTime(currentTime, currentMilestone); + revert Errors.SablierV2Lockup_EndTimeInThePast(currentTime, currentMilestone); } // Check that the deposit amount is equal to the segment amounts sum. diff --git a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol index 56e1b549a..eb391a287 100644 --- a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol +++ b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol @@ -207,7 +207,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { } /// @dev it should revert. - function test_RevertWhen_CurrentTimeNotLessThanEndTime() + function test_RevertWhen_EndTimeInThePast() external whenNoDelegateCall whenRecipientNonZeroAddress @@ -217,19 +217,16 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered - whenCurrentTimeLessThanEndTime { - vm.warp(uint256(DEFAULT_END_TIME)); + vm.warp(DEFAULT_END_TIME); vm.expectRevert( - abi.encodeWithSelector( - Errors.SablierV2Lockup_CurrentTimeNotLessThanEndTime.selector, DEFAULT_END_TIME, DEFAULT_END_TIME - ) + abi.encodeWithSelector(Errors.SablierV2Lockup_EndTimeInThePast.selector, DEFAULT_END_TIME, DEFAULT_END_TIME) ); createDefaultStream(); } - modifier whenCurrentTimeLessThanEndTime() { + modifier whenEndTimeNotInThePast() { _; } @@ -244,7 +241,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast { // Disable both the protocol and the broker fee so that they don't interfere with the calculations. changePrank({ msgSender: users.admin }); @@ -286,7 +283,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum { @@ -318,7 +315,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum whenProtocolFeeNotTooHigh @@ -343,7 +340,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum whenProtocolFeeNotTooHigh @@ -377,7 +374,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum whenProtocolFeeNotTooHigh @@ -403,7 +400,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenSegmentAmountsSumDoesNotOverflow whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenStartTimeLessThanFirstSegmentMilestone whenDepositAmountEqualToSegmentAmountsSum whenProtocolFeeNotTooHigh diff --git a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.tree b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.tree index 9960d6278..bc53e95a9 100644 --- a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.tree +++ b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.tree @@ -28,9 +28,9 @@ createWithMilestones.t.sol ├── when the segment milestones are not ordered │ └── it should revert └── when the segment milestones are ordered - ├── when the current time is not less than the end time + ├── when the end time is in the past │ └── it should revert - └── when the current time is less than the end time + └── when the end time is not in the past ├── when the deposit amount is not equal to the segment amounts sum │ └── it should revert └── when the deposit amount is equal to the segment amounts sum diff --git a/test/unit/lockup/linear/create-with-range/createWithRange.t.sol b/test/unit/lockup/linear/create-with-range/createWithRange.t.sol index 1abf0fdab..96f20c238 100644 --- a/test/unit/lockup/linear/create-with-range/createWithRange.t.sol +++ b/test/unit/lockup/linear/create-with-range/createWithRange.t.sol @@ -98,26 +98,24 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { } /// @dev it should revert. - function test_RevertWhen_CurrentTimeNotLessThanEndTime() + function test_RevertWhen_EndTimeInThePast() external whenNoDelegateCall whenRecipientNonZeroAddress whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast { - vm.warp(uint256(DEFAULT_END_TIME)); + vm.warp(DEFAULT_END_TIME); vm.expectRevert( - abi.encodeWithSelector( - Errors.SablierV2Lockup_CurrentTimeNotLessThanEndTime.selector, DEFAULT_END_TIME, DEFAULT_END_TIME - ) + abi.encodeWithSelector(Errors.SablierV2Lockup_EndTimeInThePast.selector, DEFAULT_END_TIME, DEFAULT_END_TIME) ); createDefaultStream(); } - modifier whenCurrentTimeLessThanEndTime() { + modifier whenEndTimeNotInThePast() { _; } @@ -129,7 +127,7 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast { UD60x18 protocolFee = MAX_FEE.add(ud(1)); @@ -156,7 +154,7 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenProtocolFeeNotTooHigh { UD60x18 brokerFee = MAX_FEE.add(ud(1)); @@ -176,7 +174,7 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenProtocolFeeNotTooHigh whenBrokerFeeNotTooHigh { @@ -197,7 +195,7 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenProtocolFeeNotTooHigh whenBrokerFeeNotTooHigh whenAssetContract @@ -223,7 +221,7 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { whenDepositAmountNotZero whenStartTimeNotGreaterThanCliffTime whenCliffTimeLessThanEndTime - whenCurrentTimeLessThanEndTime + whenEndTimeNotInThePast whenProtocolFeeNotTooHigh whenBrokerFeeNotTooHigh whenAssetContract diff --git a/test/unit/lockup/linear/create-with-range/createWithRange.tree b/test/unit/lockup/linear/create-with-range/createWithRange.tree index 67debe2e0..dd96a4cbe 100644 --- a/test/unit/lockup/linear/create-with-range/createWithRange.tree +++ b/test/unit/lockup/linear/create-with-range/createWithRange.tree @@ -14,9 +14,9 @@ createWithRange.t.sol ├── when the cliff time is not less than the end time │ └── it should revert └── when the cliff time is less than the end time - ├── when the current time is not less than the end time + ├── when the end time is in the past │ └── it should revert - └── when the current time is less than the end time + └── when the end time is not in the past ├── when the protocol fee is too high │ └── it should revert └── when the protocol fee is not too high From 8f846e44e1b6097274acb6dd2209eb57850eebd0 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Fri, 31 Mar 2023 13:03:31 +0300 Subject: [PATCH 4/9] refactor: alphabetical ordering chore: fix explanations in comments test: use named arguments for "vm.warp" --- src/libraries/Errors.sol | 6 +++--- src/libraries/Helpers.sol | 6 +++--- .../withdrawable-amount-of/withdrawableAmountOf.t.sol | 1 + .../withdrawable-amount-of/withdrawableAmountOf.t.sol | 1 + .../create-with-milestones/createWithMilestones.t.sol | 2 +- .../lockup/linear/create-with-range/createWithRange.t.sol | 2 +- 6 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/libraries/Errors.sol b/src/libraries/Errors.sol index 091cdd054..09be69229 100644 --- a/src/libraries/Errors.sol +++ b/src/libraries/Errors.sol @@ -50,12 +50,12 @@ library Errors { /// @notice Thrown when the broker fee is greater than the maximum fee permitted. error SablierV2Lockup_BrokerFeeTooHigh(UD60x18 brokerFee, UD60x18 maxFee); - /// @notice Thrown when attempting to create a stream with the end time in the past. - error SablierV2Lockup_EndTimeInThePast(uint40 currentTime, uint40 endTime); - /// @notice Thrown when attempting to create a stream with a zero deposit amount. error SablierV2Lockup_DepositAmountZero(); + /// @notice Thrown when attempting to create a stream with the end time in the past. + error SablierV2Lockup_EndTimeInThePast(uint40 currentTime, uint40 endTime); + /// @notice Thrown when the protocol fee is greater than the maximum fee permitted. error SablierV2Lockup_ProtocolFeeTooHigh(UD60x18 protocolFee, UD60x18 maxFee); diff --git a/src/libraries/Helpers.sol b/src/libraries/Helpers.sol index 459528b70..3686342cd 100644 --- a/src/libraries/Helpers.sol +++ b/src/libraries/Helpers.sol @@ -98,12 +98,12 @@ library Helpers { revert Errors.SablierV2LockupLinear_StartTimeGreaterThanCliffTime(range.start, range.cliff); } - // Checks: the end time is not in the past. + // Checks: the cliff time is strictly less than the end time. if (range.cliff >= range.end) { revert Errors.SablierV2LockupLinear_CliffTimeNotLessThanEndTime(range.cliff, range.end); } - // Checks: the current time is strictly less than the end time. + // Checks: the end time is not in the past. uint40 currentTime = uint40(block.timestamp); if (currentTime >= range.end) { revert Errors.SablierV2Lockup_EndTimeInThePast(currentTime, range.end); @@ -198,7 +198,7 @@ library Helpers { } // Checks: the end time is not in the past. - // When the loop exits, current milestone is actually the last milestone, i.e. the end time of the stream. + // When the loop exits, the current milestone is the last milestone, i.e. the end time of the stream. uint40 currentTime = uint40(block.timestamp); if (currentTime >= currentMilestone) { revert Errors.SablierV2Lockup_EndTimeInThePast(currentTime, currentMilestone); diff --git a/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol index 3eb34343a..22ea1c0a4 100644 --- a/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -80,6 +80,7 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { { timeWarp = boundUint40(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); + // Define the current time. uint40 currentTime = DEFAULT_START_TIME + timeWarp; // Bound the withdraw amount. diff --git a/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol index 76de2b98f..1b095cf87 100644 --- a/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -92,6 +92,7 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { timeWarp = boundUint40(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION * 2); depositAmount = boundUint128(depositAmount, 10_000, UINT128_MAX); + // Define the current time. uint40 currentTime = DEFAULT_START_TIME + timeWarp; // Bound the withdraw amount. diff --git a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol index eb391a287..714284980 100644 --- a/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol +++ b/test/unit/lockup/dynamic/create-with-milestones/createWithMilestones.t.sol @@ -218,7 +218,7 @@ contract CreateWithMilestones_Dynamic_Unit_Test is Dynamic_Unit_Test { whenStartTimeLessThanFirstSegmentMilestone whenSegmentMilestonesOrdered { - vm.warp(DEFAULT_END_TIME); + vm.warp({ timestamp: DEFAULT_END_TIME }); vm.expectRevert( abi.encodeWithSelector(Errors.SablierV2Lockup_EndTimeInThePast.selector, DEFAULT_END_TIME, DEFAULT_END_TIME) diff --git a/test/unit/lockup/linear/create-with-range/createWithRange.t.sol b/test/unit/lockup/linear/create-with-range/createWithRange.t.sol index 96f20c238..7eebb812b 100644 --- a/test/unit/lockup/linear/create-with-range/createWithRange.t.sol +++ b/test/unit/lockup/linear/create-with-range/createWithRange.t.sol @@ -107,7 +107,7 @@ contract CreateWithRange_Linear_Unit_Test is Linear_Unit_Test { whenCliffTimeLessThanEndTime whenEndTimeNotInThePast { - vm.warp(DEFAULT_END_TIME); + vm.warp({ timestamp: DEFAULT_END_TIME }); vm.expectRevert( abi.encodeWithSelector(Errors.SablierV2Lockup_EndTimeInThePast.selector, DEFAULT_END_TIME, DEFAULT_END_TIME) From b6a438faa265b572946f236d2eb3f88880305957 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Fri, 31 Mar 2023 15:54:18 +0300 Subject: [PATCH 5/9] test: warp to present time in base test test: add "MAX_SEGMENT_DURATION" test: delete stale comments about fuzzing scenarios test: improve "fuzzSegmentMilestones" test: fix "min" for cliff duration bounds test: simplify bounding for end time --- test/Base.t.sol | 3 +++ .../streamed-amount-of/streamedAmountOf.sol | 14 +---------- .../withdrawableAmountOf.t.sol | 11 +-------- .../streamed-amount-of/streamedAmountOf.sol | 7 ------ .../withdrawableAmountOf.t.sol | 12 +--------- .../getWithdrawnAmount.t.sol | 2 +- .../handlers/LockupLinearCreateHandler.t.sol | 13 ++++------ test/shared/Constants.t.sol | 7 +++--- test/shared/Fuzzers.t.sol | 24 +++++++------------ 9 files changed, 24 insertions(+), 69 deletions(-) diff --git a/test/Base.t.sol b/test/Base.t.sol index 694a3aa26..3b6371c9e 100644 --- a/test/Base.t.sol +++ b/test/Base.t.sol @@ -101,6 +101,9 @@ abstract contract Base_Test is Assertions, Calculations, Events, Fuzzers, StdChe recipient: createUser("Recipient"), sender: createUser("Sender") }); + + // Warp to March 1, 2023 at 00:00 GMT to provide a more realistic testing environment. + vm.warp({ timestamp: DEFAULT_START_TIME }); } /*////////////////////////////////////////////////////////////////////////// diff --git a/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol b/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol index fdaf0d940..6b644f365 100644 --- a/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol +++ b/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol @@ -19,12 +19,6 @@ contract StreamedAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { } /// @dev it should return the correct streamed amount. - /// - /// The fuzzing ensures that all of the following scenarios are tested: - /// - /// - Current time < end time - /// - Current time = end time - /// - Current time > end time function testFuzz_StreamedAmountOf_OneSegment(uint40 timeWarp) external whenStreamActive @@ -63,12 +57,6 @@ contract StreamedAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { } /// @dev it should return the correct streamed amount. - /// - /// The fuzzing ensures that all of the following scenarios are tested: - /// - /// - Current time < end time - /// - Current time = end time - /// - Current time > end time function testFuzz_StreamedAmountOf_CurrentMilestoneNot1st(uint40 timeWarp) external whenStreamActive @@ -76,7 +64,7 @@ contract StreamedAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { whenMultipleSegments whenCurrentMilestoneNot1st { - timeWarp = boundUint40(timeWarp, MAX_SEGMENTS[0].milestone, DEFAULT_TOTAL_DURATION * 2); + timeWarp = boundUint40(timeWarp, MAX_SEGMENT_DURATION, DEFAULT_TOTAL_DURATION * 2); // Create the stream with the multiple-segment array. uint256 streamId = createDefaultStreamWithSegments(MAX_SEGMENTS); diff --git a/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol index 22ea1c0a4..898fcb0d4 100644 --- a/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -26,12 +26,6 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { } /// @dev it should return the correct withdrawable amount. - /// - /// The fuzzing ensures that all of the following scenarios are tested: - /// - /// - Current time < end time - /// - Current time = end time - /// - Current time > end time function testFuzz_WithdrawableAmountOf_WithoutWithdrawals(uint40 timeWarp) external whenStreamActive @@ -65,10 +59,7 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { /// /// The fuzzing ensures that all of the following scenarios are tested: /// - /// - Current time < end time - /// - Current time = end time - /// - Current time > end time - /// - WithdrawFromLockupStream amount equal to deposit amount and not + /// - Withdraw amount equal to deposit amount and not function testFuzz_WithdrawableAmountOf( uint40 timeWarp, uint128 withdrawAmount diff --git a/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol b/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol index e0d3a11bb..cf3498e82 100644 --- a/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol +++ b/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol @@ -34,13 +34,6 @@ contract StreamedAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { } /// @dev it should return the correct streamed amount. - /// - /// The fuzzing ensures that all of the following scenarios are tested: - /// - /// - Current time < end time - /// - Current time = end time - /// - Current time > end time - /// - Multiple values for the deposit amount function testFuzz_StreamedAmountOf( uint40 timeWarp, uint128 depositAmount diff --git a/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol index 1b095cf87..fed296772 100644 --- a/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -34,13 +34,6 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { } /// @dev it should return the correct withdrawable amount. - /// - /// The fuzzing ensures that all of the following scenarios are tested: - /// - /// - Current time < end time - /// - Current time = end time - /// - Current time > end time - /// - Multiple values for the deposit amount function testFuzz_WithdrawableAmountOf_NoWithdrawals( uint40 timeWarp, uint128 depositAmount @@ -75,11 +68,8 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { /// /// The fuzzing ensures that all of the following scenarios are tested: /// - /// - Current time < end time - /// - Current time = end time - /// - Current time > end time /// - Multiple values for the deposit amount - /// - WithdrawFromLockupStream amount equal to deposit amount and not + /// - Withdraw amount equal to deposit amount and not function testFuzz_WithdrawableAmountOf_WithWithdrawals( uint40 timeWarp, uint128 depositAmount, diff --git a/test/fuzz/lockup/shared/get-withdrawn-amount/getWithdrawnAmount.t.sol b/test/fuzz/lockup/shared/get-withdrawn-amount/getWithdrawnAmount.t.sol index 5f2ab94b5..bdf407238 100644 --- a/test/fuzz/lockup/shared/get-withdrawn-amount/getWithdrawnAmount.t.sol +++ b/test/fuzz/lockup/shared/get-withdrawn-amount/getWithdrawnAmount.t.sol @@ -39,7 +39,7 @@ abstract contract GetWithdrawnAmount_Fuzz_Test is Fuzz_Test, Lockup_Shared_Test external whenStreamNonNull { - timeWarp = bound(timeWarp, DEFAULT_CLIFF_TIME, DEFAULT_TOTAL_DURATION - 1); + timeWarp = bound(timeWarp, DEFAULT_CLIFF_DURATION, DEFAULT_TOTAL_DURATION - 1); // Warp into the future. vm.warp({ timestamp: DEFAULT_START_TIME + timeWarp }); diff --git a/test/invariant/handlers/LockupLinearCreateHandler.t.sol b/test/invariant/handlers/LockupLinearCreateHandler.t.sol index d7578bdf0..36fad6b8a 100644 --- a/test/invariant/handlers/LockupLinearCreateHandler.t.sol +++ b/test/invariant/handlers/LockupLinearCreateHandler.t.sol @@ -94,18 +94,13 @@ contract LockupLinearCreateHandler is BaseHandler { return; } + uint40 currentTime = getBlockTimestamp(); params.broker.fee = bound(params.broker.fee, 0, MAX_FEE); - params.range.start = boundUint40(params.range.start, 0, DEFAULT_START_TIME); - params.range.cliff = boundUint40(params.range.cliff, params.range.start, 52 weeks); + params.range.start = boundUint40(params.range.start, 0, currentTime); + params.range.cliff = boundUint40(params.range.cliff, currentTime, currentTime + 52 weeks); + params.range.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP); params.totalAmount = boundUint128(params.totalAmount, 1, 1_000_000_000e18); - // If the cliff time is in the past we want to make sure that the end time is not. - if (params.range.cliff >= getBlockTimestamp()) { - params.range.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP); - } else { - params.range.end = boundUint40(params.range.end, getBlockTimestamp() + 1, MAX_UNIX_TIMESTAMP); - } - // Mint enough ERC-20 assets to the sender. deal({ token: address(asset), to: params.sender, give: asset.balanceOf(params.sender) + params.totalAmount }); diff --git a/test/shared/Constants.t.sol b/test/shared/Constants.t.sol index 0029b37bf..6c11309f7 100644 --- a/test/shared/Constants.t.sol +++ b/test/shared/Constants.t.sol @@ -28,6 +28,7 @@ abstract contract Constants { uint128 internal constant DEFAULT_WITHDRAW_AMOUNT = 2600e18; bytes32 internal constant FLASH_LOAN_CALLBACK_SUCCESS = keccak256("ERC3156FlashBorrower.onFlashLoan"); UD60x18 internal constant MAX_FEE = UD60x18.wrap(0.1e18); // 10% + uint40 internal immutable MAX_SEGMENT_DURATION; uint40 internal constant MAX_UNIX_TIMESTAMP = 2_147_483_647; // 2^31 - 1 uint128 internal constant UINT128_MAX = type(uint128).max; uint256 internal constant UINT256_MAX = type(uint256).max; @@ -56,7 +57,7 @@ abstract contract Constants { //////////////////////////////////////////////////////////////////////////*/ constructor() { - DEFAULT_START_TIME = uint40(block.timestamp); + DEFAULT_START_TIME = uint40(1_677_632_400); // March 1, 2023 at 00:00 GMT DEFAULT_CLIFF_TIME = DEFAULT_START_TIME + DEFAULT_CLIFF_DURATION; DEFAULT_END_TIME = DEFAULT_START_TIME + DEFAULT_TOTAL_DURATION; DEFAULT_LINEAR_RANGE = @@ -96,7 +97,7 @@ abstract contract Constants { unchecked { uint128 amount = DEFAULT_DEPOSIT_AMOUNT / uint128(DEFAULT_MAX_SEGMENT_COUNT); UD2x18 exponent = ud2x18(2.71e18); - uint40 duration = DEFAULT_TOTAL_DURATION / uint40(DEFAULT_MAX_SEGMENT_COUNT); + MAX_SEGMENT_DURATION = DEFAULT_TOTAL_DURATION / uint40(DEFAULT_MAX_SEGMENT_COUNT); // Generate a bunch of segments with the same amount, same exponent, and with milestones // evenly spread apart. @@ -105,7 +106,7 @@ abstract contract Constants { LockupDynamic.Segment({ amount: amount, exponent: exponent, - milestone: DEFAULT_START_TIME + duration * (i + 1) + milestone: DEFAULT_START_TIME + MAX_SEGMENT_DURATION * (i + 1) }) ); } diff --git a/test/shared/Fuzzers.t.sol b/test/shared/Fuzzers.t.sol index 98136b8dc..125d21e28 100644 --- a/test/shared/Fuzzers.t.sol +++ b/test/shared/Fuzzers.t.sol @@ -131,23 +131,22 @@ abstract contract Fuzzers is Constants, Utils { /// @dev Fuzzes the segment milestones. function fuzzSegmentMilestones(LockupDynamic.Segment[] memory segments, uint40 startTime) internal view { - // Precompute the first milestone so that we don't bump into an underflow in the first loop iteration. - segments[0].milestone = startTime + 1; - // Return here if there's only one segment to not run into division by zero. uint40 segmentCount = uint40(segments.length); if (segmentCount == 1) { - // Make sure that the milestone is strictly greater than the current time. - if (segments[0].milestone <= getBlockTimestamp()) { - segments[0].milestone = getBlockTimestamp() + 1; - } + // The end time must not be in the past. + segments[0].milestone = getBlockTimestamp() + 2 days; return; } - // Generate `segmentCount` milestones linearly spaced between `startTime + 1` and `MAX_UNIX_TIMESTAMP`. - uint40 step = (MAX_UNIX_TIMESTAMP - (startTime + 1)) / (segmentCount - 1); + // We precompute the first milestone so that we don't bump into an underflow in the first loop iteration. We + // have to add 1 because the first milestone must be greater than the start time. + segments[0].milestone = startTime + 1; + + // Generate `segmentCount` milestones linearly spaced between the first milestone and `MAX_UNIX_TIMESTAMP`. + uint40 step = (MAX_UNIX_TIMESTAMP - segments[0].milestone) / (segmentCount - 1); uint40 halfStep = step / 2; - uint256[] memory milestones = arange(startTime + 1, MAX_UNIX_TIMESTAMP, step); + uint256[] memory milestones = arange(segments[0].milestone, MAX_UNIX_TIMESTAMP, step); // Fuzz the milestones in a way that preserves their order in the array. for (uint256 i = 1; i < segmentCount; ++i) { @@ -155,10 +154,5 @@ abstract contract Fuzzers is Constants, Utils { milestone = bound(milestone, milestone - halfStep, milestone + halfStep); segments[i].milestone = uint40(milestone); } - - // Make sure that the last milestone is strictly greater than the current time. - if (segments[segmentCount - 1].milestone <= getBlockTimestamp()) { - segments[segmentCount - 1].milestone = getBlockTimestamp() + 1; - } } } From 9791ac5391badeed35566c48fe9100d7130fcd4a Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Fri, 31 Mar 2023 23:41:49 +0300 Subject: [PATCH 6/9] test: bring back fuzzing scenario comments --- .../dynamic/streamed-amount-of/streamedAmountOf.sol | 12 ++++++++++++ .../withdrawableAmountOf.t.sol | 9 +++++++++ .../linear/streamed-amount-of/streamedAmountOf.sol | 6 ++++++ .../withdrawableAmountOf.t.sol | 9 +++++++++ 4 files changed, 36 insertions(+) diff --git a/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol b/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol index 6b644f365..c9a07fcb4 100644 --- a/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol +++ b/test/fuzz/lockup/dynamic/streamed-amount-of/streamedAmountOf.sol @@ -19,6 +19,12 @@ contract StreamedAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { } /// @dev it should return the correct streamed amount. + /// + /// The fuzzing ensures that all of the following scenarios are tested: + /// + /// - Current time < end time + /// - Current time = end time + /// - Current time > end time function testFuzz_StreamedAmountOf_OneSegment(uint40 timeWarp) external whenStreamActive @@ -57,6 +63,12 @@ contract StreamedAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { } /// @dev it should return the correct streamed amount. + /// + /// The fuzzing ensures that all of the following scenarios are tested: + /// + /// - Current time < end time + /// - Current time = end time + /// - Current time > end time function testFuzz_StreamedAmountOf_CurrentMilestoneNot1st(uint40 timeWarp) external whenStreamActive diff --git a/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol index 898fcb0d4..a03d25521 100644 --- a/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/fuzz/lockup/dynamic/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -26,6 +26,12 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { } /// @dev it should return the correct withdrawable amount. + /// + /// The fuzzing ensures that all of the following scenarios are tested: + /// + /// - Current time < end time + /// - Current time = end time + /// - Current time > end time function testFuzz_WithdrawableAmountOf_WithoutWithdrawals(uint40 timeWarp) external whenStreamActive @@ -59,6 +65,9 @@ contract WithdrawableAmountOf_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test { /// /// The fuzzing ensures that all of the following scenarios are tested: /// + /// - Current time < end time + /// - Current time = end time + /// - Current time > end time /// - Withdraw amount equal to deposit amount and not function testFuzz_WithdrawableAmountOf( uint40 timeWarp, diff --git a/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol b/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol index cf3498e82..21cdfc11d 100644 --- a/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol +++ b/test/fuzz/lockup/linear/streamed-amount-of/streamedAmountOf.sol @@ -34,6 +34,12 @@ contract StreamedAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { } /// @dev it should return the correct streamed amount. + /// + /// The fuzzing ensures that all of the following scenarios are tested: + /// + /// - Current time < end time + /// - Current time = end time + /// - Current time > end time function testFuzz_StreamedAmountOf( uint40 timeWarp, uint128 depositAmount diff --git a/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol index fed296772..6f204cfd2 100644 --- a/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/fuzz/lockup/linear/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -34,6 +34,12 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { } /// @dev it should return the correct withdrawable amount. + /// + /// The fuzzing ensures that all of the following scenarios are tested: + /// + /// - Current time < end time + /// - Current time = end time + /// - Current time > end time function testFuzz_WithdrawableAmountOf_NoWithdrawals( uint40 timeWarp, uint128 depositAmount @@ -68,6 +74,9 @@ contract WithdrawableAmountOf_Linear_Fuzz_Test is Linear_Fuzz_Test { /// /// The fuzzing ensures that all of the following scenarios are tested: /// + /// - Current time < end time + /// - Current time = end time + /// - Current time > end time /// - Multiple values for the deposit amount /// - Withdraw amount equal to deposit amount and not function testFuzz_WithdrawableAmountOf_WithWithdrawals( From 1a1749f7408e43fe6808d92e23e8cadae420e73c Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Fri, 31 Mar 2023 23:48:46 +0300 Subject: [PATCH 7/9] test: allow past cliff times in invariants --- test/invariant/handlers/LockupLinearCreateHandler.t.sol | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/invariant/handlers/LockupLinearCreateHandler.t.sol b/test/invariant/handlers/LockupLinearCreateHandler.t.sol index 36fad6b8a..7a0057f14 100644 --- a/test/invariant/handlers/LockupLinearCreateHandler.t.sol +++ b/test/invariant/handlers/LockupLinearCreateHandler.t.sol @@ -97,8 +97,13 @@ contract LockupLinearCreateHandler is BaseHandler { uint40 currentTime = getBlockTimestamp(); params.broker.fee = bound(params.broker.fee, 0, MAX_FEE); params.range.start = boundUint40(params.range.start, 0, currentTime); - params.range.cliff = boundUint40(params.range.cliff, currentTime, currentTime + 52 weeks); - params.range.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP); + params.range.cliff = boundUint40(params.range.cliff, params.range.start, params.range.start + 52 weeks); + // Fuzz the end time so that it is always after the cliff time, and always greater than the current time. + params.range.end = boundUint40( + params.range.end, + (params.range.cliff <= currentTime ? currentTime : params.range.cliff) + 1, + MAX_UNIX_TIMESTAMP + ); params.totalAmount = boundUint128(params.totalAmount, 1, 1_000_000_000e18); // Mint enough ERC-20 assets to the sender. From 2311ea59b5affd18afc15d8b736c9ebf7c595824 Mon Sep 17 00:00:00 2001 From: andreivladbrg Date: Sat, 1 Apr 2023 19:06:03 +0300 Subject: [PATCH 8/9] test: use ternary operator instead of if --- test/fork/lockup/linear/Linear.t.sol | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/test/fork/lockup/linear/Linear.t.sol b/test/fork/lockup/linear/Linear.t.sol index 3a6ba697e..0c9cb43ba 100644 --- a/test/fork/lockup/linear/Linear.t.sol +++ b/test/fork/lockup/linear/Linear.t.sol @@ -107,22 +107,21 @@ abstract contract Linear_Fork_Test is Fork_Test { /// - Multiple values for the withdraw amount, including zero. function testForkFuzz_Linear_CreateWithdrawCancel(Params memory params) external { checkUsers(params.sender, params.recipient, params.broker.account, address(linear)); + + // Bound the parameters. + uint40 currentTime = getBlockTimestamp(); params.broker.fee = bound(params.broker.fee, 0, MAX_FEE); - params.range.start = boundUint40( - params.range.start, uint40(block.timestamp - 1000 seconds), uint40(block.timestamp + 10_000 seconds) - ); + 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.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP); + // Fuzz the end time so that it is always after the cliff time, and always greater than the current time. + params.range.end = boundUint40( + params.range.end, + (params.range.cliff <= currentTime ? currentTime : params.range.cliff) + 1, + MAX_UNIX_TIMESTAMP + ); params.protocolFee = bound(params.protocolFee, 0, MAX_FEE); params.totalAmount = boundUint128(params.totalAmount, 1, uint128(initialHolderBalance)); - // If the cliff time is in the past we want to make sure that the end time is not. - if (params.range.cliff >= getBlockTimestamp()) { - params.range.end = boundUint40(params.range.end, params.range.cliff + 1, MAX_UNIX_TIMESTAMP); - } else { - params.range.end = boundUint40(params.range.end, getBlockTimestamp() + 1, MAX_UNIX_TIMESTAMP); - } - // Set the fuzzed protocol fee. changePrank({ msgSender: users.admin }); comptroller.setProtocolFee({ asset: asset, newProtocolFee: params.protocolFee }); From ef43dd10487a58dbf56d03f94ad9f0594ccadb69 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Mon, 3 Apr 2023 18:40:09 +0300 Subject: [PATCH 9/9] test: fix bound in "test_Withdraw_FuzzedSegments" --- test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol b/test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol index aced47515..29624e9e1 100644 --- a/test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol +++ b/test/fuzz/lockup/dynamic/withdraw/withdraw.t.sol @@ -29,6 +29,7 @@ contract Withdraw_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test, Withdraw_Fuzz_Test { address funder; uint256 streamId; uint128 totalAmount; + uint40 totalDuration; uint128 withdrawAmount; uint128 withdrawableAmount; } @@ -60,7 +61,8 @@ contract Withdraw_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test, Withdraw_Fuzz_Test { (vars.totalAmount, vars.createAmounts) = fuzzSegmentAmountsAndCalculateCreateAmounts(params.segments); // Bound the time warp. - params.timeWarp = bound(params.timeWarp, 1, params.segments[params.segments.length - 1].milestone); + vars.totalDuration = params.segments[params.segments.length - 1].milestone - DEFAULT_START_TIME; + params.timeWarp = bound(params.timeWarp, 1, vars.totalDuration); // Mint enough ERC-20 assets to the sender. deal({ token: address(DEFAULT_ASSET), to: vars.funder, give: vars.totalAmount }); @@ -82,8 +84,15 @@ contract Withdraw_Dynamic_Fuzz_Test is Dynamic_Fuzz_Test, Withdraw_Fuzz_Test { // Warp into the future. vm.warp({ timestamp: DEFAULT_START_TIME + params.timeWarp }); - // Bound the withdraw amount. + // Query the withdrawable amount. vars.withdrawableAmount = dynamic.withdrawableAmountOf(vars.streamId); + + // Halt the test if the withdraw amount is zero. + if (vars.withdrawableAmount == 0) { + return; + } + + // Bound the withdraw amount. vars.withdrawAmount = boundUint128(vars.withdrawAmount, 1, vars.withdrawableAmount); // Expect the ERC-20 assets to be transferred to the recipient.