From 1056cafd97b1e1cf9440b98df04566b35ad32c69 Mon Sep 17 00:00:00 2001 From: andreivladbrg Date: Wed, 25 Sep 2024 19:21:56 +0300 Subject: [PATCH 1/4] perf: don't update st if withdraw amount is less than sd --- src/SablierFlow.sol | 17 +++++++++++------ tests/fork/Flow.t.sol | 3 ++- .../concrete/withdraw-max/withdrawMax.t.sol | 8 +++++--- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/SablierFlow.sol b/src/SablierFlow.sol index 7b4dcad1..fda5e947 100644 --- a/src/SablierFlow.sol +++ b/src/SablierFlow.sol @@ -813,16 +813,21 @@ contract SablierFlow is // Safe to use unchecked, the balance or total debt cannot be less than `amount` at this point. unchecked { + if (amount <= _streams[streamId].snapshotDebt) { + // If the amount is not greater than the snapshot debt, reduce it from the snapshot debt and leave the + // snapshot time unchanged. + _streams[streamId].snapshotDebt -= amount; + } else { + _streams[streamId].snapshotDebt = totalDebt - amount; + + // Effect: update the stream time. + _streams[streamId].snapshotTime = uint40(block.timestamp); + } + // Effect: update the stream balance. _streams[streamId].balance -= amount; - - // Effect: update the snapshot debt. - _streams[streamId].snapshotDebt = totalDebt - amount; } - // Effect: update the stream time. - _streams[streamId].snapshotTime = uint40(block.timestamp); - // Load the variables in memory. IERC20 token = _streams[streamId].token; UD60x18 protocolFee = protocolFee[token]; diff --git a/tests/fork/Flow.t.sol b/tests/fork/Flow.t.sol index a08ddfdc..fb3a3a4f 100644 --- a/tests/fork/Flow.t.sol +++ b/tests/fork/Flow.t.sol @@ -598,7 +598,8 @@ contract Flow_Fork_Test is Fork_Test { uint256 initialTokenBalance = token.balanceOf(address(flow)); uint128 totalDebt = flow.totalDebtOf(streamId); - vars.expectedSnapshotTime = getBlockTimestamp(); + vars.expectedSnapshotTime = + withdrawAmount > flow.getSnapshotDebt(streamId) ? flow.getSnapshotTime(streamId) : getBlockTimestamp(); (, address caller,) = vm.readCallers(); address recipient = flow.getRecipient(streamId); diff --git a/tests/integration/concrete/withdraw-max/withdrawMax.t.sol b/tests/integration/concrete/withdraw-max/withdrawMax.t.sol index 4f641efa..5f75ba6c 100644 --- a/tests/integration/concrete/withdraw-max/withdrawMax.t.sol +++ b/tests/integration/concrete/withdraw-max/withdrawMax.t.sol @@ -71,9 +71,11 @@ contract WithdrawMax_Integration_Concrete_Test is Integration_Test { uint128 actualSnapshotDebt = flow.getSnapshotDebt(defaultStreamId); assertEq(actualSnapshotDebt, 0, "snapshot debt"); - // It should update snapshot time. - uint128 actualSnapshotTime = flow.getSnapshotTime(defaultStreamId); - assertEq(actualSnapshotTime, getBlockTimestamp(), "snapshot time"); + if (!flow.isPaused(defaultStreamId)) { + // It should update snapshot time. + uint128 actualSnapshotTime = flow.getSnapshotTime(defaultStreamId); + assertEq(actualSnapshotTime, getBlockTimestamp(), "snapshot time"); + } // It should return the actual withdrawn amount. assertEq(actualWithdrawnAmount, expectedWithdrawAmount, "withdrawn amount"); From 0287de890c8fc4f9c0685ec1f4e5afc88bbc33d6 Mon Sep 17 00:00:00 2001 From: andreivladbrg Date: Thu, 26 Sep 2024 12:01:50 +0300 Subject: [PATCH 2/4] test(fork): correct operator --- tests/fork/Flow.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fork/Flow.t.sol b/tests/fork/Flow.t.sol index fb3a3a4f..f0e09c3a 100644 --- a/tests/fork/Flow.t.sol +++ b/tests/fork/Flow.t.sol @@ -599,7 +599,7 @@ contract Flow_Fork_Test is Fork_Test { uint128 totalDebt = flow.totalDebtOf(streamId); vars.expectedSnapshotTime = - withdrawAmount > flow.getSnapshotDebt(streamId) ? flow.getSnapshotTime(streamId) : getBlockTimestamp(); + withdrawAmount <= flow.getSnapshotDebt(streamId) ? flow.getSnapshotTime(streamId) : getBlockTimestamp(); (, address caller,) = vm.readCallers(); address recipient = flow.getRecipient(streamId); From f8d7f34189bbaeb549df00745012a3000401d667 Mon Sep 17 00:00:00 2001 From: smol-ninja Date: Thu, 26 Sep 2024 14:49:05 +0100 Subject: [PATCH 3/4] docs: polish comments --- src/SablierFlow.sol | 11 +++++++---- src/interfaces/ISablierFlow.sol | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/SablierFlow.sol b/src/SablierFlow.sol index fda5e947..abaef60f 100644 --- a/src/SablierFlow.sol +++ b/src/SablierFlow.sol @@ -811,13 +811,16 @@ contract SablierFlow is revert Errors.SablierFlow_Overdraw(streamId, amount, withdrawableAmount); } - // Safe to use unchecked, the balance or total debt cannot be less than `amount` at this point. + // Safe to use unchecked, `amount` cannot be greater than the balance or total debt at this point. unchecked { + // If the amount is less than the snapshot debt, reduce it from the snapshot debt and leave the snapshot + // time unchanged. if (amount <= _streams[streamId].snapshotDebt) { - // If the amount is not greater than the snapshot debt, reduce it from the snapshot debt and leave the - // snapshot time unchanged. _streams[streamId].snapshotDebt -= amount; - } else { + } + // Else reduce the amount from the ongoing debt by setting snapshot time to `block.timestamp` and set the + // snapshot debt to the remaining total debt. + else { _streams[streamId].snapshotDebt = totalDebt - amount; // Effect: update the stream time. diff --git a/src/interfaces/ISablierFlow.sol b/src/interfaces/ISablierFlow.sol index 5570aaf4..9397a08d 100644 --- a/src/interfaces/ISablierFlow.sol +++ b/src/interfaces/ISablierFlow.sol @@ -389,7 +389,7 @@ interface ISablierFlow is /// @dev Emits a {Transfer} and {WithdrawFromFlowStream} event. /// /// Notes: - /// - It sets the snapshot time to the `block.timestamp`. + /// - It sets the snapshot time to the `block.timestamp` if `amount` is greater than snapshot debt. /// - A protocol fee may be charged on the withdrawn amount if the protocol fee is enabled for the streaming token. /// /// Requirements: From 0a4af2d1e2479de5a7c15213251517171ddab212 Mon Sep 17 00:00:00 2001 From: andreivladbrg Date: Mon, 30 Sep 2024 02:30:32 +0300 Subject: [PATCH 4/4] test: add when amount not exceed snapshot debt tree branch --- .../concrete/withdraw/withdraw.t.sol | 79 +++++++++++++++---- .../concrete/withdraw/withdraw.tree | 32 ++++---- 2 files changed, 80 insertions(+), 31 deletions(-) diff --git a/tests/integration/concrete/withdraw/withdraw.t.sol b/tests/integration/concrete/withdraw/withdraw.t.sol index 3813af2c..3c84bf9a 100644 --- a/tests/integration/concrete/withdraw/withdraw.t.sol +++ b/tests/integration/concrete/withdraw/withdraw.t.sol @@ -150,6 +150,33 @@ contract Withdraw_Integration_Concrete_Test is Integration_Test { }); } + function test_WhenAmountNotExceedSnapshotDebt() + external + whenNoDelegateCall + givenNotNull + whenAmountNotZero + whenWithdrawalAddressNotZero + whenWithdrawalAddressOwner + whenAmountNotOverdraw + { + // It should not update snapshot time + // It should make the withdrawal + resetPrank({ msgSender: users.sender }); + flow.pause(defaultStreamId); + + vm.warp({ newTimestamp: getBlockTimestamp() + 1 }); + + resetPrank({ msgSender: users.recipient }); + // It should make the withdrawal. + _test_Withdraw({ + streamId: defaultStreamId, + to: users.recipient, + depositAmount: DEPOSIT_AMOUNT_6D, + feeAmount: 0, + withdrawAmount: WITHDRAW_AMOUNT_6D + }); + } + function test_GivenProtocolFeeNotZero() external whenNoDelegateCall @@ -235,6 +262,19 @@ contract Withdraw_Integration_Concrete_Test is Integration_Test { }); } + struct Vars { + IERC20 token; + uint40 previousSnapshotTime; + uint128 previousTotalDebt; + uint256 previousAggregateAmount; + uint128 expectedProtocolRevenue; + uint256 initialTokenBalance; + uint40 expectedSnapshotTime; + uint128 expectedTotalDebt; + uint128 expectedStreamBalance; + uint256 expectedTokenBalance; + } + function _test_Withdraw( uint256 streamId, address to, @@ -244,21 +284,23 @@ contract Withdraw_Integration_Concrete_Test is Integration_Test { ) private { - IERC20 token = flow.getToken(streamId); - uint128 previousTotalDebt = flow.totalDebtOf(streamId); - uint256 previousAggregateAmount = flow.aggregateBalance(token); + Vars memory vars; + vars.token = flow.getToken(streamId); + vars.previousSnapshotTime = flow.getSnapshotTime(streamId); + vars.previousTotalDebt = flow.totalDebtOf(streamId); + vars.previousAggregateAmount = flow.aggregateBalance(vars.token); - uint128 expectedProtocolRevenue = flow.protocolRevenue(token) + feeAmount; + vars.expectedProtocolRevenue = flow.protocolRevenue(vars.token) + feeAmount; // It should emit 1 {Transfer}, 1 {WithdrawFromFlowStream} and 1 {MetadataUpdated} events. - vm.expectEmit({ emitter: address(token) }); + vm.expectEmit({ emitter: address(vars.token) }); emit IERC20.Transfer({ from: address(flow), to: to, value: withdrawAmount - feeAmount }); vm.expectEmit({ emitter: address(flow) }); emit WithdrawFromFlowStream({ streamId: streamId, to: to, - token: token, + token: vars.token, caller: users.recipient, protocolFeeAmount: feeAmount, withdrawAmount: withdrawAmount - feeAmount @@ -268,33 +310,36 @@ contract Withdraw_Integration_Concrete_Test is Integration_Test { emit MetadataUpdate({ _tokenId: streamId }); // It should perform the ERC-20 transfer. - expectCallToTransfer({ token: token, to: to, amount: withdrawAmount - feeAmount }); + expectCallToTransfer({ token: vars.token, to: to, amount: withdrawAmount - feeAmount }); - uint256 initialTokenBalance = token.balanceOf(address(flow)); + vars.initialTokenBalance = vars.token.balanceOf(address(flow)); flow.withdraw({ streamId: streamId, to: to, amount: withdrawAmount }); // Assert the protocol revenue. - assertEq(flow.protocolRevenue(token), expectedProtocolRevenue, "protocol revenue"); + assertEq(flow.protocolRevenue(vars.token), vars.expectedProtocolRevenue, "protocol revenue"); // It should update snapshot time. - assertEq(flow.getSnapshotTime(streamId), getBlockTimestamp(), "snapshot time"); + vars.expectedSnapshotTime = flow.isPaused(streamId) ? vars.previousSnapshotTime : getBlockTimestamp(); + assertEq(flow.getSnapshotTime(streamId), vars.expectedSnapshotTime, "snapshot time"); // It should decrease the total debt by the withdrawn value and fee amount. - uint128 expectedTotalDebt = previousTotalDebt - withdrawAmount; - assertEq(flow.totalDebtOf(streamId), expectedTotalDebt, "total debt"); + vars.expectedTotalDebt = vars.previousTotalDebt - withdrawAmount; + assertEq(flow.totalDebtOf(streamId), vars.expectedTotalDebt, "total debt"); // It should reduce the stream balance by the withdrawn value and fee amount. - uint128 expectedStreamBalance = depositAmount - withdrawAmount; - assertEq(flow.getBalance(streamId), expectedStreamBalance, "stream balance"); + vars.expectedStreamBalance = depositAmount - withdrawAmount; + assertEq(flow.getBalance(streamId), vars.expectedStreamBalance, "stream balance"); // It should reduce the token balance of stream. - uint256 expectedTokenBalance = initialTokenBalance - (withdrawAmount - feeAmount); - assertEq(token.balanceOf(address(flow)), expectedTokenBalance, "token balance"); + vars.expectedTokenBalance = vars.initialTokenBalance - (withdrawAmount - feeAmount); + assertEq(vars.token.balanceOf(address(flow)), vars.expectedTokenBalance, "token balance"); // It should decrease the aggregate amount. assertEq( - flow.aggregateBalance(token), previousAggregateAmount - (withdrawAmount - feeAmount), "aggregate amount" + flow.aggregateBalance(vars.token), + vars.previousAggregateAmount - (withdrawAmount - feeAmount), + "aggregate amount" ); } } diff --git a/tests/integration/concrete/withdraw/withdraw.tree b/tests/integration/concrete/withdraw/withdraw.tree index 8cd6e2ee..8b9601ed 100644 --- a/tests/integration/concrete/withdraw/withdraw.tree +++ b/tests/integration/concrete/withdraw/withdraw.tree @@ -29,17 +29,21 @@ Withdraw_Integration_Concrete_Test │ ├── it should update snapshot debt │ └── it should make the withdrawal └── when amount equal total debt - ├── given protocol fee not zero - │ ├── it should update the protocol revenue - │ └── it should withdraw the net amount - └── given protocol fee zero - ├── given token has 18 decimals - │ └── it should make the withdrawal - └── given token not have 18 decimals - ├── it should withdraw the withdrawable amount - ├── it should reduce the stream balance by the withdrawn amount - ├── it should reduce the aggregate amount by the withdrawn amount - ├── it should update snapshot debt to zero - ├── it should update snapshot time - ├── it should emit 1 {Transfer}, 1 {WithdrawFromFlowStream} and 1 {MetadataUpdated} events - └── it should return the withdrawn amount + ├── when amount not exceed snapshot debt + │ ├── it should not update snapshot time + │ └── it should make the withdrawal + └── when amount exceed snapshot debt + ├── given protocol fee not zero + │ ├── it should update the protocol revenue + │ └── it should withdraw the net amount + └── given protocol fee zero + ├── given token has 18 decimals + │ └── it should make the withdrawal + └── given token not have 18 decimals + ├── it should withdraw the withdrawable amount + ├── it should reduce the stream balance by the withdrawn amount + ├── it should reduce the aggregate amount by the withdrawn amount + ├── it should update snapshot debt to zero + ├── it should update snapshot time + ├── it should emit 1 {Transfer}, 1 {WithdrawFromFlowStream} and 1 {MetadataUpdated} events + └── it should return the withdrawn amount