Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: emit metadata in _afterTokenTransfer hook #686

Merged
merged 6 commits into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/abstracts/SablierV2Lockup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
7 changes: 5 additions & 2 deletions test/fork/LockupDynamic.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
8 changes: 6 additions & 2 deletions test/fork/LockupLinear.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
15 changes: 15 additions & 0 deletions test/integration/concrete/lockup-dynamic/LockupDynamic.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
15 changes: 15 additions & 0 deletions test/integration/concrete/lockup-linear/LockupLinear.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

10 changes: 10 additions & 0 deletions test/integration/concrete/lockup/burn/burn.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);

Expand All @@ -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);
Expand Down
9 changes: 6 additions & 3 deletions test/integration/concrete/lockup/burn/burn.tree
Original file line number Diff line number Diff line change
Expand Up @@ -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
44 changes: 44 additions & 0 deletions test/integration/concrete/lockup/transfer-from/transferFrom.t.sol
Original file line number Diff line number Diff line change
@@ -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");
}
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 2 additions & 2 deletions test/utils/Precompiles.sol

Large diffs are not rendered by default.