From b69d326390657e6cc365fa86eed413e137aaded0 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Wed, 18 Oct 2023 12:01:47 +0100 Subject: [PATCH] refactor: add "recipient" in cancel event docs: refine explanatory comments refactor: reorder parameters in events --- src/SablierV2LockupDynamic.sol | 10 +++++----- src/SablierV2LockupLinear.sol | 10 +++++----- src/interfaces/ISablierV2Lockup.sol | 8 +++++--- test/fork/LockupDynamic.t.sol | 8 +++++--- test/fork/LockupLinear.t.sol | 8 +++++--- .../lockup/cancel-multiple/cancelMultiple.t.sol | 6 ++++-- test/integration/concrete/lockup/cancel/cancel.t.sol | 4 ++-- .../lockup/withdraw-multiple/withdrawMultiple.t.sol | 12 ++++++------ .../concrete/lockup/withdraw/withdraw.t.sol | 8 ++++---- test/integration/fuzz/lockup/cancel.t.sol | 2 +- test/integration/fuzz/lockup/cancelMultiple.t.sol | 2 ++ test/integration/fuzz/lockup/withdraw.t.sol | 4 ++-- test/integration/fuzz/lockup/withdrawMax.t.sol | 8 ++++---- .../fuzz/lockup/withdrawMaxAndTransfer.t.sol | 4 ++-- test/integration/fuzz/lockup/withdrawMultiple.t.sol | 4 ++-- test/utils/Events.sol | 5 +++-- 16 files changed, 57 insertions(+), 46 deletions(-) diff --git a/src/SablierV2LockupDynamic.sol b/src/SablierV2LockupDynamic.sol index 1ade6b5d1..3b96ba5e0 100644 --- a/src/SablierV2LockupDynamic.sol +++ b/src/SablierV2LockupDynamic.sol @@ -506,14 +506,14 @@ contract SablierV2LockupDynamic is address sender = _streams[streamId].sender; address recipient = _ownerOf(streamId); + // Retrieve the ERC-20 asset from storage. IERC20 asset = _streams[streamId].asset; // Interactions: refund the sender. asset.safeTransfer({ to: sender, value: senderAmount }); - // Interactions: if the recipient is a contract, try to invoke the cancel - // hook on the recipient without reverting if the hook is not implemented, and without bubbling up any - // potential revert. + // Interactions: if the recipient is a contract, try to invoke the cancel hook on the recipient without + // reverting if the hook is not implemented, and without bubbling up any potential revert. if (recipient.code.length > 0) { try ISablierV2LockupRecipient(recipient).onStreamCanceled({ streamId: streamId, @@ -524,7 +524,7 @@ contract SablierV2LockupDynamic is } // Log the cancellation. - emit ISablierV2Lockup.CancelLockupStream(streamId, sender, asset, senderAmount, recipientAmount); + emit ISablierV2Lockup.CancelLockupStream(streamId, sender, recipient, asset, senderAmount, recipientAmount); } /// @dev See the documentation for the user-facing functions that call this internal function. @@ -642,6 +642,6 @@ contract SablierV2LockupDynamic is asset.safeTransfer({ to: to, value: amount }); // Log the withdrawal. - emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, amount, asset); + emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, asset, amount); } } diff --git a/src/SablierV2LockupLinear.sol b/src/SablierV2LockupLinear.sol index 288f7437d..f97124fd7 100644 --- a/src/SablierV2LockupLinear.sol +++ b/src/SablierV2LockupLinear.sol @@ -421,14 +421,14 @@ contract SablierV2LockupLinear is address sender = _streams[streamId].sender; address recipient = _ownerOf(streamId); + // Retrieve the ERC-20 asset from storage. IERC20 asset = _streams[streamId].asset; // Interactions: refund the sender. asset.safeTransfer({ to: sender, value: senderAmount }); - // Interactions: if the recipient is a contract, try to invoke the cancel - // hook on the recipient without reverting if the hook is not implemented, and without bubbling up any - // potential revert. + // Interactions: if the recipient is a contract, try to invoke the cancel hook on the recipient without + // reverting if the hook is not implemented, and without bubbling up any potential revert. if (recipient.code.length > 0) { try ISablierV2LockupRecipient(recipient).onStreamCanceled({ streamId: streamId, @@ -439,7 +439,7 @@ contract SablierV2LockupLinear is } // Log the cancellation. - emit ISablierV2Lockup.CancelLockupStream(streamId, sender, asset, senderAmount, recipientAmount); + emit ISablierV2Lockup.CancelLockupStream(streamId, sender, recipient, asset, senderAmount, recipientAmount); } /// @dev See the documentation for the user-facing functions that call this internal function. @@ -548,6 +548,6 @@ contract SablierV2LockupLinear is asset.safeTransfer({ to: to, value: amount }); // Log the withdrawal. - emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, amount, asset); + emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, asset, amount); } } diff --git a/src/interfaces/ISablierV2Lockup.sol b/src/interfaces/ISablierV2Lockup.sol index 1a1c212af..3bb304a3c 100644 --- a/src/interfaces/ISablierV2Lockup.sol +++ b/src/interfaces/ISablierV2Lockup.sol @@ -21,14 +21,16 @@ interface ISablierV2Lockup is /// @notice Emitted when a stream is canceled. /// @param streamId The id of the stream. /// @param sender The address of the stream's sender. + /// @param recipient The address of the stream's recipient. /// @param asset The contract address of the ERC-20 asset used for streaming. /// @param senderAmount The amount of assets refunded to the stream's sender, denoted in units of the asset's /// decimals. /// @param recipientAmount The amount of assets left for the stream's recipient to withdraw, denoted in units of the /// asset's decimals. event CancelLockupStream( - uint256 indexed streamId, + uint256 streamId, address indexed sender, + address indexed recipient, IERC20 indexed asset, uint128 senderAmount, uint128 recipientAmount @@ -49,9 +51,9 @@ interface ISablierV2Lockup is /// @notice Emitted when assets are withdrawn from a stream. /// @param streamId The id of the stream. /// @param to The address that has received the withdrawn assets. - /// @param amount The amount of assets withdrawn, denoted in units of the asset's decimals. /// @param asset The contract address of the ERC-20 asset used for streaming. - event WithdrawFromLockupStream(uint256 indexed streamId, address indexed to, uint128 amount, IERC20 indexed asset); + /// @param amount The amount of assets withdrawn, denoted in units of the asset's decimals. + event WithdrawFromLockupStream(uint256 indexed streamId, address indexed to, IERC20 indexed asset, uint128 amount); /*////////////////////////////////////////////////////////////////////////// CONSTANT FUNCTIONS diff --git a/test/fork/LockupDynamic.t.sol b/test/fork/LockupDynamic.t.sol index a162415c1..7a02372ba 100644 --- a/test/fork/LockupDynamic.t.sol +++ b/test/fork/LockupDynamic.t.sol @@ -288,8 +288,8 @@ abstract contract LockupDynamic_Fork_Test is Fork_Test { emit WithdrawFromLockupStream({ streamId: vars.streamId, to: params.recipient, - amount: params.withdrawAmount, - asset: ASSET + asset: ASSET, + amount: params.withdrawAmount }); vm.expectEmit({ emitter: address(lockupDynamic) }); emit MetadataUpdate({ _tokenId: vars.streamId }); @@ -351,7 +351,9 @@ abstract contract LockupDynamic_Fork_Test is Fork_Test { vm.expectEmit({ emitter: address(lockupDynamic) }); vars.senderAmount = lockupDynamic.refundableAmountOf(vars.streamId); vars.recipientAmount = lockupDynamic.withdrawableAmountOf(vars.streamId); - emit CancelLockupStream(vars.streamId, params.sender, ASSET, vars.senderAmount, vars.recipientAmount); + emit CancelLockupStream( + vars.streamId, params.sender, params.recipient, ASSET, vars.senderAmount, vars.recipientAmount + ); vm.expectEmit({ emitter: address(lockupDynamic) }); emit MetadataUpdate({ _tokenId: vars.streamId }); diff --git a/test/fork/LockupLinear.t.sol b/test/fork/LockupLinear.t.sol index ca7f7bbbf..bd6016c64 100644 --- a/test/fork/LockupLinear.t.sol +++ b/test/fork/LockupLinear.t.sol @@ -274,8 +274,8 @@ abstract contract LockupLinear_Fork_Test is Fork_Test { emit WithdrawFromLockupStream({ streamId: vars.streamId, to: params.recipient, - amount: params.withdrawAmount, - asset: ASSET + asset: ASSET, + amount: params.withdrawAmount }); vm.expectEmit({ emitter: address(lockupLinear) }); emit MetadataUpdate({ _tokenId: vars.streamId }); @@ -335,7 +335,9 @@ abstract contract LockupLinear_Fork_Test is Fork_Test { vm.expectEmit({ emitter: address(lockupLinear) }); vars.senderAmount = lockupLinear.refundableAmountOf(vars.streamId); vars.recipientAmount = lockupLinear.withdrawableAmountOf(vars.streamId); - emit CancelLockupStream(vars.streamId, params.sender, ASSET, vars.senderAmount, vars.recipientAmount); + emit CancelLockupStream( + vars.streamId, params.sender, params.recipient, ASSET, vars.senderAmount, vars.recipientAmount + ); vm.expectEmit({ emitter: address(lockupLinear) }); emit MetadataUpdate({ _tokenId: vars.streamId }); diff --git a/test/integration/concrete/lockup/cancel-multiple/cancelMultiple.t.sol b/test/integration/concrete/lockup/cancel-multiple/cancelMultiple.t.sol index c3b39d776..4aab9c7c5 100644 --- a/test/integration/concrete/lockup/cancel-multiple/cancelMultiple.t.sol +++ b/test/integration/concrete/lockup/cancel-multiple/cancelMultiple.t.sol @@ -80,7 +80,7 @@ abstract contract CancelMultiple_Integration_Concrete_Test is givenAllStreamsWarm whenCallerUnauthorized { - // Make the recipient the caller in this test. + // Make the Recipient the caller in this test. changePrank({ msgSender: users.recipient }); // Run the test. @@ -119,7 +119,7 @@ abstract contract CancelMultiple_Integration_Concrete_Test is givenAllStreamsWarm whenCallerUnauthorized { - // Make the recipient the caller in this test. + // Make the Recipient the caller in this test. changePrank({ msgSender: users.recipient }); // Run the test. @@ -181,6 +181,7 @@ abstract contract CancelMultiple_Integration_Concrete_Test is emit CancelLockupStream({ streamId: testStreamIds[0], sender: users.sender, + recipient: users.recipient, asset: dai, senderAmount: senderAmount0, recipientAmount: defaults.DEPOSIT_AMOUNT() - senderAmount0 @@ -189,6 +190,7 @@ abstract contract CancelMultiple_Integration_Concrete_Test is emit CancelLockupStream({ streamId: testStreamIds[1], sender: users.sender, + recipient: users.recipient, asset: dai, senderAmount: senderAmount1, recipientAmount: defaults.DEPOSIT_AMOUNT() - senderAmount1 diff --git a/test/integration/concrete/lockup/cancel/cancel.t.sol b/test/integration/concrete/lockup/cancel/cancel.t.sol index 4b89b79bb..616e864ee 100644 --- a/test/integration/concrete/lockup/cancel/cancel.t.sol +++ b/test/integration/concrete/lockup/cancel/cancel.t.sol @@ -71,7 +71,7 @@ abstract contract Cancel_Integration_Concrete_Test is Integration_Test, Cancel_I givenStreamWarm whenCallerUnauthorized { - // Make recipient the caller in this test. + // Make the Recipient the caller in this test. changePrank({ msgSender: users.recipient }); // Run the test. @@ -255,7 +255,7 @@ abstract contract Cancel_Integration_Concrete_Test is Integration_Test, Cancel_I // Expect the relevant events to be emitted. vm.expectEmit({ emitter: address(lockup) }); - emit CancelLockupStream(streamId, users.sender, dai, senderAmount, recipientAmount); + emit CancelLockupStream(streamId, users.sender, address(goodRecipient), dai, senderAmount, recipientAmount); vm.expectEmit({ emitter: address(lockup) }); emit MetadataUpdate({ _tokenId: streamId }); diff --git a/test/integration/concrete/lockup/withdraw-multiple/withdrawMultiple.t.sol b/test/integration/concrete/lockup/withdraw-multiple/withdrawMultiple.t.sol index d6b1cae41..33173b6a5 100644 --- a/test/integration/concrete/lockup/withdraw-multiple/withdrawMultiple.t.sol +++ b/test/integration/concrete/lockup/withdraw-multiple/withdrawMultiple.t.sol @@ -310,22 +310,22 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is emit WithdrawFromLockupStream({ streamId: testStreamIds[0], to: users.recipient, - amount: testAmounts[0], - asset: dai + asset: dai, + amount: testAmounts[0] }); vm.expectEmit({ emitter: address(lockup) }); emit WithdrawFromLockupStream({ streamId: testStreamIds[1], to: users.recipient, - amount: testAmounts[1], - asset: dai + asset: dai, + amount: testAmounts[1] }); vm.expectEmit({ emitter: address(lockup) }); emit WithdrawFromLockupStream({ streamId: testStreamIds[2], to: users.recipient, - amount: testAmounts[2], - asset: dai + asset: dai, + amount: testAmounts[2] }); // Make the withdrawals. diff --git a/test/integration/concrete/lockup/withdraw/withdraw.t.sol b/test/integration/concrete/lockup/withdraw/withdraw.t.sol index a26d1eb28..fbbeca2f6 100644 --- a/test/integration/concrete/lockup/withdraw/withdraw.t.sol +++ b/test/integration/concrete/lockup/withdraw/withdraw.t.sol @@ -299,8 +299,8 @@ abstract contract Withdraw_Integration_Concrete_Test is Integration_Test, Withdr emit WithdrawFromLockupStream({ streamId: defaultStreamId, to: users.recipient, - amount: withdrawAmount, - asset: dai + asset: dai, + amount: withdrawAmount }); // Make the withdrawal. @@ -499,8 +499,8 @@ abstract contract Withdraw_Integration_Concrete_Test is Integration_Test, Withdr emit WithdrawFromLockupStream({ streamId: streamId, to: address(goodRecipient), - amount: withdrawAmount, - asset: dai + asset: dai, + amount: withdrawAmount }); vm.expectEmit({ emitter: address(lockup) }); emit MetadataUpdate({ _tokenId: streamId }); diff --git a/test/integration/fuzz/lockup/cancel.t.sol b/test/integration/fuzz/lockup/cancel.t.sol index 48709418e..d59082cb5 100644 --- a/test/integration/fuzz/lockup/cancel.t.sol +++ b/test/integration/fuzz/lockup/cancel.t.sol @@ -81,7 +81,7 @@ abstract contract Cancel_Integration_Fuzz_Test is Integration_Test, Cancel_Integ // Expect the relevant events to be emitted. uint128 recipientAmount = lockup.withdrawableAmountOf(streamId); vm.expectEmit({ emitter: address(lockup) }); - emit CancelLockupStream(streamId, users.sender, dai, senderAmount, recipientAmount); + emit CancelLockupStream(streamId, users.sender, address(goodRecipient), dai, senderAmount, recipientAmount); vm.expectEmit({ emitter: address(lockup) }); emit MetadataUpdate({ _tokenId: streamId }); diff --git a/test/integration/fuzz/lockup/cancelMultiple.t.sol b/test/integration/fuzz/lockup/cancelMultiple.t.sol index 082da53ef..a5333106e 100644 --- a/test/integration/fuzz/lockup/cancelMultiple.t.sol +++ b/test/integration/fuzz/lockup/cancelMultiple.t.sol @@ -47,6 +47,7 @@ abstract contract CancelMultiple_Integration_Fuzz_Test is Integration_Test, Canc emit CancelLockupStream({ streamId: streamIds[0], sender: users.sender, + recipient: users.recipient, asset: dai, senderAmount: senderAmount0, recipientAmount: defaults.DEPOSIT_AMOUNT() - senderAmount0 @@ -55,6 +56,7 @@ abstract contract CancelMultiple_Integration_Fuzz_Test is Integration_Test, Canc emit CancelLockupStream({ streamId: streamIds[1], sender: users.sender, + recipient: users.recipient, asset: dai, senderAmount: senderAmount1, recipientAmount: defaults.DEPOSIT_AMOUNT() - senderAmount1 diff --git a/test/integration/fuzz/lockup/withdraw.t.sol b/test/integration/fuzz/lockup/withdraw.t.sol index 60ed8c78b..3ef2815c4 100644 --- a/test/integration/fuzz/lockup/withdraw.t.sol +++ b/test/integration/fuzz/lockup/withdraw.t.sol @@ -88,7 +88,7 @@ abstract contract Withdraw_Integration_Fuzz_Test is Integration_Test, Withdraw_I // Expect the relevant events to be emitted. vm.expectEmit({ emitter: address(lockup) }); - emit WithdrawFromLockupStream(defaultStreamId, to, withdrawAmount, dai); + emit WithdrawFromLockupStream(defaultStreamId, to, dai, withdrawAmount); vm.expectEmit({ emitter: address(lockup) }); emit MetadataUpdate({ _tokenId: defaultStreamId }); @@ -152,7 +152,7 @@ abstract contract Withdraw_Integration_Fuzz_Test is Integration_Test, Withdraw_I // Expect the relevant events to be emitted. vm.expectEmit({ emitter: address(lockup) }); - emit WithdrawFromLockupStream(defaultStreamId, to, withdrawAmount, dai); + emit WithdrawFromLockupStream(defaultStreamId, to, dai, withdrawAmount); vm.expectEmit({ emitter: address(lockup) }); emit MetadataUpdate({ _tokenId: defaultStreamId }); diff --git a/test/integration/fuzz/lockup/withdrawMax.t.sol b/test/integration/fuzz/lockup/withdrawMax.t.sol index 41a85e3dc..60b631199 100644 --- a/test/integration/fuzz/lockup/withdrawMax.t.sol +++ b/test/integration/fuzz/lockup/withdrawMax.t.sol @@ -25,8 +25,8 @@ abstract contract WithdrawMax_Integration_Fuzz_Test is Integration_Test, Withdra emit WithdrawFromLockupStream({ streamId: defaultStreamId, to: users.recipient, - amount: defaults.DEPOSIT_AMOUNT(), - asset: dai + asset: dai, + amount: defaults.DEPOSIT_AMOUNT() }); // Make the max withdrawal. @@ -69,8 +69,8 @@ abstract contract WithdrawMax_Integration_Fuzz_Test is Integration_Test, Withdra emit WithdrawFromLockupStream({ streamId: defaultStreamId, to: users.recipient, - amount: withdrawAmount, - asset: dai + asset: dai, + amount: withdrawAmount }); // Make the max withdrawal. diff --git a/test/integration/fuzz/lockup/withdrawMaxAndTransfer.t.sol b/test/integration/fuzz/lockup/withdrawMaxAndTransfer.t.sol index 05a49832e..8aef73118 100644 --- a/test/integration/fuzz/lockup/withdrawMaxAndTransfer.t.sol +++ b/test/integration/fuzz/lockup/withdrawMaxAndTransfer.t.sol @@ -45,8 +45,8 @@ abstract contract WithdrawMaxAndTransfer_Integration_Fuzz_Test is emit WithdrawFromLockupStream({ streamId: defaultStreamId, to: users.recipient, - amount: withdrawAmount, - asset: dai + asset: dai, + amount: withdrawAmount }); } diff --git a/test/integration/fuzz/lockup/withdrawMultiple.t.sol b/test/integration/fuzz/lockup/withdrawMultiple.t.sol index e9cb53e42..eff464020 100644 --- a/test/integration/fuzz/lockup/withdrawMultiple.t.sol +++ b/test/integration/fuzz/lockup/withdrawMultiple.t.sol @@ -64,9 +64,9 @@ abstract contract WithdrawMultiple_Integration_Fuzz_Test is // Expect the relevant events to be emitted. vm.expectEmit({ emitter: address(lockup) }); - emit WithdrawFromLockupStream({ streamId: ongoingStreamId, to: to, amount: ongoingWithdrawAmount, asset: dai }); + emit WithdrawFromLockupStream({ streamId: ongoingStreamId, to: to, asset: dai, amount: ongoingWithdrawAmount }); vm.expectEmit({ emitter: address(lockup) }); - emit WithdrawFromLockupStream({ streamId: settledStreamId, to: to, amount: settledWithdrawAmount, asset: dai }); + emit WithdrawFromLockupStream({ streamId: settledStreamId, to: to, asset: dai, amount: settledWithdrawAmount }); // Make the withdrawals. uint256[] memory streamIds = Solarray.uint256s(ongoingStreamId, settledStreamId); diff --git a/test/utils/Events.sol b/test/utils/Events.sol index 851bf0f92..13192c7a9 100644 --- a/test/utils/Events.sol +++ b/test/utils/Events.sol @@ -69,8 +69,9 @@ abstract contract Events { //////////////////////////////////////////////////////////////////////////*/ event CancelLockupStream( - uint256 indexed streamId, + uint256 streamId, address indexed sender, + address indexed recipient, IERC20 indexed asset, uint128 senderAmount, uint128 recipientAmount @@ -82,7 +83,7 @@ abstract contract Events { address indexed admin, ISablierV2NFTDescriptor oldNFTDescriptor, ISablierV2NFTDescriptor newNFTDescriptor ); - event WithdrawFromLockupStream(uint256 indexed streamId, address indexed to, uint128 amount, IERC20 indexed asset); + event WithdrawFromLockupStream(uint256 indexed streamId, address indexed to, IERC20 indexed asset, uint128 amount); /*////////////////////////////////////////////////////////////////////////// SABLIER-V2-LOCKUP-DYNAMIC