Skip to content

Commit

Permalink
feat: use delegate-call in withdraw-multiple (#1101)
Browse files Browse the repository at this point in the history 
* feat: use try-catch statement in withdraw-multiple

* refactor: rename error message to InvalidWithdrawInWithdrawMultiple

* docs: improve writing in comments

refactor: rename error

* use delegatecall in withdrawMultiple

* test: simplify withdrawMultiple tests

Co-authored-by: Andrei Vlad Birgaoanu <99738872+andreivladbrg@users.noreply.github.com>

---------

Co-authored-by: Paul Razvan Berg <prberg@proton.me>
Co-authored-by: Andrei Vlad Birgaoanu <99738872+andreivladbrg@users.noreply.github.com>
  • Loading branch information
@smol-ninja @PaulRBerg @andreivladbrg
3 people authored Nov 30, 2024
1 parent d03cab8 commit b5d35c4
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 151 deletions.
1 change: 0 additions & 1 deletion .solhint.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
"func-visibility": ["error", { "ignoreConstructors": true }],
"gas-custom-errors": "off",
"max-states-count": ["warn", 20],
"imports-order": "warn",
"max-line-length": ["error", 124],
"named-parameters-mapping": "warn",
"no-empty-blocks": "off",
Expand Down
12 changes: 9 additions & 3 deletions src/abstracts/SablierLockupBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -506,10 +506,16 @@ abstract contract SablierLockupBase is
revert Errors.SablierLockupBase_WithdrawArrayCountsNotEqual(streamIdsCount, amountsCount);
}

// Iterate over the provided array of stream IDs, and withdraw from each stream to the recipient.
// Iterate over the provided array of stream IDs and withdraw from each stream to the recipient.
for (uint256 i = 0; i < streamIdsCount; ++i) {
// Checks, Effects and Interactions: check the parameters and make the withdrawal.
withdraw({ streamId: streamIds[i], to: _ownerOf(streamIds[i]), amount: amounts[i] });
// Checks, Effects and Interactions: withdraw using delegatecall.
(bool success, bytes memory result) = address(this).delegatecall(
abi.encodeCall(ISablierLockupBase.withdraw, (streamIds[i], _ownerOf(streamIds[i]), amounts[i]))
);
// If the withdrawal reverts, log it using an event, and continue with the next stream.
if (!success) {
emit InvalidWithdrawalInWithdrawMultiple(streamIds[i], result);
}
}
}

Expand Down
8 changes: 7 additions & 1 deletion src/interfaces/ISablierLockupBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ interface ISablierLockupBase is
/// @param feeAmount The amount of collected fees.
event CollectFees(address indexed admin, uint256 indexed feeAmount);

/// @notice Emitted when withdrawing from multiple streams and one particular withdrawal reverts.
/// @param streamId The stream ID that reverted during withdraw.
/// @param revertData The error data returned by the reverted withdraw.
event InvalidWithdrawalInWithdrawMultiple(uint256 streamId, bytes revertData);

/// @notice Emitted when a sender gives up the right to cancel a stream.
/// @param streamId The ID of the stream.
event RenounceLockupStream(uint256 indexed streamId);
Expand Down Expand Up @@ -375,7 +380,8 @@ interface ISablierLockupBase is

/// @notice Withdraws tokens from streams to the recipient of each stream.
///
/// @dev Emits multiple {Transfer}, {WithdrawFromLockupStream}, and {MetadataUpdate} events.
/// @dev Emits multiple {Transfer}, {WithdrawFromLockupStream}, and {MetadataUpdate} events. For each stream that
/// reverted the withdrawal, it emits an {InvalidWithdrawalInWithdrawMultiple} event.
///
/// Notes:
/// - This function attempts to call a hook on the recipient of each stream, unless `msg.sender` is the recipient.
Expand Down
175 changes: 66 additions & 109 deletions tests/integration/concrete/lockup-base/withdraw-multiple/withdrawMultiple.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;

import { Solarray } from "solarray/src/Solarray.sol";
import { ISablierLockupBase } from "src/interfaces/ISablierLockupBase.sol";
import { Errors } from "src/libraries/Errors.sol";
import { Lockup } from "src/types/DataTypes.sol";
Expand All @@ -25,25 +24,11 @@ contract WithdrawMultiple_Integration_Concrete_Test is Integration_Test {

originalTime = defaults.START_TIME();

withdrawMultipleStreamIds = warpAndCreateStreams(defaults.START_TIME());

withdrawAmounts.push(defaults.WITHDRAW_AMOUNT());
withdrawAmounts.push(defaults.DEPOSIT_AMOUNT());
withdrawAmounts.push(defaults.WITHDRAW_AMOUNT() / 2);
}

function warpAndCreateStreams(uint40 warpTime) internal returns (uint256[3] memory streamIds) {
vm.warp({ newTimestamp: warpTime });

// Create three test streams:
// 1. A default stream
// 2. A stream with an early end time
// 3. A stream meant to be canceled before the withdrawal is made
streamIds[0] = createDefaultStream();
streamIds[1] = createDefaultStreamWithEndTime(defaults.WARP_26_PERCENT() + 1);
streamIds[2] = createDefaultStream();
}

function test_RevertWhen_DelegateCall() external {
expectRevert_DelegateCall({
callData: abi.encodeCall(lockup.withdrawMultiple, (withdrawMultipleStreamIds, withdrawAmounts))
Expand All @@ -69,121 +54,85 @@ contract WithdrawMultiple_Integration_Concrete_Test is Integration_Test {
lockup.withdrawMultiple(streamIds, amounts);
}

function test_RevertGiven_AtleastOneNullStream()
external
whenNoDelegateCall
whenEqualArraysLength
whenNonZeroArrayLength
{
uint256[] memory streamIds =
Solarray.uint256s(nullStreamId, withdrawMultipleStreamIds[0], withdrawMultipleStreamIds[1]);

// Simulate the passage of time.
vm.warp({ newTimestamp: defaults.WARP_26_PERCENT() + 1 });

// It should revert.
vm.expectRevert(abi.encodeWithSelector(Errors.SablierLockupBase_Null.selector, nullStreamId));

// Withdraw from multiple streams.
lockup.withdrawMultiple({ streamIds: streamIds, amounts: withdrawAmounts });
}

function test_RevertGiven_AtleastOneDEPLETEDStream()
external
whenNoDelegateCall
whenEqualArraysLength
whenNonZeroArrayLength
givenNoNullStreams
{
// Simulate the passage of time.
vm.warp({ newTimestamp: defaults.END_TIME() });
/// @dev This modifier runs the test in four different modes:
/// - Stream's sender as caller
/// - Stream's recipient as caller
/// - Approved NFT operator as caller
/// - Random caller (Alice)
modifier whenCallerAuthorizedForAllStreams() override {
withdrawMultipleStreamIds = _warpAndCreateStreams({ warpTime: originalTime });
caller = users.sender;
_;

// Deplete the first test stream.
lockup.withdrawMax({ streamId: withdrawMultipleStreamIds[0], to: users.recipient });
withdrawMultipleStreamIds = _warpAndCreateStreams({ warpTime: originalTime });
caller = users.recipient;
_;

// It should revert.
vm.expectRevert(
abi.encodeWithSelector(Errors.SablierLockupBase_StreamDepleted.selector, withdrawMultipleStreamIds[0])
);
withdrawMultipleStreamIds = _warpAndCreateStreams({ warpTime: originalTime });
caller = users.operator;
_;

// Withdraw from multiple streams.
lockup.withdrawMultiple({ streamIds: withdrawMultipleStreamIds, amounts: withdrawAmounts });
withdrawMultipleStreamIds = _warpAndCreateStreams({ warpTime: originalTime });
caller = users.alice;
_;
}

function test_RevertWhen_AtleastOneZeroAmount()
function test_WhenOneStreamIDReverts()
external
whenNoDelegateCall
whenEqualArraysLength
whenNonZeroArrayLength
givenNoNullStreams
givenNoDEPLETEDStreams
whenCallerAuthorizedForAllStreams
{
// Simulate the passage of time.
vm.warp({ newTimestamp: defaults.WARP_26_PERCENT() + 1 });

// Run the test.
uint128[] memory amounts = Solarray.uint128s(defaults.WITHDRAW_AMOUNT(), 0, 0);

// It should revert.
vm.expectRevert(
abi.encodeWithSelector(Errors.SablierLockupBase_WithdrawAmountZero.selector, withdrawMultipleStreamIds[1])
);
lockup.withdrawMultiple({ streamIds: withdrawMultipleStreamIds, amounts: amounts });
}

function test_RevertWhen_AtleastOneAmountOverdraws()
external
whenNoDelegateCall
whenEqualArraysLength
whenNonZeroArrayLength
givenNoNullStreams
givenNoDEPLETEDStreams
whenNoZeroAmounts
{
// Simulate the passage of time.
vm.warp({ newTimestamp: defaults.WARP_26_PERCENT() + 1 });
// Run the test with the caller provided in {whenCallerAuthorizedForAllStreams}.
resetPrank({ msgSender: caller });

// Run the test.
uint128 withdrawableAmount = lockup.withdrawableAmountOf(withdrawMultipleStreamIds[2]);
uint128[] memory amounts = Solarray.uint128s(withdrawAmounts[0], withdrawAmounts[1], MAX_UINT128);
// It should emit {WithdrawFromLockupStream} events for first two streams.
vm.expectEmit({ emitter: address(lockup) });
emit ISablierLockupBase.WithdrawFromLockupStream({
streamId: withdrawMultipleStreamIds[0],
to: users.recipient,
token: dai,
amount: withdrawAmounts[0]
});
vm.expectEmit({ emitter: address(lockup) });
emit ISablierLockupBase.WithdrawFromLockupStream({
streamId: withdrawMultipleStreamIds[1],
to: users.recipient,
token: dai,
amount: withdrawAmounts[1]
});

// It should revert.
vm.expectRevert(
abi.encodeWithSelector(
// It should emit {InvalidWithdrawalInWithdrawMultiple} event for third stream.
vm.expectEmit({ emitter: address(lockup) });
emit ISablierLockupBase.InvalidWithdrawalInWithdrawMultiple({
streamId: withdrawMultipleStreamIds[2],
revertData: abi.encodeWithSelector(
Errors.SablierLockupBase_Overdraw.selector,
withdrawMultipleStreamIds[2],
MAX_UINT128,
withdrawableAmount
lockup.withdrawableAmountOf(withdrawMultipleStreamIds[2])
)
);
lockup.withdrawMultiple({ streamIds: withdrawMultipleStreamIds, amounts: amounts });
}

/// @dev This modifier runs the test in three different modes:
/// - Stream's sender as caller
/// - Stream's recipient as caller
/// - Approved NFT operator as caller
modifier whenCallerAuthorizedForAllStreams() override {
caller = users.sender;
_;
});

withdrawMultipleStreamIds = warpAndCreateStreams({ warpTime: originalTime });
caller = users.recipient;
_;
// Make the withdrawals with overdrawn withdraw amount for the third stream.
withdrawAmounts[2] = MAX_UINT128;
lockup.withdrawMultiple({ streamIds: withdrawMultipleStreamIds, amounts: withdrawAmounts });

withdrawMultipleStreamIds = warpAndCreateStreams({ warpTime: originalTime });
caller = users.operator;
_;
// It should update the withdrawn amounts only for first two streams.
assertEq(lockup.getWithdrawnAmount(withdrawMultipleStreamIds[0]), withdrawAmounts[0], "withdrawnAmount0");
assertEq(lockup.getWithdrawnAmount(withdrawMultipleStreamIds[1]), withdrawAmounts[1], "withdrawnAmount1");
assertEq(lockup.getWithdrawnAmount(withdrawMultipleStreamIds[2]), 0, "withdrawnAmount2");
}

function test_WhenNoAmountsOverdraw()
function test_WhenNoStreamIDsRevert()
external
whenNoDelegateCall
whenEqualArraysLength
whenNonZeroArrayLength
givenNoNullStreams
givenNoDEPLETEDStreams
whenNoZeroAmounts
whenCallerAuthorizedForAllStreams
{
// Simulate the passage of time.
Expand All @@ -196,12 +145,7 @@ contract WithdrawMultiple_Integration_Concrete_Test is Integration_Test {
// Run the test with the caller provided in {whenCallerAuthorizedForAllStreams}.
resetPrank({ msgSender: caller });

// It should make the withdrawals.
expectCallToTransfer({ to: users.recipient, value: withdrawAmounts[0] });
expectCallToTransfer({ to: users.recipient, value: withdrawAmounts[1] });
expectCallToTransfer({ to: users.recipient, value: withdrawAmounts[2] });

// It should emit multiple {WithdrawFromLockupStream} events.
// It should emit {WithdrawFromLockupStream} events for all streams.
vm.expectEmit({ emitter: address(lockup) });
emit ISablierLockupBase.WithdrawFromLockupStream({
streamId: withdrawMultipleStreamIds[0],
Expand Down Expand Up @@ -242,4 +186,17 @@ contract WithdrawMultiple_Integration_Concrete_Test is Integration_Test {
assertEq(lockup.getRecipient(withdrawMultipleStreamIds[1]), users.recipient, "NFT owner1");
assertEq(lockup.getRecipient(withdrawMultipleStreamIds[2]), users.recipient, "NFT owner2");
}

// A helper function to warp to the original time and create test streams.
function _warpAndCreateStreams(uint40 warpTime) private returns (uint256[3] memory streamIds) {
vm.warp({ newTimestamp: warpTime });

// Create three test streams:
// 1. A default stream
// 2. A stream with an early end time
// 3. A stream meant to be canceled before the withdrawal is made
streamIds[0] = createDefaultStream();
streamIds[1] = createDefaultStreamWithEndTime(defaults.WARP_26_PERCENT() + 1);
streamIds[2] = createDefaultStream();
}
}
25 changes: 9 additions & 16 deletions tests/integration/concrete/lockup-base/withdraw-multiple/withdrawMultiple.tree
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,12 @@ WithdrawMultiple_Integration_Concrete_Test
├── when zero array length
│ └── it should do nothing
└── when non zero array length
├── given atleast one null stream
│ └── it should revert
└── given no null streams
├── given atleast one DEPLETED stream
│ └── it should revert
└── given no DEPLETED streams
├── when atleast one zero amount
│ └── it should revert
└── when no zero amounts
├── when atleast one amount overdraws
│ └── it should revert
└── when no amounts overdraw
├── it should make the withdrawals
├── it should update the statuses
├── it should update the withdrawn amounts
└── it should emit multiple {WithdrawFromLockupStream} events
├── when one stream ID reverts
│ ├── it should emit {WithdrawFromLockupStream} events for first two streams
│ ├── it should emit {InvalidWithdrawalInWithdrawMultiple} event for third stream
│ └── it should update the withdrawn amounts only for first two streams
└── when no stream IDs revert
├── it should make the withdrawals on all streams
├── it should update the statuses
├── it should update the withdrawn amounts
└── it should emit {WithdrawFromLockupStream} events for all streams
25 changes: 4 additions & 21 deletions tests/utils/Modifiers.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ abstract contract Modifiers is Fuzzers {
_;
}

modifier givenNoNullStreams() {
_;
}

modifier givenNotCanceledStream() {
_;
}
Expand Down Expand Up @@ -452,19 +456,6 @@ abstract contract Modifiers is Fuzzers {
WITHDRAW-MULTIPLE
//////////////////////////////////////////////////////////////////////////*/

modifier whenArraysEqual() {
_;
}

modifier givenNoDEPLETEDStreams() {
vm.warp({ newTimestamp: defaults.START_TIME() });
_;
}

modifier givenNoNullStreams() {
_;
}

modifier whenEqualArraysLength() {
_;
}
Expand All @@ -473,14 +464,6 @@ abstract contract Modifiers is Fuzzers {
_;
}

modifier whenNoAmountOverdraws() {
_;
}

modifier whenNoZeroAmounts() {
_;
}

/*//////////////////////////////////////////////////////////////////////////
WITHDRAW-MAX-AND-TRANSFER
//////////////////////////////////////////////////////////////////////////*/
Expand Down

0 comments on commit b5d35c4

Please sign in to comment.