Merge pull request #686 from sablier-labs/feat/emit-metadata

feat: emit metadata in `_afterTokenTransfer` hook
sablier-labs · Sep 14, 2023 · cfd3150 · cfd3150
2 parents a31dec3 + f28b25a
commit cfd3150
Show file tree

Hide file tree

Showing 19 changed files with 155 additions and 15 deletions.
diff --git a/src/abstracts/SablierV2Lockup.sol b/src/abstracts/SablierV2Lockup.sol
@@ -324,11 +324,34 @@ abstract contract SablierV2Lockup is
                              INTERNAL CONSTANT FUNCTIONS
     //////////////////////////////////////////////////////////////////////////*/
 
+    /// @notice Overrides the internal ERC-721 transfer function to emit an ERC-4906 event upon transfer. The goal is to
+    /// refresh the NFT metadata on external platforms.
+    /// @dev This event is also emitted when the NFT is minted or burned.
+    function _afterTokenTransfer(
+        address, /* from */
+        address, /* to */
+        uint256 streamId,
+        uint256 /* batchSize */
+    )
+        internal
+        override
+        updateMetadata(streamId)
+    { }
+
     /// @notice Overrides the internal ERC-721 transfer function to check that the stream is transferable.
     /// @dev There are two cases when the transferable flag is ignored:
     /// - If `from` is 0, then the transfer is a mint and is allowed.
     /// - If `to` is 0, then the transfer is a burn and is also allowed.
-    function _beforeTokenTransfer(address from, address to, uint256 streamId, uint256) internal view override {
+    function _beforeTokenTransfer(
+        address from,
+        address to,
+        uint256 streamId,
+        uint256 /* batchSize */
+    )
+        internal
+        view
+        override
+    {
         if (!isTransferable(streamId) && to != address(0) && from != address(0)) {
             revert Errors.SablierV2Lockup_NotTransferrable(streamId);
         }

diff --git a/test/fork/LockupDynamic.t.sol b/test/fork/LockupDynamic.t.sol
@@ -154,11 +154,14 @@ abstract contract LockupDynamic_Fork_Test is Fork_Test {
         vars.initialLockupDynamicBalance = vars.balances[0];
         vars.initialBrokerBalance = vars.balances[1];
 
-        // Expect the relevant event to be emitted.
         vars.streamId = lockupDynamic.nextStreamId();
-        vm.expectEmit({ emitter: address(lockupDynamic) });
         vars.range =
             LockupDynamic.Range({ start: params.startTime, end: params.segments[params.segments.length - 1].milestone });
+
+        // Expect the relevant events to be emitted.
+        vm.expectEmit({ emitter: address(lockupDynamic) });
+        emit MetadataUpdate({ _tokenId: vars.streamId });
+        vm.expectEmit({ emitter: address(lockupDynamic) });
         emit CreateLockupDynamicStream({
             streamId: vars.streamId,
             funder: holder,

diff --git a/test/fork/LockupLinear.t.sol b/test/fork/LockupLinear.t.sol
@@ -92,7 +92,8 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
     /// - It should bump the next stream id.
     /// - It should record the protocol fee.
     /// - It should mint the NFT.
-    /// - It should emit a {CreateLockupDynamicStream} event.
+    /// - It should emit a {MetadataUpdate} event
+    /// - It should emit a {CreateLockupLinearStream} event.
     /// - It may make a withdrawal.
     /// - It may update the withdrawn amounts.
     /// - It may emit a {WithdrawFromLockupStream} event.
@@ -158,8 +159,11 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
         vars.createAmounts.brokerFee = ud(params.totalAmount).mul(params.broker.fee).intoUint128();
         vars.createAmounts.deposit = params.totalAmount - vars.createAmounts.protocolFee - vars.createAmounts.brokerFee;
 
-        // Expect the relevant event to be emitted.
         vars.streamId = lockupLinear.nextStreamId();
+
+        // Expect the relevant events to be emitted.
+        vm.expectEmit({ emitter: address(lockupLinear) });
+        emit MetadataUpdate({ _tokenId: vars.streamId });
         vm.expectEmit({ emitter: address(lockupLinear) });
         emit CreateLockupLinearStream({
             streamId: vars.streamId,

diff --git a/test/integration/concrete/lockup-dynamic/LockupDynamic.t.sol b/test/integration/concrete/lockup-dynamic/LockupDynamic.t.sol
@@ -30,6 +30,7 @@ import { Renounce_Integration_Concrete_Test } from "../lockup/renounce/renounce.
 import { SetComptroller_Integration_Concrete_Test } from "../lockup/set-comptroller/setComptroller.t.sol";
 import { SetNFTDescriptor_Integration_Concrete_Test } from "../lockup/set-nft-descriptor/setNFTDescriptor.t.sol";
 import { StatusOf_Integration_Concrete_Test } from "../lockup/status-of/statusOf.t.sol";
+import { TransferFrom_Integration_Concrete_Test } from "../lockup/transfer-from/transferFrom.t.sol";
 import { Withdraw_Integration_Concrete_Test } from "../lockup/withdraw/withdraw.t.sol";
 import { WasCanceled_Integration_Concrete_Test } from "../lockup/was-canceled/wasCanceled.t.sol";
 import { WithdrawMax_Integration_Concrete_Test } from "../lockup/withdraw-max/withdrawMax.t.sol";
@@ -391,6 +392,20 @@ contract StatusOf_LockupDynamic_Integration_Concrete_Test is
     }
 }
 
+contract TransferFrom_LockupDynamic_Integration_Concrete_Test is
+    LockupDynamic_Integration_Concrete_Test,
+    TransferFrom_Integration_Concrete_Test
+{
+    function setUp()
+        public
+        virtual
+        override(LockupDynamic_Integration_Concrete_Test, TransferFrom_Integration_Concrete_Test)
+    {
+        LockupDynamic_Integration_Concrete_Test.setUp();
+        TransferFrom_Integration_Concrete_Test.setUp();
+    }
+}
+
 contract WasCanceled_LockupDynamic_Integration_Concrete_Test is
     LockupDynamic_Integration_Concrete_Test,
     WasCanceled_Integration_Concrete_Test

diff --git a/test/integration/concrete/lockup-dynamic/create-with-deltas/createWithDeltas.t.sol b/test/integration/concrete/lockup-dynamic/create-with-deltas/createWithDeltas.t.sol
@@ -145,7 +145,9 @@ contract CreateWithDeltas_LockupDynamic_Integration_Concrete_Test is
         // Expect the broker fee to be paid to the broker.
         expectCallToTransferFrom({ from: funder, to: users.broker, amount: defaults.BROKER_FEE_AMOUNT() });
 
-        // Expect the relevant event to be emitted.
+        // Expect the relevant events to be emitted.
+        vm.expectEmit({ emitter: address(lockupDynamic) });
+        emit MetadataUpdate({ _tokenId: streamId });
         vm.expectEmit({ emitter: address(lockupDynamic) });
         emit CreateLockupDynamicStream({
             streamId: streamId,

diff --git a/test/integration/concrete/lockup-dynamic/create-with-deltas/createWithDeltas.tree b/test/integration/concrete/lockup-dynamic/create-with-deltas/createWithDeltas.tree
@@ -18,5 +18,6 @@ createWithDeltas.t.sol
              ├── it should bump the next stream id
              ├── it should record the protocol fee
              ├── it should mint the NFT
+             ├── it should emit a {MetadataUpdate} event
              ├── it should perform the ERC-20 transfers
              └── it should emit a {CreateLockupDynamicStream} event
diff --git a/test/integration/concrete/lockup-dynamic/create-with-milestones/createWithMilestones.t.sol b/test/integration/concrete/lockup-dynamic/create-with-milestones/createWithMilestones.t.sol
@@ -359,7 +359,9 @@ contract CreateWithMilestones_LockupDynamic_Integration_Concrete_Test is
             amount: defaults.BROKER_FEE_AMOUNT()
         });
 
-        // Expect the relevant event to be emitted.
+        // Expect the relevant events to be emitted.
+        vm.expectEmit({ emitter: address(lockupDynamic) });
+        emit MetadataUpdate({ _tokenId: streamId });
         vm.expectEmit({ emitter: address(lockupDynamic) });
         emit CreateLockupDynamicStream({
             streamId: streamId,

diff --git a/test/integration/concrete/lockup-dynamic/create-with-milestones/createWithMilestones.tree b/test/integration/concrete/lockup-dynamic/create-with-milestones/createWithMilestones.tree
@@ -45,12 +45,14 @@ createWithMilestones.t.sol
                                        │  ├── it should bump the next stream id
                                        │  ├── it should record the protocol fee
                                        │  ├── it should mint the NFT
+                                       │  ├── it should emit a {MetadataUpdate} event
                                        │  ├── it should perform the ERC-20 transfers
                                        │  └── it should emit a {CreateLockupDynamicStream} event
                                        └── when the asset does not miss the ERC-20 return value
                                           ├── it should create the stream
                                           ├── it should bump the next stream id
                                           ├── it should record the protocol fee
                                           ├── it should mint the NFT
+                                          ├── it should emit a {MetadataUpdate} event
                                           ├── it should perform the ERC-20 transfers
                                           └── it should emit a {CreateLockupDynamicStream} event
diff --git a/test/integration/concrete/lockup-linear/LockupLinear.t.sol b/test/integration/concrete/lockup-linear/LockupLinear.t.sol
@@ -31,6 +31,7 @@ import { Renounce_Integration_Concrete_Test } from "../lockup/renounce/renounce.
 import { SetComptroller_Integration_Concrete_Test } from "../lockup/set-comptroller/setComptroller.t.sol";
 import { SetNFTDescriptor_Integration_Concrete_Test } from "../lockup/set-nft-descriptor/setNFTDescriptor.t.sol";
 import { StatusOf_Integration_Concrete_Test } from "../lockup/status-of/statusOf.t.sol";
+import { TransferFrom_Integration_Concrete_Test } from "../lockup/transfer-from/transferFrom.t.sol";
 import { WasCanceled_Integration_Concrete_Test } from "../lockup/was-canceled/wasCanceled.t.sol";
 import { Withdraw_Integration_Concrete_Test } from "../lockup/withdraw/withdraw.t.sol";
 import { WithdrawMax_Integration_Concrete_Test } from "../lockup/withdraw-max/withdrawMax.t.sol";
@@ -392,6 +393,20 @@ contract StatusOf_LockupLinear_Integration_Concrete_Test is
     }
 }
 
+contract TransferFrom_LockupLinear_Integration_Concrete_Test is
+    LockupLinear_Integration_Concrete_Test,
+    TransferFrom_Integration_Concrete_Test
+{
+    function setUp()
+        public
+        virtual
+        override(LockupLinear_Integration_Concrete_Test, TransferFrom_Integration_Concrete_Test)
+    {
+        LockupLinear_Integration_Concrete_Test.setUp();
+        TransferFrom_Integration_Concrete_Test.setUp();
+    }
+}
+
 contract WasCanceled_LockupLinear_Integration_Concrete_Test is
     LockupLinear_Integration_Concrete_Test,
     WasCanceled_Integration_Concrete_Test

diff --git a/test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.t.sol b/test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.t.sol
@@ -110,7 +110,9 @@ contract CreateWithDurations_LockupLinear_Integration_Concrete_Test is
         // Expect the broker fee to be paid to the broker.
         expectCallToTransferFrom({ from: funder, to: users.broker, amount: defaults.BROKER_FEE_AMOUNT() });
 
-        // Expect the relevant event to be emitted.
+        // Expect the relevant events to be emitted.
+        vm.expectEmit({ emitter: address(lockupLinear) });
+        emit MetadataUpdate({ _tokenId: streamId });
         vm.expectEmit({ emitter: address(lockupLinear) });
         emit CreateLockupLinearStream({
             streamId: streamId,

diff --git a/test/integration/concrete/lockup-linear/create-with-range/createWithRange.t.sol b/test/integration/concrete/lockup-linear/create-with-range/createWithRange.t.sol
@@ -197,7 +197,9 @@ contract CreateWithRange_LockupLinear_Integration_Concrete_Test is
             amount: defaults.BROKER_FEE_AMOUNT()
         });
 
-        // Expect the relevant event to be emitted.
+        // Expect the relevant events to be emitted.
+        vm.expectEmit({ emitter: address(lockupLinear) });
+        emit MetadataUpdate({ _tokenId: streamId });
         vm.expectEmit({ emitter: address(lockupLinear) });
         emit CreateLockupLinearStream({
             streamId: streamId,

diff --git a/test/integration/concrete/lockup-linear/create-with-range/createWithRange.tree b/test/integration/concrete/lockup-linear/create-with-range/createWithRange.tree
@@ -31,13 +31,15 @@ createWithRange.t.sol
                            │  ├── it should bump the next stream id
                            │  ├── it should record the protocol fee
                            │  ├── it should mint the NFT
+                           │  ├── it should emit a {MetadataUpdate} event
                            │  ├── it should perform the ERC-20 transfers
                            │  └── it should emit a {CreateLockupLinearStream} event
                            └── when the asset does not miss the ERC-20 return value
                               ├── it should create the stream
                               ├── it should bump the next stream id
                               ├── it should record the protocol fee
                               ├── it should mint the NFT
+                              ├── it should emit a {MetadataUpdate} event
                               ├── it should perform the ERC-20 transfers
                               └── it should emit a {CreateLockupLinearStream} event
 
diff --git a/test/integration/concrete/lockup/burn/burn.t.sol b/test/integration/concrete/lockup/burn/burn.t.sol
@@ -136,6 +136,9 @@ abstract contract Burn_Integration_Concrete_Test is Integration_Test, Lockup_Int
         whenCallerAuthorized
         givenNFTExists
     {
+        // Expect the relevant event to be emitted.
+        vm.expectEmit({ emitter: address(lockup) });
+        emit MetadataUpdate({ _tokenId: notTransferableStreamId });
         lockup.burn(notTransferableStreamId);
         vm.expectRevert("ERC721: invalid token ID");
         lockup.getRecipient(notTransferableStreamId);
@@ -160,6 +163,10 @@ abstract contract Burn_Integration_Concrete_Test is Integration_Test, Lockup_Int
         // Make the approved operator the caller in this test.
         changePrank({ msgSender: users.operator });
 
+        // Expect the relevant event to be emitted.
+        vm.expectEmit({ emitter: address(lockup) });
+        emit MetadataUpdate({ _tokenId: streamId });
+
         // Burn the NFT.
         lockup.burn(streamId);
 
@@ -177,6 +184,9 @@ abstract contract Burn_Integration_Concrete_Test is Integration_Test, Lockup_Int
         givenNFTExists
         givenTransferableStream
     {
+        // Expect the relevant event to be emitted.
+        vm.expectEmit({ emitter: address(lockup) });
+        emit MetadataUpdate({ _tokenId: streamId });
         lockup.burn(streamId);
         vm.expectRevert("ERC721: invalid token ID");
         lockup.getRecipient(streamId);

diff --git a/test/integration/concrete/lockup/burn/burn.tree b/test/integration/concrete/lockup/burn/burn.tree
@@ -22,9 +22,12 @@ burn.t.sol
             │  └── it should revert
             └── given the NFT exists
                ├── given the NFT is not transferable
-               │  └── it should burn the NFT
+               │  ├── it should burn the NFT
+               │  └── it should emit a {MetadataUpdate} event
                └── given the NFT is transferable
                   ├── when the caller is an approved third party
-                  │  └── it should burn the NFT
+                  │  ├── it should burn the NFT
+                  │  └── it should emit a {MetadataUpdate} event
                   └── when the caller is the owner of the NFT
-                     └── it should burn the NFT
+                     ├── it should burn the NFT
+                     └── it should emit a {MetadataUpdate} event
diff --git a/test/integration/concrete/lockup/transfer-from/transferFrom.t.sol b/test/integration/concrete/lockup/transfer-from/transferFrom.t.sol
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity >=0.8.19 <0.9.0;
+
+import { Errors } from "src/libraries/Errors.sol";
+
+import { Lockup_Integration_Shared_Test } from "../../../shared/lockup/Lockup.t.sol";
+import { Integration_Test } from "../../../Integration.t.sol";
+
+abstract contract TransferFrom_Integration_Concrete_Test is Integration_Test, Lockup_Integration_Shared_Test {
+    function setUp() public virtual override(Integration_Test, Lockup_Integration_Shared_Test) {
+        changePrank({ msgSender: users.recipient });
+    }
+
+    function test_RevertGiven_StreamNotTransferable() external {
+        uint256 notTransferableStreamId = createDefaultStreamNotTransferable();
+        vm.expectRevert(
+            abi.encodeWithSelector(Errors.SablierV2Lockup_NotTransferrable.selector, notTransferableStreamId)
+        );
+        lockup.transferFrom({ from: users.recipient, to: users.alice, tokenId: notTransferableStreamId });
+    }
+
+    modifier givenStreamTransferable() {
+        _;
+    }
+
+    function test_TransferFrom() external givenStreamTransferable {
+        // Create a stream.
+        uint256 streamId = createDefaultStream();
+
+        // Expect the relevant events to be emitted.
+        vm.expectEmit({ emitter: address(lockup) });
+        emit Transfer({ from: users.recipient, to: users.alice, tokenId: streamId });
+        vm.expectEmit({ emitter: address(lockup) });
+        emit MetadataUpdate({ _tokenId: streamId });
+
+        // Transfer the NFT.
+        lockup.transferFrom({ from: users.recipient, to: users.alice, tokenId: streamId });
+
+        // Assert that Alice is the new stream recipient (and NFT owner).
+        address actualRecipient = lockup.getRecipient(streamId);
+        address expectedRecipient = users.alice;
+        assertEq(actualRecipient, expectedRecipient, "recipient");
+    }
+}
diff --git a/test/integration/concrete/lockup/transfer-from/transferFrom.tree b/test/integration/concrete/lockup/transfer-from/transferFrom.tree
@@ -0,0 +1,7 @@
+transferFrom.t.sol
+├── given the stream is not transferable
+│  └── it should revert
+└── given the stream is transferable
+   ├── it should transfer the NFT
+   ├── it should emit a {Transfer} event
+   └── it should emit a {MetadataUpdate} event
diff --git a/test/integration/concrete/lockup/withdraw-max-and-transfer/withdrawMaxAndTransfer.t.sol b/test/integration/concrete/lockup/withdraw-max-and-transfer/withdrawMaxAndTransfer.t.sol
@@ -103,6 +103,8 @@ abstract contract WithdrawMaxAndTransfer_Integration_Concrete_Test is
         emit WithdrawFromLockupStream({ streamId: defaultStreamId, to: users.recipient, amount: withdrawAmount });
         vm.expectEmit({ emitter: address(lockup) });
         emit Transfer({ from: users.recipient, to: users.alice, tokenId: defaultStreamId });
+        vm.expectEmit({ emitter: address(lockup) });
+        emit MetadataUpdate({ _tokenId: defaultStreamId });
 
         // Make the max withdrawal and transfer the NFT.
         lockup.withdrawMaxAndTransfer({ streamId: defaultStreamId, newRecipient: users.alice });

diff --git a/test/integration/concrete/lockup/withdraw-max-and-transfer/withdrawMaxAndTransfer.tree b/test/integration/concrete/lockup/withdraw-max-and-transfer/withdrawMaxAndTransfer.tree
@@ -21,4 +21,5 @@ withdrawMaxAndTransfer.t.sol
                   ├── it should update the withdrawn amount
                   ├── it should transfer the NFT
                   ├── it should emit a {WithdrawFromLockupStream} event
-                  └── it should emit a {Transfer} event
+                  ├── it should emit a {Transfer} event
+                  └── it should emit a {MetadataUpdate} event
diff --git a/test/utils/Precompiles.sol b/test/utils/Precompiles.sol