From 0cc534668ea6448d33008a89e7f3b873a7b4c1f0 Mon Sep 17 00:00:00 2001 From: smol-ninja Date: Mon, 23 Dec 2024 13:37:28 +0530 Subject: [PATCH 1/7] feat: bubble up revert in batch function feat: return results in batch function test: comprehensive testing for batch with Lockup test: move allowToHook call to Integration --- src/abstracts/Batch.sol | 21 +- src/interfaces/IBatch.sol | 3 +- tests/integration/Integration.t.sol | 54 ++-- tests/integration/concrete/batch/batch.t.sol | 233 ++++++++++++++++++ .../concrete/lockup-base/batch/batch.t.sol | 41 --- .../concrete/lockup-base/batch/batch.tree | 10 - .../concrete/lockup-base/cancel/cancel.t.sol | 5 - .../lockup-base/withdraw/withdraw.t.sol | 5 - 8 files changed, 286 insertions(+), 86 deletions(-) create mode 100644 tests/integration/concrete/batch/batch.t.sol delete mode 100644 tests/integration/concrete/lockup-base/batch/batch.t.sol delete mode 100644 tests/integration/concrete/lockup-base/batch/batch.tree diff --git a/src/abstracts/Batch.sol b/src/abstracts/Batch.sol index 8b8e879a6..6e277571b 100644 --- a/src/abstracts/Batch.sol +++ b/src/abstracts/Batch.sol @@ -1,8 +1,8 @@ // SPDX-License-Identifier: GPL-3.0-or-later +// solhint-disable no-inline-assembly pragma solidity >=0.8.22; import { IBatch } from "../interfaces/IBatch.sol"; -import { Errors } from "../libraries/Errors.sol"; /// @title Batch /// @notice See the documentation in {IBatch}. @@ -13,15 +13,28 @@ abstract contract Batch is IBatch { //////////////////////////////////////////////////////////////////////////*/ /// @inheritdoc IBatch - /// @dev The `msg.value` should not be used on any method called in the batch. - function batch(bytes[] calldata calls) external payable override { + /// @dev Since `msg.value` can be reused across the calls, BE VERY CAREFUL when using it. Refer to + /// https://www.paradigm.xyz/2021/08/two-rights-might-make-a-wrong for more information. + function batch(bytes[] calldata calls) external payable override returns (bytes[] memory results) { uint256 count = calls.length; + results = new bytes[](count); for (uint256 i = 0; i < count; ++i) { (bool success, bytes memory result) = address(this).delegatecall(calls[i]); + + // Revert with result data if delegatecall fails. Assembly code is used to bubble up the revert reason. if (!success) { - revert Errors.BatchError(result); + assembly { + // Get the length of the result stored in the first 32 bytes. + let resultSize := mload(result) + + // Forward the pointer by 32 bytes at the beginning of the result data. + revert(add(32, result), resultSize) + } } + + // Store the result of the delegatecall. + results[i] = result; } } } diff --git a/src/interfaces/IBatch.sol b/src/interfaces/IBatch.sol index 862762efb..d6f21dbcf 100644 --- a/src/interfaces/IBatch.sol +++ b/src/interfaces/IBatch.sol @@ -5,5 +5,6 @@ pragma solidity >=0.8.22; interface IBatch { /// @notice Allows batched call to self, `this` contract. /// @param calls An array of inputs for each call. - function batch(bytes[] calldata calls) external payable; + /// @return results An array of results from each call. Store empty bytes for calls that do not return anything. + function batch(bytes[] calldata calls) external payable returns (bytes[] memory results); } diff --git a/tests/integration/Integration.t.sol b/tests/integration/Integration.t.sol index 640fe1729..cd823a9e5 100644 --- a/tests/integration/Integration.t.sol +++ b/tests/integration/Integration.t.sol @@ -70,16 +70,8 @@ abstract contract Integration_Test is Base_Test { function setUp() public virtual override { Base_Test.setUp(); - recipientInterfaceIDIncorrect = new RecipientInterfaceIDIncorrect(); - recipientInterfaceIDMissing = new RecipientInterfaceIDMissing(); - recipientInvalidSelector = new RecipientInvalidSelector(); - recipientReentrant = new RecipientReentrant(); - recipientReverting = new RecipientReverting(); - vm.label({ account: address(recipientInterfaceIDIncorrect), newLabel: "Recipient Interface ID Incorrect" }); - vm.label({ account: address(recipientInterfaceIDMissing), newLabel: "Recipient Interface ID Missing" }); - vm.label({ account: address(recipientInvalidSelector), newLabel: "Recipient Invalid Selector" }); - vm.label({ account: address(recipientReentrant), newLabel: "Recipient Reentrant" }); - vm.label({ account: address(recipientReverting), newLabel: "Recipient Reverting" }); + // Initialize the recipients with Hook implementations. + initializeRecipientsWithHooks(); _defaultParams.createWithTimestamps = defaults.createWithTimestamps(); _defaultParams.createWithDurations = defaults.createWithDurations(); @@ -108,6 +100,38 @@ abstract contract Integration_Test is Base_Test { initializeDefaultStreamIds(); } + /*////////////////////////////////////////////////////////////////////////// + INITIALIZE-FUNCTIONS + //////////////////////////////////////////////////////////////////////////*/ + + function initializeDefaultStreamIds() internal { + defaultStreamId = createDefaultStream(); + notCancelableStreamId = createDefaultStreamNonCancelable(); + notTransferableStreamId = createDefaultStreamNonTransferable(); + recipientGoodStreamId = createDefaultStreamWithRecipient(address(recipientGood)); + recipientInvalidSelectorStreamId = createDefaultStreamWithRecipient(address(recipientInvalidSelector)); + recipientReentrantStreamId = createDefaultStreamWithRecipient(address(recipientReentrant)); + recipientRevertStreamId = createDefaultStreamWithRecipient(address(recipientReverting)); + } + + function initializeRecipientsWithHooks() internal { + recipientInterfaceIDIncorrect = new RecipientInterfaceIDIncorrect(); + recipientInterfaceIDMissing = new RecipientInterfaceIDMissing(); + recipientInvalidSelector = new RecipientInvalidSelector(); + recipientReentrant = new RecipientReentrant(); + recipientReverting = new RecipientReverting(); + vm.label({ account: address(recipientInterfaceIDIncorrect), newLabel: "Recipient Interface ID Incorrect" }); + vm.label({ account: address(recipientInterfaceIDMissing), newLabel: "Recipient Interface ID Missing" }); + vm.label({ account: address(recipientInvalidSelector), newLabel: "Recipient Invalid Selector" }); + vm.label({ account: address(recipientReentrant), newLabel: "Recipient Reentrant" }); + vm.label({ account: address(recipientReverting), newLabel: "Recipient Reverting" }); + + // Allow the recipients to Hook. + resetPrank({ msgSender: users.admin }); + lockup.allowToHook(address(recipientReverting)); + resetPrank({ msgSender: users.sender }); + } + /*////////////////////////////////////////////////////////////////////////// CREATE-DEFAULT //////////////////////////////////////////////////////////////////////////*/ @@ -179,16 +203,6 @@ abstract contract Integration_Test is Base_Test { streamId = createDefaultStream(params); } - function initializeDefaultStreamIds() internal { - defaultStreamId = createDefaultStream(); - notCancelableStreamId = createDefaultStreamNonCancelable(); - notTransferableStreamId = createDefaultStreamNonTransferable(); - recipientGoodStreamId = createDefaultStreamWithRecipient(address(recipientGood)); - recipientInvalidSelectorStreamId = createDefaultStreamWithRecipient(address(recipientInvalidSelector)); - recipientReentrantStreamId = createDefaultStreamWithRecipient(address(recipientReentrant)); - recipientRevertStreamId = createDefaultStreamWithRecipient(address(recipientReverting)); - } - /*////////////////////////////////////////////////////////////////////////// COMMON-REVERT-TESTS //////////////////////////////////////////////////////////////////////////*/ diff --git a/tests/integration/concrete/batch/batch.t.sol b/tests/integration/concrete/batch/batch.t.sol new file mode 100644 index 000000000..4dd9a045e --- /dev/null +++ b/tests/integration/concrete/batch/batch.t.sol @@ -0,0 +1,233 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.22; + +import { Errors } from "src/libraries/Errors.sol"; + +import { Integration_Test } from "../../Integration.t.sol"; + +contract Batch_Integration_Concrete_Test is Integration_Test { + /*////////////////////////////////////////////////////////////////////////// + REVERT + //////////////////////////////////////////////////////////////////////////*/ + + /// @dev The batch call includes ETH and a non-payable function. + function test_RevertWhen_ETHWithNonPayable() external { + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeCall(lockup.isCancelable, (defaultStreamId)); + + vm.expectRevert(); + lockup.batch{ value: 1 wei }(calls); + } + + /// @dev The batch call cancels a non-cancelable stream. + function test_RevertWhen_LockupThrows() external { + bytes[] memory calls = new bytes[](2); + calls[0] = abi.encodeCall(lockup.cancel, (defaultStreamId)); + calls[1] = abi.encodeCall(lockup.cancel, (notCancelableStreamId)); + + // Expect revert on notCancelableStreamId. + vm.expectRevert( + abi.encodeWithSelector(Errors.SablierLockupBase_StreamNotCancelable.selector, notCancelableStreamId) + ); + lockup.batch(calls); + } + + /// @dev The batch call includes a non-existent function. + function test_RevertWhen_NonExistentFunction() external { + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSignature("nonExistentFunction()"); + + vm.expectRevert(); + lockup.batch(calls); + } + + /// @dev The batch call reverts with a string reason. + function test_RevertWhen_StringReason() external { + bytes[] memory calls = new bytes[](2); + calls[0] = abi.encodeCall(lockup.cancel, (defaultStreamId)); + calls[1] = abi.encodeCall(lockup.cancel, (recipientRevertStreamId)); + + vm.expectRevert("You shall not pass"); + lockup.batch(calls); + } + + /*////////////////////////////////////////////////////////////////////////// + RETURN + //////////////////////////////////////////////////////////////////////////*/ + + /// @dev The batch call includes: + /// - Returning state changing functions + /// - Non-returning state changing functions + /// - View only functions + function test_Batch_StateChangingAndViewFunctions() external { + uint256 expectedNextStreamId = lockup.nextStreamId(); + vm.warp(defaults.WARP_26_PERCENT()); + + bytes[] memory calls = new bytes[](6); + // It will return True. + calls[0] = abi.encodeCall(lockup.isCancelable, (defaultStreamId)); + // It will return the withdrawn amount. + calls[1] = abi.encodeCall(lockup.withdrawMax, (notCancelableStreamId, users.recipient)); + // It will not return anything. + calls[2] = abi.encodeCall(lockup.cancel, (defaultStreamId)); + // It will return the next stream ID. + calls[3] = abi.encodeCall(lockup.nextStreamId, ()); + // It will return the stream ID created. + calls[4] = abi.encodeCall( + lockup.createWithTimestampsLL, + (defaults.createWithTimestamps(), defaults.unlockAmounts(), defaults.CLIFF_TIME()) + ); + // It will not return anything. + calls[5] = abi.encodeCall(lockup.renounce, (notTransferableStreamId)); + + bytes[] memory results = lockup.batch(calls); + assertEq(results.length, 6); + assertEq(abi.decode(results[0], (bool)), true); + assertEq(abi.decode(results[1], (uint128)), defaults.WITHDRAW_AMOUNT()); + assertEq(results[2], hex""); + assertEq(abi.decode(results[3], (uint256)), expectedNextStreamId); + assertEq(abi.decode(results[4], (uint256)), expectedNextStreamId); + assertEq(results[5], hex""); + } + + /// @dev The batch call includes: + /// - ETH value + /// - Returning state changing functions + /// - Non-returning state changing functions + function test_BatchPayable_StateChangingFunctions() external { + uint256 expectedNextStreamId = lockup.nextStreamId(); + uint256 initialEthBalance = address(lockup).balance; + vm.warp(defaults.WARP_26_PERCENT()); + + bytes[] memory calls = new bytes[](4); + // It will return the withdrawn amount. + calls[0] = abi.encodeCall(lockup.withdrawMax, (notCancelableStreamId, users.recipient)); + // It will not return anything. + calls[1] = abi.encodeCall(lockup.cancel, (defaultStreamId)); + // It will return the stream ID created. + calls[2] = abi.encodeCall( + lockup.createWithTimestampsLL, + (defaults.createWithTimestamps(), defaults.unlockAmounts(), defaults.CLIFF_TIME()) + ); + // It will not return anything. + calls[3] = abi.encodeCall(lockup.renounce, (notTransferableStreamId)); + + bytes[] memory results = lockup.batch{ value: 1 wei }(calls); + assertEq(results.length, 4); + assertEq(abi.decode(results[0], (uint128)), defaults.WITHDRAW_AMOUNT()); + assertEq(results[1], hex""); + assertEq(abi.decode(results[2], (uint256)), expectedNextStreamId); + assertEq(results[3], hex""); + assertEq(address(lockup).balance, initialEthBalance + 1 wei); + } + + /*////////////////////////////////////////////////////////////////////////// + BATCH + LOCKUP + //////////////////////////////////////////////////////////////////////////*/ + + /// @dev The batch call includes: + /// - ETH value + /// - All create stream functions that return a value + function test_BatchPayable_CreateStreams() external { + uint256 expectedNextStreamId = lockup.nextStreamId(); + uint256 initialEthBalance = address(lockup).balance; + + bytes[] memory calls = new bytes[](6); + calls[0] = abi.encodeCall( + lockup.createWithDurationsLD, (defaults.createWithDurations(), defaults.segmentsWithDurations()) + ); + calls[1] = abi.encodeCall( + lockup.createWithDurationsLL, + (defaults.createWithDurations(), defaults.unlockAmounts(), defaults.durations()) + ); + calls[2] = abi.encodeCall( + lockup.createWithDurationsLT, (defaults.createWithDurations(), defaults.tranchesWithDurations()) + ); + calls[3] = abi.encodeCall(lockup.createWithTimestampsLD, (defaults.createWithTimestamps(), defaults.segments())); + calls[4] = abi.encodeCall( + lockup.createWithTimestampsLL, + (defaults.createWithTimestamps(), defaults.unlockAmounts(), defaults.CLIFF_TIME()) + ); + calls[5] = abi.encodeCall(lockup.createWithTimestampsLT, (defaults.createWithTimestamps(), defaults.tranches())); + + // It should return the stream IDs created. + bytes[] memory results = lockup.batch{ value: 1 wei }(calls); + assertEq(results.length, 6); + assertEq(abi.decode(results[0], (uint256)), expectedNextStreamId); + assertEq(abi.decode(results[1], (uint256)), expectedNextStreamId + 1); + assertEq(abi.decode(results[2], (uint256)), expectedNextStreamId + 2); + assertEq(abi.decode(results[3], (uint256)), expectedNextStreamId + 3); + assertEq(abi.decode(results[4], (uint256)), expectedNextStreamId + 4); + assertEq(abi.decode(results[5], (uint256)), expectedNextStreamId + 5); + assertEq(address(lockup).balance, initialEthBalance + 1 wei); + } + + /// @dev The batch call includes: + /// - ETH value + /// - All recipient related functions with both returns and non-returns + function test_BatchPayable_RecipientFunctions() external { + uint256 initialEthBalance = address(lockup).balance; + vm.warp(defaults.WARP_26_PERCENT()); + + bytes[] memory calls = new bytes[](4); + calls[0] = abi.encodeCall(lockup.cancel, (defaultStreamId)); + + uint256[] memory streamIds = new uint256[](2); + streamIds[0] = recipientGoodStreamId; + streamIds[1] = recipientInvalidSelectorStreamId; + calls[1] = abi.encodeCall(lockup.cancelMultiple, (streamIds)); + + calls[2] = abi.encodeCall(lockup.renounce, (recipientReentrantStreamId)); + + streamIds = new uint256[](1); + streamIds[0] = recipientRevertStreamId; + calls[3] = abi.encodeCall(lockup.renounceMultiple, (streamIds)); + + bytes[] memory results = lockup.batch{ value: 1 wei }(calls); + + assertEq(results.length, 4); + assertEq(results[0], hex""); + assertEq(results[1], hex""); + assertEq(results[2], hex""); + assertEq(results[3], hex""); + assertEq(address(lockup).balance, initialEthBalance + 1 wei); + } + + /// @dev The batch call includes: + /// - ETH value + /// - All sender related functions with both returns and non-returns + function test_BatchPayable_SenderFunctions() external { + uint256 initialEthBalance = address(lockup).balance; + // Warp to the end time so that `burn` can be added to the call list. + vm.warp(defaults.END_TIME()); + + bytes[] memory calls = new bytes[](5); + // It should not return anything. + calls[0] = abi.encodeCall(lockup.withdraw, (defaultStreamId, users.recipient, 1)); + // It should return the withdrawn amount. + calls[1] = abi.encodeCall(lockup.withdrawMax, (defaultStreamId, users.recipient)); + + uint256[] memory streamIds = new uint256[](2); + streamIds[0] = streamIds[1] = notCancelableStreamId; + uint128[] memory amounts = new uint128[](2); + amounts[0] = amounts[1] = 1; + // It should not return anything. + calls[2] = abi.encodeCall(lockup.withdrawMultiple, (streamIds, amounts)); + + // It should return the withdrawn amount. + calls[3] = abi.encodeCall(lockup.withdrawMaxAndTransfer, (notCancelableStreamId, users.recipient)); + // It should not return anything. + calls[4] = abi.encodeCall(lockup.burn, (defaultStreamId)); + + resetPrank({ msgSender: users.recipient }); + bytes[] memory results = lockup.batch{ value: 1 wei }(calls); + + assertEq(results.length, 5); + assertEq(results[0], hex""); + assertEq(abi.decode(results[1], (uint128)), defaults.DEPOSIT_AMOUNT() - 1); + assertEq(results[2], hex""); + assertEq(abi.decode(results[3], (uint128)), defaults.DEPOSIT_AMOUNT() - 2); + assertEq(results[4], hex""); + assertEq(address(lockup).balance, initialEthBalance + 1 wei); + } +} diff --git a/tests/integration/concrete/lockup-base/batch/batch.t.sol b/tests/integration/concrete/lockup-base/batch/batch.t.sol deleted file mode 100644 index bb1c14e07..000000000 --- a/tests/integration/concrete/lockup-base/batch/batch.t.sol +++ /dev/null @@ -1,41 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity >=0.8.22 <0.9.0; - -import { Errors } from "src/libraries/Errors.sol"; - -import { Integration_Test } from "../../../Integration.t.sol"; - -contract Batch_Integration_Concrete_Test is Integration_Test { - function test_RevertWhen_CallFunctionNotExist() external { - bytes[] memory calls = new bytes[](1); - calls[0] = abi.encodeWithSignature("nonExistentFunction()"); - - vm.expectRevert(); - lockup.batch(calls); - } - - function test_RevertWhen_DataInvalid() external whenCallFunctionExists { - bytes[] memory calls = new bytes[](1); - calls[0] = abi.encodeCall(lockup.getDepositedAmount, (nullStreamId)); - - bytes memory expectedRevertData = abi.encodeWithSelector( - Errors.BatchError.selector, abi.encodeWithSelector(Errors.SablierLockupBase_Null.selector, nullStreamId) - ); - - vm.expectRevert(expectedRevertData); - - lockup.batch(calls); - } - - function test_WhenDataValid() external whenCallFunctionExists { - resetPrank({ msgSender: users.sender }); - - assertFalse(lockup.wasCanceled(defaultStreamId)); - - bytes[] memory calls = new bytes[](1); - calls[0] = abi.encodeCall(lockup.cancel, (defaultStreamId)); - - lockup.batch(calls); - assertTrue(lockup.wasCanceled(defaultStreamId)); - } -} diff --git a/tests/integration/concrete/lockup-base/batch/batch.tree b/tests/integration/concrete/lockup-base/batch/batch.tree deleted file mode 100644 index b720dde0c..000000000 --- a/tests/integration/concrete/lockup-base/batch/batch.tree +++ /dev/null @@ -1,10 +0,0 @@ -Batch_Integration_Concrete_Test -├── when call function not exist -│ └── it should revert -└── when call function exists - ├── when data invalid - │ └── it should revert - └── when data valid - └── it should call the function - - diff --git a/tests/integration/concrete/lockup-base/cancel/cancel.t.sol b/tests/integration/concrete/lockup-base/cancel/cancel.t.sol index 7a0c0bd55..1a5660625 100644 --- a/tests/integration/concrete/lockup-base/cancel/cancel.t.sol +++ b/tests/integration/concrete/lockup-base/cancel/cancel.t.sol @@ -135,11 +135,6 @@ abstract contract Cancel_Integration_Concrete_Test is Integration_Test { givenSTREAMINGStatus givenRecipientAllowedToHook { - // Allow the recipient to hook. - resetPrank({ msgSender: users.admin }); - lockup.allowToHook(address(recipientReverting)); - resetPrank({ msgSender: users.sender }); - // It should revert. vm.expectRevert("You shall not pass"); diff --git a/tests/integration/concrete/lockup-base/withdraw/withdraw.t.sol b/tests/integration/concrete/lockup-base/withdraw/withdraw.t.sol index b07f2cf1f..456f4be4f 100644 --- a/tests/integration/concrete/lockup-base/withdraw/withdraw.t.sol +++ b/tests/integration/concrete/lockup-base/withdraw/withdraw.t.sol @@ -336,11 +336,6 @@ abstract contract Withdraw_Integration_Concrete_Test is Integration_Test { givenNotCanceledStream givenRecipientAllowedToHook { - // Allow the recipient to hook. - resetPrank({ msgSender: users.admin }); - lockup.allowToHook(address(recipientReverting)); - resetPrank({ msgSender: users.sender }); - // Expect a revert. uint128 withdrawAmount = defaults.WITHDRAW_AMOUNT(); vm.expectRevert("You shall not pass"); From 17dc65135b371fa9824fffafca6c5ded5b5d4a7e Mon Sep 17 00:00:00 2001 From: smol-ninja Date: Mon, 23 Dec 2024 15:46:20 +0530 Subject: [PATCH 2/7] docs: polish natspecs --- src/abstracts/Batch.sol | 8 ++++---- src/interfaces/IBatch.sol | 8 +++++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/abstracts/Batch.sol b/src/abstracts/Batch.sol index 6e277571b..a047967fc 100644 --- a/src/abstracts/Batch.sol +++ b/src/abstracts/Batch.sol @@ -13,7 +13,7 @@ abstract contract Batch is IBatch { //////////////////////////////////////////////////////////////////////////*/ /// @inheritdoc IBatch - /// @dev Since `msg.value` can be reused across the calls, BE VERY CAREFUL when using it. Refer to + /// @dev Since `msg.value` can be reused across the calls, be VERY CAREFUL when using it. Refer to /// https://www.paradigm.xyz/2021/08/two-rights-might-make-a-wrong for more information. function batch(bytes[] calldata calls) external payable override returns (bytes[] memory results) { uint256 count = calls.length; @@ -22,18 +22,18 @@ abstract contract Batch is IBatch { for (uint256 i = 0; i < count; ++i) { (bool success, bytes memory result) = address(this).delegatecall(calls[i]); - // Revert with result data if delegatecall fails. Assembly code is used to bubble up the revert reason. + // Check: If delegatecall fails, load and bubble up the revert reason. if (!success) { assembly { // Get the length of the result stored in the first 32 bytes. let resultSize := mload(result) - // Forward the pointer by 32 bytes at the beginning of the result data. + // Forward the pointer by 32 bytes at the beginning of the result data and revert it. revert(add(32, result), resultSize) } } - // Store the result of the delegatecall. + // Push the result onto the results array. results[i] = result; } } diff --git a/src/interfaces/IBatch.sol b/src/interfaces/IBatch.sol index d6f21dbcf..b3900ee50 100644 --- a/src/interfaces/IBatch.sol +++ b/src/interfaces/IBatch.sol @@ -4,7 +4,13 @@ pragma solidity >=0.8.22; /// @notice This contract implements logic to batch call any function. interface IBatch { /// @notice Allows batched call to self, `this` contract. + /// @dev `results` contains `0x` for calls that do not return anything. + /// + /// Note: + /// - Since `msg.value` can be reused across the calls, be VERY CAREFUL when using it. Refer to + /// https://www.paradigm.xyz/2021/08/two-rights-might-make-a-wrong for more information. + /// /// @param calls An array of inputs for each call. - /// @return results An array of results from each call. Store empty bytes for calls that do not return anything. + /// @return results An array of results from each call. function batch(bytes[] calldata calls) external payable returns (bytes[] memory results); } From 1049ab8412af8a36bddaf706948cacfc5e3cf880 Mon Sep 17 00:00:00 2001 From: smol-ninja Date: Mon, 23 Dec 2024 17:06:05 +0530 Subject: [PATCH 3/7] test: unit tests for Batch --- tests/integration/concrete/batch/batch.t.sol | 69 +-------- tests/mocks/BatchMock.sol | 48 +++++++ tests/unit/concrete/batch/batch.t.sol | 144 +++++++++++++++++++ tests/unit/concrete/batch/batch.tree | 24 ++++ 4 files changed, 217 insertions(+), 68 deletions(-) create mode 100644 tests/mocks/BatchMock.sol create mode 100644 tests/unit/concrete/batch/batch.t.sol create mode 100644 tests/unit/concrete/batch/batch.tree diff --git a/tests/integration/concrete/batch/batch.t.sol b/tests/integration/concrete/batch/batch.t.sol index 4dd9a045e..862f0652c 100644 --- a/tests/integration/concrete/batch/batch.t.sol +++ b/tests/integration/concrete/batch/batch.t.sol @@ -7,18 +7,9 @@ import { Integration_Test } from "../../Integration.t.sol"; contract Batch_Integration_Concrete_Test is Integration_Test { /*////////////////////////////////////////////////////////////////////////// - REVERT + BATCH + LOCKUP //////////////////////////////////////////////////////////////////////////*/ - /// @dev The batch call includes ETH and a non-payable function. - function test_RevertWhen_ETHWithNonPayable() external { - bytes[] memory calls = new bytes[](1); - calls[0] = abi.encodeCall(lockup.isCancelable, (defaultStreamId)); - - vm.expectRevert(); - lockup.batch{ value: 1 wei }(calls); - } - /// @dev The batch call cancels a non-cancelable stream. function test_RevertWhen_LockupThrows() external { bytes[] memory calls = new bytes[](2); @@ -32,29 +23,6 @@ contract Batch_Integration_Concrete_Test is Integration_Test { lockup.batch(calls); } - /// @dev The batch call includes a non-existent function. - function test_RevertWhen_NonExistentFunction() external { - bytes[] memory calls = new bytes[](1); - calls[0] = abi.encodeWithSignature("nonExistentFunction()"); - - vm.expectRevert(); - lockup.batch(calls); - } - - /// @dev The batch call reverts with a string reason. - function test_RevertWhen_StringReason() external { - bytes[] memory calls = new bytes[](2); - calls[0] = abi.encodeCall(lockup.cancel, (defaultStreamId)); - calls[1] = abi.encodeCall(lockup.cancel, (recipientRevertStreamId)); - - vm.expectRevert("You shall not pass"); - lockup.batch(calls); - } - - /*////////////////////////////////////////////////////////////////////////// - RETURN - //////////////////////////////////////////////////////////////////////////*/ - /// @dev The batch call includes: /// - Returning state changing functions /// - Non-returning state changing functions @@ -90,41 +58,6 @@ contract Batch_Integration_Concrete_Test is Integration_Test { assertEq(results[5], hex""); } - /// @dev The batch call includes: - /// - ETH value - /// - Returning state changing functions - /// - Non-returning state changing functions - function test_BatchPayable_StateChangingFunctions() external { - uint256 expectedNextStreamId = lockup.nextStreamId(); - uint256 initialEthBalance = address(lockup).balance; - vm.warp(defaults.WARP_26_PERCENT()); - - bytes[] memory calls = new bytes[](4); - // It will return the withdrawn amount. - calls[0] = abi.encodeCall(lockup.withdrawMax, (notCancelableStreamId, users.recipient)); - // It will not return anything. - calls[1] = abi.encodeCall(lockup.cancel, (defaultStreamId)); - // It will return the stream ID created. - calls[2] = abi.encodeCall( - lockup.createWithTimestampsLL, - (defaults.createWithTimestamps(), defaults.unlockAmounts(), defaults.CLIFF_TIME()) - ); - // It will not return anything. - calls[3] = abi.encodeCall(lockup.renounce, (notTransferableStreamId)); - - bytes[] memory results = lockup.batch{ value: 1 wei }(calls); - assertEq(results.length, 4); - assertEq(abi.decode(results[0], (uint128)), defaults.WITHDRAW_AMOUNT()); - assertEq(results[1], hex""); - assertEq(abi.decode(results[2], (uint256)), expectedNextStreamId); - assertEq(results[3], hex""); - assertEq(address(lockup).balance, initialEthBalance + 1 wei); - } - - /*////////////////////////////////////////////////////////////////////////// - BATCH + LOCKUP - //////////////////////////////////////////////////////////////////////////*/ - /// @dev The batch call includes: /// - ETH value /// - All create stream functions that return a value diff --git a/tests/mocks/BatchMock.sol b/tests/mocks/BatchMock.sol new file mode 100644 index 000000000..b9d4b4edd --- /dev/null +++ b/tests/mocks/BatchMock.sol @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.22; + +import { Batch } from "src/abstracts/Batch.sol"; + +contract BatchMock is Batch { + error InvalidNumber(uint256); + + uint256 internal _number = 10; + + // A view only function. + function getNumber() public view returns (uint256) { + return _number; + } + + // A view only function that reverts. + function getNumberAndRevert() public pure returns (uint256) { + revert InvalidNumber(1); + } + + // A state changing function with no return value. + function setNumber(uint256 number) public { + _number = number; + } + + // A state changing function with payable modifier and return value. + function setNumberWithPayableAndReturn(uint256 number) public payable returns (uint256) { + _number = number; + return _number; + } + + // A state changing function with payable modifier but reverts with custom error. + function setNumberWithPayableAndRevert(uint256 number) public payable { + _number = number; + revert InvalidNumber(number); + } + + // A state changing function with payable modifier but reverts with string error. + function setNumberWithPayableAndRevertString(uint256 number) public payable { + _number = number; + revert("You cannot pass"); + } + + // A state changing function with payable modifier and no return value. + function setNumberWithPayable(uint256 number) public payable { + _number = number; + } +} diff --git a/tests/unit/concrete/batch/batch.t.sol b/tests/unit/concrete/batch/batch.t.sol new file mode 100644 index 000000000..cb0af504d --- /dev/null +++ b/tests/unit/concrete/batch/batch.t.sol @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.22; + +import { Base_Test } from "../../../Base.t.sol"; +import { BatchMock } from "../../../mocks/BatchMock.sol"; + +contract Batch_Unit_Concrete_Test is Base_Test { + BatchMock internal batchMock; + bytes[] internal calls; + bytes[] internal results; + uint256 internal newNumber = 100; + + function setUp() public virtual override { + Base_Test.setUp(); + + batchMock = new BatchMock(); + } + + function test_RevertWhen_FunctionDoesNotExist() external { + calls = new bytes[](1); + calls[0] = abi.encodeWithSignature("nonExistentFunction()"); + + // It should revert. + vm.expectRevert(bytes("")); + batchMock.batch(calls); + } + + modifier whenFunctionExists() { + _; + } + + modifier givenNonStateChangingFunction() { + _; + } + + function test_RevertWhen_FunctionReverts() external whenFunctionExists givenNonStateChangingFunction { + calls = new bytes[](1); + calls[0] = abi.encodeCall(batchMock.getNumberAndRevert, ()); + + // It should revert. + vm.expectRevert(abi.encodeWithSelector(BatchMock.InvalidNumber.selector, 1)); + batchMock.batch(calls); + } + + function test_WhenFunctionNotRevert() external whenFunctionExists givenNonStateChangingFunction { + calls = new bytes[](1); + calls[0] = abi.encodeCall(batchMock.getNumber, ()); + results = batchMock.batch(calls); + + // It should return expected value. + assertEq(results.length, 1); + assertEq(abi.decode(results[0], (uint256)), 10); + } + + modifier givenStateChangingFunction() { + _; + } + + modifier whenNotPayable() { + _; + } + + function test_RevertWhen_BatchIncludesETHValue() + external + whenFunctionExists + givenStateChangingFunction + whenNotPayable + { + calls = new bytes[](1); + calls[0] = abi.encodeCall(batchMock.setNumber, (newNumber)); + + // It should revert. + vm.expectRevert(bytes("")); + batchMock.batch{ value: 1 wei }(calls); + } + + function test_WhenBatchNotIncludeETHValue() external whenFunctionExists givenStateChangingFunction whenNotPayable { + calls = new bytes[](1); + calls[0] = abi.encodeCall(batchMock.setNumber, (newNumber)); + + results = batchMock.batch(calls); + + // It should return empty value. + assertEq(results.length, 1); + assertEq(results[0], hex""); + } + + modifier whenPayable() { + _; + } + + function test_RevertWhen_FunctionRevertsWithCustomError() + external + whenFunctionExists + givenStateChangingFunction + whenPayable + { + calls = new bytes[](1); + calls[0] = abi.encodeCall(batchMock.setNumberWithPayableAndRevert, (newNumber)); + + // It should revert. + vm.expectRevert(abi.encodeWithSelector(BatchMock.InvalidNumber.selector, newNumber)); + batchMock.batch{ value: 1 wei }(calls); + } + + function test_RevertWhen_FunctionRevertsWithStringError() + external + whenFunctionExists + givenStateChangingFunction + whenPayable + { + calls = new bytes[](1); + calls[0] = abi.encodeCall(batchMock.setNumberWithPayableAndRevertString, (newNumber)); + + // It should revert. + vm.expectRevert("You cannot pass"); + batchMock.batch{ value: 1 wei }(calls); + } + + function test_WhenFunctionReturnsAValue() external whenFunctionExists givenStateChangingFunction whenPayable { + calls = new bytes[](1); + calls[0] = abi.encodeCall(batchMock.setNumberWithPayableAndReturn, (newNumber)); + results = batchMock.batch{ value: 1 wei }(calls); + + // It should return expected value. + assertEq(results.length, 1); + assertEq(abi.decode(results[0], (uint256)), newNumber); + } + + function test_WhenFunctionDoesNotReturnAValue() + external + whenFunctionExists + givenStateChangingFunction + whenPayable + { + calls = new bytes[](1); + calls[0] = abi.encodeCall(batchMock.setNumberWithPayable, (newNumber)); + results = batchMock.batch{ value: 1 wei }(calls); + + // It should return empty value. + assertEq(results.length, 1); + assertEq(results[0], hex""); + } +} diff --git a/tests/unit/concrete/batch/batch.tree b/tests/unit/concrete/batch/batch.tree new file mode 100644 index 000000000..763be37a8 --- /dev/null +++ b/tests/unit/concrete/batch/batch.tree @@ -0,0 +1,24 @@ +Batch_Unit_Concrete_Test +├── when function does not exist +│ └── it should revert +└── when function exists + ├── given non state changing function + │ ├── when function reverts + │ │ └── it should revert + │ └── when function not revert + │ └── it should return expected value + └── given state changing function + ├── when not payable + │ ├── when batch includes ETH value + │ │ └── it should revert + │ └── when batch not include ETH value + │ └── it should return empty value + └── when payable + ├── when function reverts with custom error + │ └── it should revert + ├── when function reverts with string error + │ └── it should revert + ├── when function returns a value + │ └── it should return expected value + └── when function does not return a value + └── it should return empty value From d4a95b04e91581b7bcf0c601efe1517085315344 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Mon, 23 Dec 2024 14:30:46 +0200 Subject: [PATCH 4/7] docs: polish comments test: refactor BatchMock test: refactor branches in batch tests test: rename initialize function --- src/abstracts/Batch.sol | 10 +-- src/interfaces/IBatch.sol | 12 +-- tests/integration/Integration.t.sol | 6 +- tests/integration/concrete/batch/batch.t.sol | 77 +++++++++---------- .../renounce-multiple/renounceMultiple.t.sol | 6 +- .../renounce-multiple/renounceMultiple.tree | 6 +- .../lockup-dynamic/LockupDynamic.t.sol | 2 +- .../concrete/lockup-linear/LockupLinear.t.sol | 2 +- .../lockup-tranched/LockupTranched.t.sol | 2 +- .../fuzz/lockup-dynamic/LockupDynamic.t.sol | 2 +- .../fuzz/lockup-linear/LockupLinear.t.sol | 2 +- .../fuzz/lockup-tranched/LockupTranched.t.sol | 2 +- tests/mocks/BatchMock.sol | 22 +++--- tests/unit/concrete/batch/batch.t.sol | 33 ++++---- tests/unit/concrete/batch/batch.tree | 4 +- 15 files changed, 89 insertions(+), 99 deletions(-) diff --git a/src/abstracts/Batch.sol b/src/abstracts/Batch.sol index a047967fc..b0cbd0bf2 100644 --- a/src/abstracts/Batch.sol +++ b/src/abstracts/Batch.sol @@ -13,8 +13,8 @@ abstract contract Batch is IBatch { //////////////////////////////////////////////////////////////////////////*/ /// @inheritdoc IBatch - /// @dev Since `msg.value` can be reused across the calls, be VERY CAREFUL when using it. Refer to - /// https://www.paradigm.xyz/2021/08/two-rights-might-make-a-wrong for more information. + /// @dev Since `msg.value` can be reused across calls, be VERY CAREFUL when using it. Refer to + /// https://paradigm.xyz/2021/08/two-rights-might-make-a-wrong for more information. function batch(bytes[] calldata calls) external payable override returns (bytes[] memory results) { uint256 count = calls.length; results = new bytes[](count); @@ -22,18 +22,18 @@ abstract contract Batch is IBatch { for (uint256 i = 0; i < count; ++i) { (bool success, bytes memory result) = address(this).delegatecall(calls[i]); - // Check: If delegatecall fails, load and bubble up the revert reason. + // Check: If the delegate call failed, load and bubble up the revert data. if (!success) { assembly { // Get the length of the result stored in the first 32 bytes. let resultSize := mload(result) - // Forward the pointer by 32 bytes at the beginning of the result data and revert it. + // Forward the pointer by 32 bytes to skip the length argument, and revert with the result. revert(add(32, result), resultSize) } } - // Push the result onto the results array. + // Push the result into the results array. results[i] = result; } } diff --git a/src/interfaces/IBatch.sol b/src/interfaces/IBatch.sol index b3900ee50..dd6de1ac8 100644 --- a/src/interfaces/IBatch.sol +++ b/src/interfaces/IBatch.sol @@ -3,14 +3,10 @@ pragma solidity >=0.8.22; /// @notice This contract implements logic to batch call any function. interface IBatch { - /// @notice Allows batched call to self, `this` contract. - /// @dev `results` contains `0x` for calls that do not return anything. - /// - /// Note: - /// - Since `msg.value` can be reused across the calls, be VERY CAREFUL when using it. Refer to - /// https://www.paradigm.xyz/2021/08/two-rights-might-make-a-wrong for more information. - /// + /// @notice Allows batched calls to self, i.e., `this` contract. + /// @dev Since `msg.value` can be reused across calls, be VERY CAREFUL when using it. Refer to + /// https://paradigm.xyz/2021/08/two-rights-might-make-a-wrong for more information. /// @param calls An array of inputs for each call. - /// @return results An array of results from each call. + /// @return results An array of results from each call. Empty when the calls do not return anything. function batch(bytes[] calldata calls) external payable returns (bytes[] memory results); } diff --git a/tests/integration/Integration.t.sol b/tests/integration/Integration.t.sol index cd823a9e5..bca9c0c06 100644 --- a/tests/integration/Integration.t.sol +++ b/tests/integration/Integration.t.sol @@ -96,15 +96,15 @@ abstract contract Integration_Test is Base_Test { // Set the default Lockup model as Dynamic, we will override the default stream IDs where necessary. lockupModel = Lockup.Model.LOCKUP_DYNAMIC; - // Initialize default streams IDs. - initializeDefaultStreamIds(); + // Initialize default streams. + initializeDefaultStreams(); } /*////////////////////////////////////////////////////////////////////////// INITIALIZE-FUNCTIONS //////////////////////////////////////////////////////////////////////////*/ - function initializeDefaultStreamIds() internal { + function initializeDefaultStreams() internal { defaultStreamId = createDefaultStream(); notCancelableStreamId = createDefaultStreamNonCancelable(); notTransferableStreamId = createDefaultStreamNonTransferable(); diff --git a/tests/integration/concrete/batch/batch.t.sol b/tests/integration/concrete/batch/batch.t.sol index 862f0652c..f32d74362 100644 --- a/tests/integration/concrete/batch/batch.t.sol +++ b/tests/integration/concrete/batch/batch.t.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity >=0.8.22; +import { Solarray } from "solarray/src/Solarray.sol"; import { Errors } from "src/libraries/Errors.sol"; import { Integration_Test } from "../../Integration.t.sol"; @@ -32,34 +33,34 @@ contract Batch_Integration_Concrete_Test is Integration_Test { vm.warp(defaults.WARP_26_PERCENT()); bytes[] memory calls = new bytes[](6); - // It will return True. + // It should return True. calls[0] = abi.encodeCall(lockup.isCancelable, (defaultStreamId)); - // It will return the withdrawn amount. + // It should return the withdrawn amount. calls[1] = abi.encodeCall(lockup.withdrawMax, (notCancelableStreamId, users.recipient)); - // It will not return anything. + // It should return nothing. calls[2] = abi.encodeCall(lockup.cancel, (defaultStreamId)); - // It will return the next stream ID. + // It should return the next stream ID. calls[3] = abi.encodeCall(lockup.nextStreamId, ()); - // It will return the stream ID created. + // It should return the stream ID created. calls[4] = abi.encodeCall( lockup.createWithTimestampsLL, (defaults.createWithTimestamps(), defaults.unlockAmounts(), defaults.CLIFF_TIME()) ); - // It will not return anything. + // It should return nothing. calls[5] = abi.encodeCall(lockup.renounce, (notTransferableStreamId)); bytes[] memory results = lockup.batch(calls); - assertEq(results.length, 6); - assertEq(abi.decode(results[0], (bool)), true); - assertEq(abi.decode(results[1], (uint128)), defaults.WITHDRAW_AMOUNT()); - assertEq(results[2], hex""); - assertEq(abi.decode(results[3], (uint256)), expectedNextStreamId); - assertEq(abi.decode(results[4], (uint256)), expectedNextStreamId); - assertEq(results[5], hex""); + assertEq(results.length, 6, "batch results length"); + assertTrue(abi.decode(results[0], (bool)), "batch results[0]"); + assertEq(abi.decode(results[1], (uint128)), defaults.WITHDRAW_AMOUNT(), "batch results[1]"); + assertEq(results[2], hex"", "batch results[2]"); + assertEq(abi.decode(results[3], (uint256)), expectedNextStreamId, "batch results[3]"); + assertEq(abi.decode(results[4], (uint256)), expectedNextStreamId, "batch results[4]"); + assertEq(results[5], hex"", "batch results[5]"); } /// @dev The batch call includes: - /// - ETH value + /// - Payable functions /// - All create stream functions that return a value function test_BatchPayable_CreateStreams() external { uint256 expectedNextStreamId = lockup.nextStreamId(); @@ -85,18 +86,18 @@ contract Batch_Integration_Concrete_Test is Integration_Test { // It should return the stream IDs created. bytes[] memory results = lockup.batch{ value: 1 wei }(calls); - assertEq(results.length, 6); - assertEq(abi.decode(results[0], (uint256)), expectedNextStreamId); - assertEq(abi.decode(results[1], (uint256)), expectedNextStreamId + 1); - assertEq(abi.decode(results[2], (uint256)), expectedNextStreamId + 2); - assertEq(abi.decode(results[3], (uint256)), expectedNextStreamId + 3); - assertEq(abi.decode(results[4], (uint256)), expectedNextStreamId + 4); - assertEq(abi.decode(results[5], (uint256)), expectedNextStreamId + 5); - assertEq(address(lockup).balance, initialEthBalance + 1 wei); + assertEq(results.length, 6, "batch results length"); + assertEq(abi.decode(results[0], (uint256)), expectedNextStreamId, "batch results[0]"); + assertEq(abi.decode(results[1], (uint256)), expectedNextStreamId + 1, "batch results[1]"); + assertEq(abi.decode(results[2], (uint256)), expectedNextStreamId + 2, "batch results[2]"); + assertEq(abi.decode(results[3], (uint256)), expectedNextStreamId + 3, "batch results[3]"); + assertEq(abi.decode(results[4], (uint256)), expectedNextStreamId + 4, "batch results[4]"); + assertEq(abi.decode(results[5], (uint256)), expectedNextStreamId + 5, "batch results[5]"); + assertEq(address(lockup).balance, initialEthBalance + 1 wei, "batch contract balance"); } /// @dev The batch call includes: - /// - ETH value + /// - Payable functions /// - All recipient related functions with both returns and non-returns function test_BatchPayable_RecipientFunctions() external { uint256 initialEthBalance = address(lockup).balance; @@ -127,7 +128,7 @@ contract Batch_Integration_Concrete_Test is Integration_Test { } /// @dev The batch call includes: - /// - ETH value + /// - Payable functions /// - All sender related functions with both returns and non-returns function test_BatchPayable_SenderFunctions() external { uint256 initialEthBalance = address(lockup).balance; @@ -135,32 +136,30 @@ contract Batch_Integration_Concrete_Test is Integration_Test { vm.warp(defaults.END_TIME()); bytes[] memory calls = new bytes[](5); - // It should not return anything. + // It should return nothing. calls[0] = abi.encodeCall(lockup.withdraw, (defaultStreamId, users.recipient, 1)); // It should return the withdrawn amount. calls[1] = abi.encodeCall(lockup.withdrawMax, (defaultStreamId, users.recipient)); - uint256[] memory streamIds = new uint256[](2); - streamIds[0] = streamIds[1] = notCancelableStreamId; - uint128[] memory amounts = new uint128[](2); - amounts[0] = amounts[1] = 1; - // It should not return anything. - calls[2] = abi.encodeCall(lockup.withdrawMultiple, (streamIds, amounts)); + uint256[] memory streamIds = Solarray.uint256s(notCancelableStreamId, notCancelableStreamId); + uint128[] memory amounts = Solarray.uint128s(1, 1); + // It should return nothing. + calls[2] = abi.encodeCall(lockup.withdrawMultiple, (streamIds, amounts)); // It should return the withdrawn amount. calls[3] = abi.encodeCall(lockup.withdrawMaxAndTransfer, (notCancelableStreamId, users.recipient)); - // It should not return anything. + // It should return nothing. calls[4] = abi.encodeCall(lockup.burn, (defaultStreamId)); resetPrank({ msgSender: users.recipient }); bytes[] memory results = lockup.batch{ value: 1 wei }(calls); - assertEq(results.length, 5); - assertEq(results[0], hex""); - assertEq(abi.decode(results[1], (uint128)), defaults.DEPOSIT_AMOUNT() - 1); - assertEq(results[2], hex""); - assertEq(abi.decode(results[3], (uint128)), defaults.DEPOSIT_AMOUNT() - 2); - assertEq(results[4], hex""); - assertEq(address(lockup).balance, initialEthBalance + 1 wei); + assertEq(results.length, 5, "batch results length"); + assertEq(results[0], hex"", "batch results[0]"); + assertEq(abi.decode(results[1], (uint128)), defaults.DEPOSIT_AMOUNT() - 1, "batch results[1]"); + assertEq(results[2], hex"", "batch results[2]"); + assertEq(abi.decode(results[3], (uint128)), defaults.DEPOSIT_AMOUNT() - 2, "batch results[3]"); + assertEq(results[4], hex"", "batch results[4]"); + assertEq(address(lockup).balance, initialEthBalance + 1 wei, "batch contract balance"); } } diff --git a/tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.t.sol b/tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.t.sol index 5007d5c0c..5d286d92d 100644 --- a/tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.t.sol +++ b/tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.t.sol @@ -31,13 +31,13 @@ contract RenounceMultiple_Integration_Concrete_Test is Integration_Test { lockup.renounceMultiple(nullStreamIds); } - function test_RevertGiven_AtleastOneNullStream() external whenNoDelegateCall whenNonZeroArrayLength { + function test_RevertGiven_AtLeastOneNullStream() external whenNoDelegateCall whenNonZeroArrayLength { expectRevert_Null({ callData: abi.encodeCall(lockup.renounceMultiple, Solarray.uint256s(streamIds[0], nullStreamId)) }); } - function test_RevertGiven_AtleastOneColdStream() + function test_RevertGiven_AtLeastOneColdStream() external whenNoDelegateCall whenNonZeroArrayLength @@ -68,7 +68,7 @@ contract RenounceMultiple_Integration_Concrete_Test is Integration_Test { lockup.renounceMultiple(streamIds); } - function test_RevertGiven_AtleastOneNonCancelableStream() + function test_RevertGiven_AtLeastOneNonCancelableStream() external whenNoDelegateCall whenNonZeroArrayLength diff --git a/tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.tree b/tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.tree index 14e843efe..b170af391 100644 --- a/tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.tree +++ b/tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.tree @@ -5,16 +5,16 @@ RenounceMultiple_Integration_Concrete_Test ├── when zero array length │ └── it should do nothing └── when non zero array length - ├── given atleast one null stream + ├── given at least one null stream │ └── it should revert └── given no null streams - ├── given atleast one cold stream + ├── given at least one cold stream │ └── it should revert └── given no cold streams ├── when caller unauthorized for any │ └── it should revert └── when caller authorized for all streams - ├── given atleast one non cancelable stream + ├── given at least one non cancelable stream │ └── it should revert └── given all streams cancelable ├── it should emit {MetadataUpdate} and {RenounceLockupStream} events diff --git a/tests/integration/concrete/lockup-dynamic/LockupDynamic.t.sol b/tests/integration/concrete/lockup-dynamic/LockupDynamic.t.sol index c156a9890..c080d91aa 100644 --- a/tests/integration/concrete/lockup-dynamic/LockupDynamic.t.sol +++ b/tests/integration/concrete/lockup-dynamic/LockupDynamic.t.sol @@ -15,7 +15,7 @@ abstract contract Lockup_Dynamic_Integration_Concrete_Test is Integration_Test { Integration_Test.setUp(); lockupModel = Lockup.Model.LOCKUP_DYNAMIC; - initializeDefaultStreamIds(); + initializeDefaultStreams(); } } diff --git a/tests/integration/concrete/lockup-linear/LockupLinear.t.sol b/tests/integration/concrete/lockup-linear/LockupLinear.t.sol index c91be3585..6bc4d61c8 100644 --- a/tests/integration/concrete/lockup-linear/LockupLinear.t.sol +++ b/tests/integration/concrete/lockup-linear/LockupLinear.t.sol @@ -15,7 +15,7 @@ abstract contract Lockup_Linear_Integration_Concrete_Test is Integration_Test { Integration_Test.setUp(); lockupModel = Lockup.Model.LOCKUP_LINEAR; - initializeDefaultStreamIds(); + initializeDefaultStreams(); } } diff --git a/tests/integration/concrete/lockup-tranched/LockupTranched.t.sol b/tests/integration/concrete/lockup-tranched/LockupTranched.t.sol index 6d57fb04b..f1f55da2a 100644 --- a/tests/integration/concrete/lockup-tranched/LockupTranched.t.sol +++ b/tests/integration/concrete/lockup-tranched/LockupTranched.t.sol @@ -15,7 +15,7 @@ abstract contract Lockup_Tranched_Integration_Concrete_Test is Integration_Test Integration_Test.setUp(); lockupModel = Lockup.Model.LOCKUP_TRANCHED; - initializeDefaultStreamIds(); + initializeDefaultStreams(); } } diff --git a/tests/integration/fuzz/lockup-dynamic/LockupDynamic.t.sol b/tests/integration/fuzz/lockup-dynamic/LockupDynamic.t.sol index bc2253003..d3c0dd784 100644 --- a/tests/integration/fuzz/lockup-dynamic/LockupDynamic.t.sol +++ b/tests/integration/fuzz/lockup-dynamic/LockupDynamic.t.sol @@ -12,7 +12,7 @@ abstract contract Lockup_Dynamic_Integration_Fuzz_Test is Integration_Test { Integration_Test.setUp(); lockupModel = Lockup.Model.LOCKUP_DYNAMIC; - initializeDefaultStreamIds(); + initializeDefaultStreams(); } } diff --git a/tests/integration/fuzz/lockup-linear/LockupLinear.t.sol b/tests/integration/fuzz/lockup-linear/LockupLinear.t.sol index 7b3e14589..450cdc972 100644 --- a/tests/integration/fuzz/lockup-linear/LockupLinear.t.sol +++ b/tests/integration/fuzz/lockup-linear/LockupLinear.t.sol @@ -13,7 +13,7 @@ abstract contract Lockup_Linear_Integration_Fuzz_Test is Integration_Test { Integration_Test.setUp(); lockupModel = Lockup.Model.LOCKUP_LINEAR; - initializeDefaultStreamIds(); + initializeDefaultStreams(); } } diff --git a/tests/integration/fuzz/lockup-tranched/LockupTranched.t.sol b/tests/integration/fuzz/lockup-tranched/LockupTranched.t.sol index 6746d0d8c..db48a6b83 100644 --- a/tests/integration/fuzz/lockup-tranched/LockupTranched.t.sol +++ b/tests/integration/fuzz/lockup-tranched/LockupTranched.t.sol @@ -12,7 +12,7 @@ abstract contract Lockup_Tranched_Integration_Fuzz_Test is Integration_Test { Integration_Test.setUp(); lockupModel = Lockup.Model.LOCKUP_TRANCHED; - initializeDefaultStreamIds(); + initializeDefaultStreams(); } } diff --git a/tests/mocks/BatchMock.sol b/tests/mocks/BatchMock.sol index b9d4b4edd..84e3bd902 100644 --- a/tests/mocks/BatchMock.sol +++ b/tests/mocks/BatchMock.sol @@ -6,7 +6,7 @@ import { Batch } from "src/abstracts/Batch.sol"; contract BatchMock is Batch { error InvalidNumber(uint256); - uint256 internal _number = 10; + uint256 internal _number = 42; // A view only function. function getNumber() public view returns (uint256) { @@ -18,31 +18,31 @@ contract BatchMock is Batch { revert InvalidNumber(1); } - // A state changing function with no return value. + // A state changing function with no payable modifier and no return value. function setNumber(uint256 number) public { _number = number; } - // A state changing function with payable modifier and return value. + // A state changing function with a payable modifier and no return value. + function setNumberWithPayable(uint256 number) public payable { + _number = number; + } + + // A state changing function with a payable modifier and a return value. function setNumberWithPayableAndReturn(uint256 number) public payable returns (uint256) { _number = number; return _number; } - // A state changing function with payable modifier but reverts with custom error. - function setNumberWithPayableAndRevert(uint256 number) public payable { + // A state changing function with a payable modifier, which reverts with a custom error. + function setNumberWithPayableAndRevertError(uint256 number) public payable { _number = number; revert InvalidNumber(number); } - // A state changing function with payable modifier but reverts with string error. + // A state changing function with a payable modifier, which reverts with a reason string. function setNumberWithPayableAndRevertString(uint256 number) public payable { _number = number; revert("You cannot pass"); } - - // A state changing function with payable modifier and no return value. - function setNumberWithPayable(uint256 number) public payable { - _number = number; - } } diff --git a/tests/unit/concrete/batch/batch.t.sol b/tests/unit/concrete/batch/batch.t.sol index cb0af504d..624b6fceb 100644 --- a/tests/unit/concrete/batch/batch.t.sol +++ b/tests/unit/concrete/batch/batch.t.sol @@ -7,8 +7,8 @@ import { BatchMock } from "../../../mocks/BatchMock.sol"; contract Batch_Unit_Concrete_Test is Base_Test { BatchMock internal batchMock; bytes[] internal calls; - bytes[] internal results; uint256 internal newNumber = 100; + bytes[] internal results; function setUp() public virtual override { Base_Test.setUp(); @@ -29,11 +29,11 @@ contract Batch_Unit_Concrete_Test is Base_Test { _; } - modifier givenNonStateChangingFunction() { + modifier whenNonStateChangingFunction() { _; } - function test_RevertWhen_FunctionReverts() external whenFunctionExists givenNonStateChangingFunction { + function test_RevertWhen_FunctionReverts() external whenFunctionExists whenNonStateChangingFunction { calls = new bytes[](1); calls[0] = abi.encodeCall(batchMock.getNumberAndRevert, ()); @@ -42,7 +42,7 @@ contract Batch_Unit_Concrete_Test is Base_Test { batchMock.batch(calls); } - function test_WhenFunctionNotRevert() external whenFunctionExists givenNonStateChangingFunction { + function test_WhenFunctionNotRevert() external whenFunctionExists whenNonStateChangingFunction { calls = new bytes[](1); calls[0] = abi.encodeCall(batchMock.getNumber, ()); results = batchMock.batch(calls); @@ -52,7 +52,7 @@ contract Batch_Unit_Concrete_Test is Base_Test { assertEq(abi.decode(results[0], (uint256)), 10); } - modifier givenStateChangingFunction() { + modifier whenStateChangingFunction() { _; } @@ -63,7 +63,7 @@ contract Batch_Unit_Concrete_Test is Base_Test { function test_RevertWhen_BatchIncludesETHValue() external whenFunctionExists - givenStateChangingFunction + whenStateChangingFunction whenNotPayable { calls = new bytes[](1); @@ -74,13 +74,13 @@ contract Batch_Unit_Concrete_Test is Base_Test { batchMock.batch{ value: 1 wei }(calls); } - function test_WhenBatchNotIncludeETHValue() external whenFunctionExists givenStateChangingFunction whenNotPayable { + function test_WhenBatchNotIncludeETHValue() external whenFunctionExists whenStateChangingFunction whenNotPayable { calls = new bytes[](1); calls[0] = abi.encodeCall(batchMock.setNumber, (newNumber)); results = batchMock.batch(calls); - // It should return empty value. + // It should return the empty string. assertEq(results.length, 1); assertEq(results[0], hex""); } @@ -92,11 +92,11 @@ contract Batch_Unit_Concrete_Test is Base_Test { function test_RevertWhen_FunctionRevertsWithCustomError() external whenFunctionExists - givenStateChangingFunction + whenStateChangingFunction whenPayable { calls = new bytes[](1); - calls[0] = abi.encodeCall(batchMock.setNumberWithPayableAndRevert, (newNumber)); + calls[0] = abi.encodeCall(batchMock.setNumberWithPayableAndRevertError, (newNumber)); // It should revert. vm.expectRevert(abi.encodeWithSelector(BatchMock.InvalidNumber.selector, newNumber)); @@ -106,7 +106,7 @@ contract Batch_Unit_Concrete_Test is Base_Test { function test_RevertWhen_FunctionRevertsWithStringError() external whenFunctionExists - givenStateChangingFunction + whenStateChangingFunction whenPayable { calls = new bytes[](1); @@ -117,7 +117,7 @@ contract Batch_Unit_Concrete_Test is Base_Test { batchMock.batch{ value: 1 wei }(calls); } - function test_WhenFunctionReturnsAValue() external whenFunctionExists givenStateChangingFunction whenPayable { + function test_WhenFunctionReturnsAValue() external whenFunctionExists whenStateChangingFunction whenPayable { calls = new bytes[](1); calls[0] = abi.encodeCall(batchMock.setNumberWithPayableAndReturn, (newNumber)); results = batchMock.batch{ value: 1 wei }(calls); @@ -127,17 +127,12 @@ contract Batch_Unit_Concrete_Test is Base_Test { assertEq(abi.decode(results[0], (uint256)), newNumber); } - function test_WhenFunctionDoesNotReturnAValue() - external - whenFunctionExists - givenStateChangingFunction - whenPayable - { + function test_WhenFunctionDoesNotReturnAValue() external whenFunctionExists whenStateChangingFunction whenPayable { calls = new bytes[](1); calls[0] = abi.encodeCall(batchMock.setNumberWithPayable, (newNumber)); results = batchMock.batch{ value: 1 wei }(calls); - // It should return empty value. + // It should return an empty value. assertEq(results.length, 1); assertEq(results[0], hex""); } diff --git a/tests/unit/concrete/batch/batch.tree b/tests/unit/concrete/batch/batch.tree index 763be37a8..f42cca878 100644 --- a/tests/unit/concrete/batch/batch.tree +++ b/tests/unit/concrete/batch/batch.tree @@ -2,12 +2,12 @@ Batch_Unit_Concrete_Test ├── when function does not exist │ └── it should revert └── when function exists - ├── given non state changing function + ├── when non state-changing function │ ├── when function reverts │ │ └── it should revert │ └── when function not revert │ └── it should return expected value - └── given state changing function + └── when state-changing function ├── when not payable │ ├── when batch includes ETH value │ │ └── it should revert From e4e0a44846197b786c5a27d994d72f1109afcae0 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Mon, 23 Dec 2024 15:11:58 +0200 Subject: [PATCH 5/7] test: fix failing test --- tests/unit/concrete/batch/batch.t.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/unit/concrete/batch/batch.t.sol b/tests/unit/concrete/batch/batch.t.sol index 624b6fceb..11eb9a2ee 100644 --- a/tests/unit/concrete/batch/batch.t.sol +++ b/tests/unit/concrete/batch/batch.t.sol @@ -47,9 +47,9 @@ contract Batch_Unit_Concrete_Test is Base_Test { calls[0] = abi.encodeCall(batchMock.getNumber, ()); results = batchMock.batch(calls); - // It should return expected value. - assertEq(results.length, 1); - assertEq(abi.decode(results[0], (uint256)), 10); + // It should return the expected value. + assertEq(results.length, 1, "batch results length"); + assertEq(abi.decode(results[0], (uint256)), 42, "batch results[0]"); } modifier whenStateChangingFunction() { @@ -81,8 +81,8 @@ contract Batch_Unit_Concrete_Test is Base_Test { results = batchMock.batch(calls); // It should return the empty string. - assertEq(results.length, 1); - assertEq(results[0], hex""); + assertEq(results.length, 1, "batch results length"); + assertEq(results[0], hex"", "batch results[0]"); } modifier whenPayable() { @@ -123,8 +123,8 @@ contract Batch_Unit_Concrete_Test is Base_Test { results = batchMock.batch{ value: 1 wei }(calls); // It should return expected value. - assertEq(results.length, 1); - assertEq(abi.decode(results[0], (uint256)), newNumber); + assertEq(results.length, 1, "batch results length"); + assertEq(abi.decode(results[0], (uint256)), newNumber, "batch results[0]"); } function test_WhenFunctionDoesNotReturnAValue() external whenFunctionExists whenStateChangingFunction whenPayable { @@ -133,7 +133,7 @@ contract Batch_Unit_Concrete_Test is Base_Test { results = batchMock.batch{ value: 1 wei }(calls); // It should return an empty value. - assertEq(results.length, 1); - assertEq(results[0], hex""); + assertEq(results.length, 1, "batch results length"); + assertEq(results[0], hex"", "batch results[0]"); } } From 00facce845209b55a24f3c021730f4c5f916362e Mon Sep 17 00:00:00 2001 From: smol-ninja Date: Mon, 23 Dec 2024 20:58:51 +0530 Subject: [PATCH 6/7] test: polish comments --- tests/integration/concrete/batch/batch.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/concrete/batch/batch.t.sol b/tests/integration/concrete/batch/batch.t.sol index f32d74362..087afba87 100644 --- a/tests/integration/concrete/batch/batch.t.sol +++ b/tests/integration/concrete/batch/batch.t.sol @@ -60,7 +60,7 @@ contract Batch_Integration_Concrete_Test is Integration_Test { } /// @dev The batch call includes: - /// - Payable functions + /// - ETH value /// - All create stream functions that return a value function test_BatchPayable_CreateStreams() external { uint256 expectedNextStreamId = lockup.nextStreamId(); @@ -97,7 +97,7 @@ contract Batch_Integration_Concrete_Test is Integration_Test { } /// @dev The batch call includes: - /// - Payable functions + /// - ETH value /// - All recipient related functions with both returns and non-returns function test_BatchPayable_RecipientFunctions() external { uint256 initialEthBalance = address(lockup).balance; @@ -128,7 +128,7 @@ contract Batch_Integration_Concrete_Test is Integration_Test { } /// @dev The batch call includes: - /// - Payable functions + /// - ETH value /// - All sender related functions with both returns and non-returns function test_BatchPayable_SenderFunctions() external { uint256 initialEthBalance = address(lockup).balance; From f890214f5b565ff8ab301a30cf0b032342c5e6a0 Mon Sep 17 00:00:00 2001 From: smol-ninja Date: Tue, 24 Dec 2024 10:34:46 +0530 Subject: [PATCH 7/7] chore: polish batch comments and tests Co-authored-by: andreivladbrg --- src/abstracts/Batch.sol | 3 +-- tests/integration/concrete/batch/batch.t.sol | 26 ++++++++++---------- tests/unit/concrete/batch/batch.t.sol | 4 +-- tests/unit/concrete/batch/batch.tree | 4 +-- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/abstracts/Batch.sol b/src/abstracts/Batch.sol index b0cbd0bf2..129d108ad 100644 --- a/src/abstracts/Batch.sol +++ b/src/abstracts/Batch.sol @@ -6,7 +6,6 @@ import { IBatch } from "../interfaces/IBatch.sol"; /// @title Batch /// @notice See the documentation in {IBatch}. -/// @dev Forked from: https://github.com/boringcrypto/BoringSolidity/blob/master/contracts/BoringBatchable.sol abstract contract Batch is IBatch { /*////////////////////////////////////////////////////////////////////////// USER-FACING NON-CONSTANT FUNCTIONS @@ -22,7 +21,7 @@ abstract contract Batch is IBatch { for (uint256 i = 0; i < count; ++i) { (bool success, bytes memory result) = address(this).delegatecall(calls[i]); - // Check: If the delegate call failed, load and bubble up the revert data. + // Check: If the delegatecall failed, load and bubble up the revert data. if (!success) { assembly { // Get the length of the result stored in the first 32 bytes. diff --git a/tests/integration/concrete/batch/batch.t.sol b/tests/integration/concrete/batch/batch.t.sol index 087afba87..c231a2be6 100644 --- a/tests/integration/concrete/batch/batch.t.sol +++ b/tests/integration/concrete/batch/batch.t.sol @@ -53,10 +53,10 @@ contract Batch_Integration_Concrete_Test is Integration_Test { assertEq(results.length, 6, "batch results length"); assertTrue(abi.decode(results[0], (bool)), "batch results[0]"); assertEq(abi.decode(results[1], (uint128)), defaults.WITHDRAW_AMOUNT(), "batch results[1]"); - assertEq(results[2], hex"", "batch results[2]"); + assertEq(results[2], "", "batch results[2]"); assertEq(abi.decode(results[3], (uint256)), expectedNextStreamId, "batch results[3]"); assertEq(abi.decode(results[4], (uint256)), expectedNextStreamId, "batch results[4]"); - assertEq(results[5], hex"", "batch results[5]"); + assertEq(results[5], "", "batch results[5]"); } /// @dev The batch call includes: @@ -93,7 +93,7 @@ contract Batch_Integration_Concrete_Test is Integration_Test { assertEq(abi.decode(results[3], (uint256)), expectedNextStreamId + 3, "batch results[3]"); assertEq(abi.decode(results[4], (uint256)), expectedNextStreamId + 4, "batch results[4]"); assertEq(abi.decode(results[5], (uint256)), expectedNextStreamId + 5, "batch results[5]"); - assertEq(address(lockup).balance, initialEthBalance + 1 wei, "batch contract balance"); + assertEq(address(lockup).balance, initialEthBalance + 1 wei, "lockup contract balance"); } /// @dev The batch call includes: @@ -119,12 +119,12 @@ contract Batch_Integration_Concrete_Test is Integration_Test { bytes[] memory results = lockup.batch{ value: 1 wei }(calls); - assertEq(results.length, 4); - assertEq(results[0], hex""); - assertEq(results[1], hex""); - assertEq(results[2], hex""); - assertEq(results[3], hex""); - assertEq(address(lockup).balance, initialEthBalance + 1 wei); + assertEq(results.length, 4, "batch results length"); + assertEq(results[0], "", "batch results[0]"); + assertEq(results[1], "", "batch results[1]"); + assertEq(results[2], "", "batch results[2]"); + assertEq(results[3], "", "batch results[3]"); + assertEq(address(lockup).balance, initialEthBalance + 1 wei, "lockup contract balance"); } /// @dev The batch call includes: @@ -155,11 +155,11 @@ contract Batch_Integration_Concrete_Test is Integration_Test { bytes[] memory results = lockup.batch{ value: 1 wei }(calls); assertEq(results.length, 5, "batch results length"); - assertEq(results[0], hex"", "batch results[0]"); + assertEq(results[0], "", "batch results[0]"); assertEq(abi.decode(results[1], (uint128)), defaults.DEPOSIT_AMOUNT() - 1, "batch results[1]"); - assertEq(results[2], hex"", "batch results[2]"); + assertEq(results[2], "", "batch results[2]"); assertEq(abi.decode(results[3], (uint128)), defaults.DEPOSIT_AMOUNT() - 2, "batch results[3]"); - assertEq(results[4], hex"", "batch results[4]"); - assertEq(address(lockup).balance, initialEthBalance + 1 wei, "batch contract balance"); + assertEq(results[4], "", "batch results[4]"); + assertEq(address(lockup).balance, initialEthBalance + 1 wei, "lockup contract balance"); } } diff --git a/tests/unit/concrete/batch/batch.t.sol b/tests/unit/concrete/batch/batch.t.sol index 11eb9a2ee..a32bc5f67 100644 --- a/tests/unit/concrete/batch/batch.t.sol +++ b/tests/unit/concrete/batch/batch.t.sol @@ -82,7 +82,7 @@ contract Batch_Unit_Concrete_Test is Base_Test { // It should return the empty string. assertEq(results.length, 1, "batch results length"); - assertEq(results[0], hex"", "batch results[0]"); + assertEq(results[0], "", "batch results[0]"); } modifier whenPayable() { @@ -134,6 +134,6 @@ contract Batch_Unit_Concrete_Test is Base_Test { // It should return an empty value. assertEq(results.length, 1, "batch results length"); - assertEq(results[0], hex"", "batch results[0]"); + assertEq(results[0], "", "batch results[0]"); } } diff --git a/tests/unit/concrete/batch/batch.tree b/tests/unit/concrete/batch/batch.tree index f42cca878..35501ea18 100644 --- a/tests/unit/concrete/batch/batch.tree +++ b/tests/unit/concrete/batch/batch.tree @@ -2,12 +2,12 @@ Batch_Unit_Concrete_Test ├── when function does not exist │ └── it should revert └── when function exists - ├── when non state-changing function + ├── when non state changing function │ ├── when function reverts │ │ └── it should revert │ └── when function not revert │ └── it should return expected value - └── when state-changing function + └── when state changing function ├── when not payable │ ├── when batch includes ETH value │ │ └── it should revert