test(refactor): core tests to use Bulloak (#1022)

* ci: bulloak check

* test(refactor): refactor lockup integration tests using bulloak

* test(refactor): refactor lockup dynamic tests using bulloak

* test(refactor): refactor lockup linear tests using bulloak

* test(refactor): refactor lockup tranched tests using bulloak

* test(refactor): refactor nft descriptor tests using bulloak

* test(refactor): fix bugs in core tests using bulloak

* ci: update tree-path as per latest bulloak changelog

* test(refactor): polish core tests using BTT

* test(fix): bulloak check

* test(refactor): streamedAmountOf

* ci(refactor): remove newline

* test: remove "is" from test branches

* refactor: re-order checks in withdraw function

* test(tree): delete are keywords from branches

---------

Co-authored-by: andreivladbrg <andreivladbrg@gmail.com>

.github/workflows/ci.yml

@@ -17,6 +17,13 @@ jobs:
   lint:
     uses: "sablier-labs/reusable-workflows/.github/workflows/forge-lint.yml@main"
 
+  bulloak:
+    needs: ["lint"]
+    uses: "sablier-labs/reusable-workflows/.github/workflows/bulloak-check.yml@main"
+    with:
+      skip-modifiers: true
+      tree-path: "test/core"
+
   build:
     uses: "sablier-labs/reusable-workflows/.github/workflows/forge-build.yml@main"
 
@@ -63,4 +70,4 @@ jobs:
       CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
     uses: "sablier-labs/reusable-workflows/.github/workflows/forge-coverage.yml@main"
     with:
-      match-path: "test/{core,periphery}/{integration,unit}/**/*.sol"
+      match-path: "test/{core,periphery}/{integration,unit}/**/*.sol"

src/core/abstracts/SablierLockup.sol

@@ -374,11 +374,6 @@ abstract contract SablierLockup is
             revert Errors.SablierLockup_WithdrawToZeroAddress(streamId);
         }
 
-        // Check: the withdraw amount is not zero.
-        if (amount == 0) {
-            revert Errors.SablierLockup_WithdrawAmountZero(streamId);
-        }
-
         // Retrieve the recipient from storage.
         address recipient = _ownerOf(streamId);
 
@@ -388,6 +383,11 @@ abstract contract SablierLockup is
             revert Errors.SablierLockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
         }
 
+        // Check: the withdraw amount is not zero.
+        if (amount == 0) {
+            revert Errors.SablierLockup_WithdrawAmountZero(streamId);
+        }
+
         // Check: the withdraw amount is not greater than the withdrawable amount.
         uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
         if (amount > withdrawableAmount) {

test/core/fork/LockupDynamic.t.sol

@@ -374,7 +374,7 @@ abstract contract LockupDynamic_Fork_Test is Fork_Test {
             assertEq(vars.actualRecipientBalance, vars.expectedRecipientBalance, "post-cancel Recipient balance");
         }
 
-        // Assert that the NFT has not been burned.
+        // Assert that the not burned NFT.
         vars.actualNFTOwner = lockupDynamic.ownerOf({ tokenId: vars.streamId });
         vars.expectedNFTOwner = params.recipient;
         assertEq(vars.actualNFTOwner, vars.expectedNFTOwner, "post-cancel NFT owner");

test/core/fork/LockupLinear.t.sol

@@ -376,7 +376,7 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
             assertEq(vars.actualRecipientBalance, vars.expectedRecipientBalance, "post-cancel Recipient balance");
         }
 
-        // Assert that the NFT has not been burned.
+        // Assert that the not burned NFT.
         vars.actualNFTOwner = lockupLinear.ownerOf({ tokenId: vars.streamId });
         vars.expectedNFTOwner = params.recipient;
         assertEq(vars.actualNFTOwner, vars.expectedNFTOwner, "post-cancel NFT owner");

test/core/fork/LockupTranched.t.sol

@@ -374,7 +374,7 @@ abstract contract LockupTranched_Fork_Test is Fork_Test {
             assertEq(vars.actualRecipientBalance, vars.expectedRecipientBalance, "post-cancel Recipient balance");
         }
 
-        // Assert that the NFT has not been burned.
+        // Assert that the not burned NFT.
         vars.actualNFTOwner = lockupTranched.ownerOf({ tokenId: vars.streamId });
         vars.expectedNFTOwner = params.recipient;
         assertEq(vars.actualNFTOwner, vars.expectedNFTOwner, "post-cancel NFT owner");

test/core/integration/concrete/lockup-dynamic/create-with-durations/createWithDurations.t.sol

@@ -24,22 +24,24 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
         streamId = lockupDynamic.nextStreamId();
     }
 
-    /// @dev it should revert.
-    function test_RevertWhen_DelegateCalled() external {
+    function test_RevertWhen_DelegateCall() external {
         bytes memory callData =
             abi.encodeCall(ISablierLockupDynamic.createWithDurations, defaults.createWithDurationsLD());
         (bool success, bytes memory returnData) = address(lockupDynamic).delegatecall(callData);
         expectRevertDueToDelegateCall(success, returnData);
     }
 
-    /// @dev it should revert.
-    function test_RevertWhen_SegmentCountTooHigh() external whenNotDelegateCalled {
+    function test_RevertWhen_SegmentCountExceedsMaxValue() external whenNoDelegateCall {
         LockupDynamic.SegmentWithDuration[] memory segments = new LockupDynamic.SegmentWithDuration[](25_000);
         vm.expectRevert(abi.encodeWithSelector(Errors.SablierLockupDynamic_SegmentCountTooHigh.selector, 25_000));
         createDefaultStreamWithDurations(segments);
     }
 
-    function test_RevertWhen_DurationsZero() external whenNotDelegateCalled whenSegmentCountNotTooHigh {
+    function test_RevertWhen_FirstIndexHasZeroDuration()
+        external
+        whenNoDelegateCall
+        whenSegmentCountNotExceedMaxValue
+    {
         uint40 startTime = getBlockTimestamp();
         LockupDynamic.SegmentWithDuration[] memory segments = defaults.createWithDurationsLD().segments;
         segments[1].duration = 0;
@@ -55,11 +57,12 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
         createDefaultStreamWithDurations(segments);
     }
 
-    function test_RevertWhen_TimestampsCalculationsOverflows_StartTimeNotLessThanFirstSegmentTimestamp()
+    function test_RevertWhen_StartTimeExceedsFirstTimestamp()
         external
-        whenNotDelegateCalled
-        whenSegmentCountNotTooHigh
-        whenDurationsNotZero
+        whenNoDelegateCall
+        whenSegmentCountNotExceedMaxValue
+        whenFirstIndexHasNonZeroDuration
+        whenTimestampsCalculationOverflows
     {
         unchecked {
             uint40 startTime = getBlockTimestamp();
@@ -76,11 +79,13 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
         }
     }
 
-    function test_RevertWhen_TimestampsCalculationsOverflows_SegmentTimestampsNotOrdered()
+    function test_RevertWhen_TimestampsNotStrictlyIncreasing()
         external
-        whenNotDelegateCalled
-        whenSegmentCountNotTooHigh
-        whenDurationsNotZero
+        whenNoDelegateCall
+        whenSegmentCountNotExceedMaxValue
+        whenFirstIndexHasNonZeroDuration
+        whenTimestampsCalculationOverflows
+        whenStartTimeNotExceedsFirstTimestamp
     {
         unchecked {
             uint40 startTime = getBlockTimestamp();
@@ -111,12 +116,11 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
         }
     }
 
-    function test_CreateWithDurations()
+    function test_WhenTimestampsCalculationNotOverflow()
         external
-        whenNotDelegateCalled
-        whenSegmentCountNotTooHigh
-        whenDurationsNotZero
-        whenTimestampsCalculationsDoNotOverflow
+        whenNoDelegateCall
+        whenSegmentCountNotExceedMaxValue
+        whenFirstIndexHasNonZeroDuration
     {
         // Make the Sender the stream's funder
         address funder = users.sender;
@@ -132,13 +136,13 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
         segments[0].timestamp = timestamps.start + segmentsWithDurations[0].duration;
         segments[1].timestamp = segments[0].timestamp + segmentsWithDurations[1].duration;
 
-        // Expect the assets to be transferred from the funder to {SablierLockupDynamic}.
+        // It should perform the ERC-20 transfers.
         expectCallToTransferFrom({ from: funder, to: address(lockupDynamic), value: defaults.DEPOSIT_AMOUNT() });
 
         // Expect the broker fee to be paid to the broker.
         expectCallToTransferFrom({ from: funder, to: users.broker, value: defaults.BROKER_FEE_AMOUNT() });
 
-        // Expect the relevant events to be emitted.
+        // It should emit {CreateLockupDynamicStream} and {MetadataUpdate} events.
         vm.expectEmit({ emitter: address(lockupDynamic) });
         emit MetadataUpdate({ _tokenId: streamId });
         vm.expectEmit({ emitter: address(lockupDynamic) });
@@ -159,7 +163,7 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
         // Create the stream.
         createDefaultStreamWithDurations();
 
-        // Assert that the stream has been created.
+        // It should create the stream.
         LockupDynamic.StreamLD memory actualStream = lockupDynamic.getStream(streamId);
         LockupDynamic.StreamLD memory expectedStream = defaults.lockupDynamicStream();
         expectedStream.endTime = timestamps.end;
@@ -172,12 +176,12 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
         Lockup.Status expectedStatus = Lockup.Status.STREAMING;
         assertEq(actualStatus, expectedStatus);
 
-        // Assert that the next stream ID has been bumped.
+        // It should bump the next stream ID.
         uint256 actualNextStreamId = lockupDynamic.nextStreamId();
         uint256 expectedNextStreamId = streamId + 1;
         assertEq(actualNextStreamId, expectedNextStreamId, "nextStreamId");
 
-        // Assert that the NFT has been minted.
+        // It should mint the NFT.
         address actualNFTOwner = lockupDynamic.ownerOf({ tokenId: streamId });
         address expectedNFTOwner = users.recipient;
         assertEq(actualNFTOwner, expectedNFTOwner, "NFT owner");

test/core/integration/concrete/lockup-dynamic/create-with-durations/createWithDurations.tree

@@ -1,22 +1,22 @@
-createWithDurations.t.sol
-├── when delegate called
+CreateWithDurations_LockupDynamic_Integration_Concrete_Test
+├── when delegate call
 │  └── it should revert
-└── when not delegate called
-   ├── when the segment count is too high
+└── when no delegate call
+   ├── when segment count exceeds max value
    │  └── it should revert
-   └── when the segment count is not too high
-       ├── when at least one of the durations at index one or greater is zero
+   └── when segment count not exceed max value
+       ├── when first index has zero duration
        │  └── it should revert
-       └── when none of the durations is zero
-          ├── when the segment timestamp calculations overflow uint256
-          │  ├── when the start time is not less than the first segment timestamp
+       └── when first index has non zero duration
+          ├── when timestamps calculation overflows
+          │  ├── when start time exceeds first timestamp
           │  │  └── it should revert
-          │  └── when the segment timestamps are not ordered
-          │     └── it should revert
-          └── when the segment timestamp calculations do not overflow uint256
+          │  └── when start time not exceeds first timestamp
+          │     └── when timestamps not strictly increasing
+          │        └── it should revert
+          └── when timestamps calculation not overflow
              ├── it should create the stream
              ├── it should bump the next stream ID
              ├── it should mint the NFT
-             ├── it should emit a {MetadataUpdate} event
-             ├── it should perform the ERC-20 transfers
-             └── it should emit a {CreateLockupDynamicStream} event
+             ├── it should emit {CreateLockupDynamicStream} and {MetadataUpdate} events
+             └── it should perform the ERC-20 transfers