From e15c02c07273661b5c11370d2e82cc73f9988b33 Mon Sep 17 00:00:00 2001 From: smol-ninja Date: Mon, 29 Apr 2024 13:08:39 +0100 Subject: [PATCH] Refactoring open ended (#52) * perf: optimize modifiers * refactor: rename streamDebt to streamDebtOf * fix: add override * style: solhint-disable no-console * chore: use return variable in streamDebtOf * test: update streamDebt files --------- Co-authored-by: andreivladbrg --- script/Base.s.sol | 3 +- src/SablierV2OpenEnded.sol | 88 ++++++++++++++++--- src/abstracts/SablierV2OpenEndedState.sol | 26 +++--- src/interfaces/ISablierV2OpenEnded.sol | 18 ++-- .../streamDebtOf.t.sol} | 18 ++-- .../streamDebtOf.tree} | 2 +- 6 files changed, 111 insertions(+), 44 deletions(-) rename test/integration/{stream-debt/streamDebt.t.sol => stream-debt-of/streamDebtOf.t.sol} (50%) rename test/integration/{stream-debt/streamDebt.tree => stream-debt-of/streamDebtOf.tree} (96%) diff --git a/script/Base.s.sol b/script/Base.s.sol index a64b9c2b..3b551b38 100644 --- a/script/Base.s.sol +++ b/script/Base.s.sol @@ -1,4 +1,5 @@ // SPDX-License-Identifier: GPL-3.0-or-later +// solhint-disable no-console pragma solidity >=0.8.22; import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; @@ -57,7 +58,7 @@ abstract contract BaseScript is Script { string memory json = vm.readFile("package.json"); string memory version = json.readString(".version"); string memory create2Salt = string.concat("ChainID ", chainId, ", Version ", version); - console2.log("The CREATE2 salt is \"%s\"", create2Salt); + console2.log("The CREATE2 salt is %s", create2Salt); return bytes32(abi.encodePacked(create2Salt)); } } diff --git a/src/SablierV2OpenEnded.sol b/src/SablierV2OpenEnded.sol index 81afd8dd..7f1ec665 100644 --- a/src/SablierV2OpenEnded.sol +++ b/src/SablierV2OpenEnded.sol @@ -28,6 +28,7 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope view override notCanceled(streamId) + notNull(streamId) returns (uint128 refundableAmount) { refundableAmount = _refundableAmountOf(streamId, uint40(block.timestamp)); @@ -42,25 +43,40 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope view override notCanceled(streamId) + notNull(streamId) returns (uint128 refundableAmount) { refundableAmount = _refundableAmountOf(streamId, time); } /// @inheritdoc ISablierV2OpenEnded - function streamDebt(uint256 streamId) external view notCanceled(streamId) returns (uint128 debt) { + function streamDebtOf(uint256 streamId) + external + view + override + notCanceled(streamId) + notNull(streamId) + returns (uint128 debt) + { uint128 balance = _streams[streamId].balance; uint128 streamedAmount = _streamedAmountOf(streamId, uint40(block.timestamp)); - if (balance >= streamedAmount) { + if (balance < streamedAmount) { + debt = streamedAmount - balance; + } else { return 0; } - - debt = streamedAmount - balance; } /// @inheritdoc ISablierV2OpenEnded - function streamedAmountOf(uint256 streamId) external view notCanceled(streamId) returns (uint128 streamedAmount) { + function streamedAmountOf(uint256 streamId) + external + view + override + notCanceled(streamId) + notNull(streamId) + returns (uint128 streamedAmount) + { streamedAmount = _streamedAmountOf(streamId, uint40(block.timestamp)); } @@ -71,7 +87,9 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope ) external view + override notCanceled(streamId) + notNull(streamId) returns (uint128 streamedAmount) { streamedAmount = _streamedAmountOf(streamId, time); @@ -81,7 +99,9 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope function withdrawableAmountOf(uint256 streamId) external view + override notCanceled(streamId) + notNull(streamId) returns (uint128 withdrawableAmount) { withdrawableAmount = _withdrawableAmountOf(streamId, uint40(block.timestamp)); @@ -94,7 +114,9 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope ) external view + override notCanceled(streamId) + notNull(streamId) returns (uint128 withdrawableAmount) { withdrawableAmount = _withdrawableAmountOf(streamId, time); @@ -110,8 +132,10 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope uint128 newRatePerSecond ) external + override noDelegateCall notCanceled(streamId) + notNull(streamId) onlySender(streamId) { // Effects and Interactions: adjust the stream. @@ -119,7 +143,14 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope } /// @inheritdoc ISablierV2OpenEnded - function cancel(uint256 streamId) public noDelegateCall notCanceled(streamId) onlySender(streamId) { + function cancel(uint256 streamId) + public + override + noDelegateCall + notCanceled(streamId) + notNull(streamId) + onlySender(streamId) + { _cancel(streamId); } @@ -141,6 +172,7 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope IERC20 asset ) external + override returns (uint256 streamId) { // Checks, Effects and Interactions: create the stream. @@ -156,6 +188,7 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope uint128 depositAmount ) external + override returns (uint256 streamId) { // Checks, Effects and Interactions: create the stream. @@ -173,6 +206,7 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope IERC20 asset ) public + override returns (uint256[] memory streamIds) { uint256 recipientsCount = recipients.length; @@ -202,6 +236,7 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope uint128[] calldata depositAmounts ) external + override returns (uint256[] memory streamIds) { streamIds = new uint256[](recipients.length); @@ -211,13 +246,22 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope } /// @inheritdoc ISablierV2OpenEnded - function deposit(uint256 streamId, uint128 depositAmount) external noDelegateCall notCanceled(streamId) { + function deposit( + uint256 streamId, + uint128 depositAmount + ) + external + override + noDelegateCall + notCanceled(streamId) + notNull(streamId) + { // Checks, Effects and Interactions: deposit on stream. _deposit(streamId, depositAmount); } /// @inheritdoc ISablierV2OpenEnded - function depositMultiple(uint256[] memory streamIds, uint128[] calldata amounts) public noDelegateCall { + function depositMultiple(uint256[] memory streamIds, uint128[] calldata amounts) public override noDelegateCall { uint256 streamIdsCount = streamIds.length; uint256 depositAmountsCount = amounts.length; @@ -238,13 +282,20 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope } /// @inheritdoc ISablierV2OpenEnded - function restartStream(uint256 streamId, uint128 ratePerSecond) external { + function restartStream(uint256 streamId, uint128 ratePerSecond) external override { // Checks, Effects and Interactions: restart the stream. _restartStream(streamId, ratePerSecond); } /// @inheritdoc ISablierV2OpenEnded - function restartStreamAndDeposit(uint256 streamId, uint128 ratePerSecond, uint128 depositAmount) external { + function restartStreamAndDeposit( + uint256 streamId, + uint128 ratePerSecond, + uint128 depositAmount + ) + external + override + { // Checks, Effects and Interactions: restart the stream. _restartStream(streamId, ratePerSecond); @@ -258,8 +309,10 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope uint128 refundAmount ) external + override noDelegateCall notCanceled(streamId) + notNull(streamId) onlySender(streamId) { // Checks, Effects and Interactions: make the refund. @@ -267,7 +320,7 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope } /// @inheritdoc ISablierV2OpenEnded - function withdraw(uint256 streamId, address to, uint40 time) external { + function withdraw(uint256 streamId, address to, uint40 time) external override { // Checks, Effects and Interactions: make the withdrawal. _withdraw(streamId, to, time); } @@ -289,7 +342,7 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope } /// @inheritdoc ISablierV2OpenEnded - function withdrawMax(uint256 streamId, address to) external { + function withdrawMax(uint256 streamId, address to) external override { // Checks, Effects and Interactions: make the withdrawal. _withdraw(streamId, to, uint40(block.timestamp)); } @@ -632,7 +685,16 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope } /// @dev See the documentation for the user-facing functions that call this internal function. - function _withdraw(uint256 streamId, address to, uint40 time) internal noDelegateCall notCanceled(streamId) { + function _withdraw( + uint256 streamId, + address to, + uint40 time + ) + internal + noDelegateCall + notCanceled(streamId) + notNull(streamId) + { // Check: the withdrawal address is not zero. if (to == address(0)) { revert Errors.SablierV2OpenEnded_WithdrawToZeroAddress(); diff --git a/src/abstracts/SablierV2OpenEndedState.sol b/src/abstracts/SablierV2OpenEndedState.sol index d1b0e0b2..6464fcf9 100644 --- a/src/abstracts/SablierV2OpenEndedState.sol +++ b/src/abstracts/SablierV2OpenEndedState.sol @@ -34,7 +34,7 @@ abstract contract SablierV2OpenEndedState is ISablierV2OpenEndedState { /// @dev Checks that `streamId` does not reference a canceled stream. modifier notCanceled(uint256 streamId) { - if (isCanceled(streamId)) { + if (_streams[streamId].isCanceled) { revert Errors.SablierV2OpenEnded_StreamCanceled(streamId); } _; @@ -50,7 +50,7 @@ abstract contract SablierV2OpenEndedState is ISablierV2OpenEndedState { /// @dev Checks the `msg.sender` is the stream's sender. modifier onlySender(uint256 streamId) { - if (!_isCallerStreamSender(streamId)) { + if (msg.sender != _streams[streamId].sender) { revert Errors.SablierV2OpenEnded_Unauthorized(streamId, msg.sender); } _; @@ -109,12 +109,18 @@ abstract contract SablierV2OpenEndedState is ISablierV2OpenEndedState { } /// @inheritdoc ISablierV2OpenEndedState - function getSender(uint256 streamId) external view notNull(streamId) returns (address sender) { + function getSender(uint256 streamId) external view override notNull(streamId) returns (address sender) { sender = _streams[streamId].sender; } /// @inheritdoc ISablierV2OpenEndedState - function getStream(uint256 streamId) external view notNull(streamId) returns (OpenEnded.Stream memory stream) { + function getStream(uint256 streamId) + external + view + override + notNull(streamId) + returns (OpenEnded.Stream memory stream) + { stream = _streams[streamId]; } @@ -124,17 +130,7 @@ abstract contract SablierV2OpenEndedState is ISablierV2OpenEndedState { } /// @inheritdoc ISablierV2OpenEndedState - function isStream(uint256 streamId) public view returns (bool result) { + function isStream(uint256 streamId) public view override returns (bool result) { result = _streams[streamId].isStream; } - - /*////////////////////////////////////////////////////////////////////////// - INTERNAL CONSTANT FUNCTIONS - //////////////////////////////////////////////////////////////////////////*/ - - /// @notice Checks whether `msg.sender` is the stream's sender. - /// @param streamId The stream ID for the query. - function _isCallerStreamSender(uint256 streamId) internal view returns (bool) { - return msg.sender == _streams[streamId].sender; - } } diff --git a/src/interfaces/ISablierV2OpenEnded.sol b/src/interfaces/ISablierV2OpenEnded.sol index 84538ad1..1035c4bd 100644 --- a/src/interfaces/ISablierV2OpenEnded.sol +++ b/src/interfaces/ISablierV2OpenEnded.sol @@ -99,42 +99,44 @@ interface ISablierV2OpenEnded is ISablierV2OpenEndedState { //////////////////////////////////////////////////////////////////////////*/ /// @notice Calculates the amount that the sender can refund from stream, denoted in 18 decimals. - /// @dev Reverts if `streamId` references a canceled stream. + /// @dev Reverts if `streamId` references a canceled or a null stream. /// @param streamId The stream ID for the query. + /// @return refundableAmount The amount that the sender can refund. function refundableAmountOf(uint256 streamId) external view returns (uint128 refundableAmount); /// @notice Calculates the amount that the sender can refund from stream at `time`, denoted in 18 decimals. - /// @dev Reverts if `streamId` references a canceled stream. + /// @dev Reverts if `streamId` references a canceled or a null stream. /// @param streamId The stream ID for the query. /// @param time The Unix timestamp for the streamed amount calculation. + /// @return refundableAmount The amount that the sender can refund. function refundableAmountOf(uint256 streamId, uint40 time) external view returns (uint128 refundableAmount); /// @notice Calculates the amount that the sender owes on the stream, i.e. if more assets have been streamed than /// its balance, denoted in 18 decimals. If there is no debt, it will return zero. - /// @dev Reverts if `streamId` references a canceled stream. + /// @dev Reverts if `streamId` references a canceled or a null stream. /// @param streamId The stream ID for the query. - function streamDebt(uint256 streamId) external view returns (uint128 debt); + function streamDebtOf(uint256 streamId) external view returns (uint128 debt); /// @notice Calculates the amount streamed to the recipient from the last time update to the current time, /// denoted in 18 decimals. - /// @dev Reverts if `streamId` references a canceled stream. + /// @dev Reverts if `streamId` references a canceled or a null stream. /// @param streamId The stream ID for the query. function streamedAmountOf(uint256 streamId) external view returns (uint128 streamedAmount); /// @notice Calculates the amount streamed to the recipient from the last time update to `time` passed as parameter, /// denoted in 18 decimals. - /// @dev Reverts if `streamId` references a canceled stream. + /// @dev Reverts if `streamId` references a canceled or a null stream. /// @param streamId The stream ID for the query. /// @param time The Unix timestamp for the streamed amount calculation. function streamedAmountOf(uint256 streamId, uint40 time) external view returns (uint128 streamedAmount); /// @notice Calculates the amount that the recipient can withdraw from the stream, denoted in 18 decimals. - /// @dev Reverts if `streamId` references a canceled stream. + /// @dev Reverts if `streamId` references a canceled or a null stream. /// @param streamId The stream ID for the query. function withdrawableAmountOf(uint256 streamId) external view returns (uint128 withdrawableAmount); /// @notice Calculates the amount that the recipient can withdraw from the stream at `time`, denoted in 18 decimals. - /// @dev Reverts if `streamId` references a canceled stream. + /// @dev Reverts if `streamId` references a canceled or a null stream. /// @param streamId The stream ID for the query. /// @param time The Unix timestamp for the streamed amount calculation. function withdrawableAmountOf(uint256 streamId, uint40 time) external view returns (uint128 withdrawableAmount); diff --git a/test/integration/stream-debt/streamDebt.t.sol b/test/integration/stream-debt-of/streamDebtOf.t.sol similarity index 50% rename from test/integration/stream-debt/streamDebt.t.sol rename to test/integration/stream-debt-of/streamDebtOf.t.sol index d4a28a7b..a0ff6bd3 100644 --- a/test/integration/stream-debt/streamDebt.t.sol +++ b/test/integration/stream-debt-of/streamDebtOf.t.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity >=0.8.22; +import { console2 } from "forge-std/src/console2.sol"; import { Integration_Test } from "../Integration.t.sol"; contract StreamDebt_Integration_Test is Integration_Test { @@ -10,23 +11,28 @@ contract StreamDebt_Integration_Test is Integration_Test { function test_RevertGiven_Null() external { expectRevertNull(); - openEnded.streamDebt(nullStreamId); + openEnded.streamDebtOf(nullStreamId); } function test_RevertGiven_Canceled() external givenNotNull { expectRevertCanceled(); - openEnded.streamDebt(defaultStreamId); + openEnded.streamDebtOf(defaultStreamId); } - function test_StreamDebt_BalanceGreaterThanOrEqualStreamedAmount() external givenNotNull givenNotCanceled { + function test_StreamDebtOf_BalanceGreaterThanOrEqualStreamedAmount() external givenNotNull givenNotCanceled { defaultDeposit(); - uint128 streamDebt = openEnded.streamDebt(defaultStreamId); + uint128 streamDebt = openEnded.streamDebtOf(defaultStreamId); + assertEq(streamDebt, 0, "stream debt"); } - function test_StreamDebt() external givenNotNull givenNotCanceled { + function test_streamDebtOf() external givenNotNull givenNotCanceled { vm.warp({ newTimestamp: WARP_ONE_MONTH }); - uint128 streamDebt = openEnded.streamDebt(defaultStreamId); + uint128 streamDebt = openEnded.streamDebtOf(defaultStreamId); + + console2.log("streamedAmountOf %s", openEnded.streamedAmountOf(defaultStreamId)); + console2.log("balance %s", openEnded.getBalance(defaultStreamId)); + assertEq(streamDebt, ONE_MONTH_STREAMED_AMOUNT, "stream debt"); } } diff --git a/test/integration/stream-debt/streamDebt.tree b/test/integration/stream-debt-of/streamDebtOf.tree similarity index 96% rename from test/integration/stream-debt/streamDebt.tree rename to test/integration/stream-debt-of/streamDebtOf.tree index 18102d58..db21f29e 100644 --- a/test/integration/stream-debt/streamDebt.tree +++ b/test/integration/stream-debt-of/streamDebtOf.tree @@ -1,4 +1,4 @@ -streamDebt.t.sol +streamDebtOf.t.sol ├── given the id references a null stream │ └── it should revert └── given the id does not reference a null stream