Skip to content

Commit

Permalink
Upgrade to OpenZeppelin V5 (#806)
Browse files Browse the repository at this point in the history 
* build: update openzeppelin to 5.0.0

refactor: remove _after and _before hooks and implement _update function
refactor: use newly added _requireOwned function
refactor: use the new named parameter in ERC20 functions
test: add mocks for ERC20 and ERC721
test: use openeppelin's custom errors in cheatcode
chore: provide a more precise explanation for _update
chore: define "_update" alphabetically
docs: document "_update" function
test: check IERC20Errors custom error revert

* perf: optimize getRecipient function

docs: use known terminology in _update natspec
  • Loading branch information
@andreivladbrg
andreivladbrg committed Feb 12, 2024
1 parent 22a8191 commit 45980e7
Show file tree
Hide file tree
Showing 38 changed files with 147 additions and 128 deletions.
Binary file modified bun.lockb
View file
Binary file not shown.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"url": "https://github.com/sablier-labs/v2-core/issues"
},
"dependencies": {
"@openzeppelin/contracts": "4.9.2",
"@openzeppelin/contracts": "5.0.0",
"@prb/math": "4.0.2"
},
"devDependencies": {
Expand Down
8 changes: 4 additions & 4 deletions script/Init.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { Broker, LockupDynamic, LockupLinear } from "../src/types/DataTypes.sol"
import { BaseScript } from "./Base.s.sol";

interface IERC20Mint {
function mint(address beneficiary, uint256 amount) external;
function mint(address beneficiary, uint256 value) external;
}

/// @notice Initializes the protocol by setting up the comptroller and creating some streams.
Expand All @@ -35,11 +35,11 @@ contract Init is BaseScript {
//////////////////////////////////////////////////////////////////////////*/

// Mint enough assets to the sender.
IERC20Mint(address(asset)).mint({ beneficiary: sender, amount: 131_601.1e18 + 10_000e18 });
IERC20Mint(address(asset)).mint({ beneficiary: sender, value: 131_601.1e18 + 10_000e18 });

// Approve the Sablier contracts to transfer the ERC-20 assets from the sender.
asset.approve({ spender: address(lockupLinear), amount: type(uint256).max });
asset.approve({ spender: address(lockupDynamic), amount: type(uint256).max });
asset.approve({ spender: address(lockupLinear), value: type(uint256).max });
asset.approve({ spender: address(lockupDynamic), value: type(uint256).max });

// Create 7 Lockup Linear streams with various amounts and durations.
//
Expand Down
71 changes: 32 additions & 39 deletions src/abstracts/SablierV2Lockup.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,8 @@ abstract contract SablierV2Lockup is

/// @inheritdoc ISablierV2Lockup
function getRecipient(uint256 streamId) external view override returns (address recipient) {
// Checks: the stream NFT exists.
_requireMinted({ tokenId: streamId });

// The NFT owner is the stream's recipient.
recipient = _ownerOf(streamId);
// Checks: the stream NFT exists, and return the owner, which is the stream's recipient.
recipient = _requireOwned({ tokenId: streamId });
}

/// @inheritdoc ISablierV2Lockup
Expand All @@ -104,7 +101,7 @@ abstract contract SablierV2Lockup is
/// @inheritdoc ERC721
function tokenURI(uint256 streamId) public view override(IERC721Metadata, ERC721) returns (string memory uri) {
// Checks: the stream NFT exists.
_requireMinted({ tokenId: streamId });
_requireOwned({ tokenId: streamId });

// Generate the URI describing the stream NFT.
uri = nftDescriptor.tokenURI({ sablier: this, streamId: streamId });
Expand Down Expand Up @@ -355,39 +352,6 @@ 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 /* batchSize */
)
internal
view
override
{
if (!isTransferable(streamId) && to != address(0) && from != address(0)) {
revert Errors.SablierV2Lockup_NotTransferable(streamId);
}
}

/// @notice Checks whether `msg.sender` is the stream's recipient or an approved third party.
/// @param streamId The stream id for the query.
function _isCallerStreamRecipientOrApproved(uint256 streamId) internal view returns (bool) {
Expand All @@ -403,6 +367,35 @@ abstract contract SablierV2Lockup is
/// @dev Retrieves the stream's status without performing a null check.
function _statusOf(uint256 streamId) internal view virtual returns (Lockup.Status);

/// @notice Overrides the internal ERC-721 `_update` function to check that the stream is transferable and emit
/// an ERC-4906 event.
/// @dev There are two cases when the transferable flag is ignored:
/// - If `from` is 0, then the update is a mint and is allowed.
/// - If `to` is 0, then the update is a burn and is also allowed.
/// @param to The address of the new recipient of the stream.
/// @param streamId Id of the stream to update.
/// @param auth Optional parameter. If the value is non 0, the upstream implementation of this function will check
/// that `auth` is either the recipient of the stream, or an approved third party.
/// @return The original recipient of the `streamId` before the update.
function _update(
address to,
uint256 streamId,
address auth
)
internal
override
updateMetadata(streamId)
returns (address)
{
address from = _ownerOf(streamId);

if (!isTransferable(streamId) && from != address(0) && to != address(0)) {
revert Errors.SablierV2Lockup_NotTransferable(streamId);
}

return super._update(to, streamId, auth);
}

/// @dev See the documentation for the user-facing functions that call this internal function.
function _withdrawableAmountOf(uint256 streamId) internal view virtual returns (uint128);

Expand Down
38 changes: 19 additions & 19 deletions test/Base.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;

import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

import { ISablierV2Comptroller } from "../src/interfaces/ISablierV2Comptroller.sol";
Expand All @@ -13,6 +12,7 @@ import { SablierV2LockupDynamic } from "../src/SablierV2LockupDynamic.sol";
import { SablierV2LockupLinear } from "../src/SablierV2LockupLinear.sol";
import { SablierV2NFTDescriptor } from "../src/SablierV2NFTDescriptor.sol";

import { ERC20Mock } from "./mocks/erc20/ERC20Mock.sol";
import { ERC20MissingReturn } from "./mocks/erc20/ERC20MissingReturn.sol";
import { Noop } from "./mocks/Noop.sol";
import { GoodRecipient } from "./mocks/hooks/GoodRecipient.sol";
Expand All @@ -39,7 +39,7 @@ abstract contract Base_Test is Assertions, Calculations, Constants, DeployOptimi
//////////////////////////////////////////////////////////////////////////*/

ISablierV2Comptroller internal comptroller;
ERC20 internal dai;
ERC20Mock internal dai;
Defaults internal defaults;
GoodRecipient internal goodRecipient;
GoodSender internal goodSender;
Expand All @@ -55,7 +55,7 @@ abstract contract Base_Test is Assertions, Calculations, Constants, DeployOptimi

function setUp() public virtual {
// Deploy the base test contracts.
dai = new ERC20("Dai Stablecoin", "DAI");
dai = new ERC20Mock("Dai Stablecoin", "DAI");
goodRecipient = new GoodRecipient();
goodSender = new GoodSender();
noop = new Noop();
Expand Down Expand Up @@ -96,26 +96,26 @@ abstract contract Base_Test is Assertions, Calculations, Constants, DeployOptimi
/// @dev Approves all V2 Core contracts to spend assets from the Sender, Recipient, Alice and Eve.
function approveProtocol() internal {
changePrank({ msgSender: users.sender });
dai.approve({ spender: address(lockupLinear), amount: MAX_UINT256 });
dai.approve({ spender: address(lockupDynamic), amount: MAX_UINT256 });
dai.approve({ spender: address(lockupLinear), value: MAX_UINT256 });
dai.approve({ spender: address(lockupDynamic), value: MAX_UINT256 });
usdt.approve({ spender: address(lockupLinear), value: MAX_UINT256 });
usdt.approve({ spender: address(lockupDynamic), value: MAX_UINT256 });

changePrank({ msgSender: users.recipient });
dai.approve({ spender: address(lockupLinear), amount: MAX_UINT256 });
dai.approve({ spender: address(lockupDynamic), amount: MAX_UINT256 });
dai.approve({ spender: address(lockupLinear), value: MAX_UINT256 });
dai.approve({ spender: address(lockupDynamic), value: MAX_UINT256 });
usdt.approve({ spender: address(lockupLinear), value: MAX_UINT256 });
usdt.approve({ spender: address(lockupDynamic), value: MAX_UINT256 });

changePrank({ msgSender: users.alice });
dai.approve({ spender: address(lockupLinear), amount: MAX_UINT256 });
dai.approve({ spender: address(lockupDynamic), amount: MAX_UINT256 });
dai.approve({ spender: address(lockupLinear), value: MAX_UINT256 });
dai.approve({ spender: address(lockupDynamic), value: MAX_UINT256 });
usdt.approve({ spender: address(lockupLinear), value: MAX_UINT256 });
usdt.approve({ spender: address(lockupDynamic), value: MAX_UINT256 });

changePrank({ msgSender: users.eve });
dai.approve({ spender: address(lockupLinear), amount: MAX_UINT256 });
dai.approve({ spender: address(lockupDynamic), amount: MAX_UINT256 });
dai.approve({ spender: address(lockupLinear), value: MAX_UINT256 });
dai.approve({ spender: address(lockupDynamic), value: MAX_UINT256 });
usdt.approve({ spender: address(lockupLinear), value: MAX_UINT256 });
usdt.approve({ spender: address(lockupDynamic), value: MAX_UINT256 });

Expand Down Expand Up @@ -160,22 +160,22 @@ abstract contract Base_Test is Assertions, Calculations, Constants, DeployOptimi
//////////////////////////////////////////////////////////////////////////*/

/// @dev Expects a call to {IERC20.transfer}.
function expectCallToTransfer(address to, uint256 amount) internal {
vm.expectCall({ callee: address(dai), data: abi.encodeCall(IERC20.transfer, (to, amount)) });
function expectCallToTransfer(address to, uint256 value) internal {
vm.expectCall({ callee: address(dai), data: abi.encodeCall(IERC20.transfer, (to, value)) });
}

/// @dev Expects a call to {IERC20.transfer}.
function expectCallToTransfer(IERC20 asset, address to, uint256 amount) internal {
vm.expectCall({ callee: address(asset), data: abi.encodeCall(IERC20.transfer, (to, amount)) });
function expectCallToTransfer(IERC20 asset, address to, uint256 value) internal {
vm.expectCall({ callee: address(asset), data: abi.encodeCall(IERC20.transfer, (to, value)) });
}

/// @dev Expects a call to {IERC20.transferFrom}.
function expectCallToTransferFrom(address from, address to, uint256 amount) internal {
vm.expectCall({ callee: address(dai), data: abi.encodeCall(IERC20.transferFrom, (from, to, amount)) });
function expectCallToTransferFrom(address from, address to, uint256 value) internal {
vm.expectCall({ callee: address(dai), data: abi.encodeCall(IERC20.transferFrom, (from, to, value)) });
}

/// @dev Expects a call to {IERC20.transferFrom}.
function expectCallToTransferFrom(IERC20 asset, address from, address to, uint256 amount) internal {
vm.expectCall({ callee: address(asset), data: abi.encodeCall(IERC20.transferFrom, (from, to, amount)) });
function expectCallToTransferFrom(IERC20 asset, address from, address to, uint256 value) internal {
vm.expectCall({ callee: address(asset), data: abi.encodeCall(IERC20.transferFrom, (from, to, value)) });
}
}
4 changes: 2 additions & 2 deletions test/integration/concrete/lockup-dynamic/create-with-durations/createWithDurations.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,11 @@ contract CreateWithDurations_LockupDynamic_Integration_Concrete_Test is
expectCallToTransferFrom({
from: funder,
to: address(lockupDynamic),
amount: defaults.DEPOSIT_AMOUNT() + defaults.PROTOCOL_FEE_AMOUNT()
value: defaults.DEPOSIT_AMOUNT() + defaults.PROTOCOL_FEE_AMOUNT()
});

// Expect the broker fee to be paid to the broker.
expectCallToTransferFrom({ from: funder, to: users.broker, amount: defaults.BROKER_FEE_AMOUNT() });
expectCallToTransferFrom({ from: funder, to: users.broker, value: defaults.BROKER_FEE_AMOUNT() });

// Expect the relevant events to be emitted.
vm.expectEmit({ emitter: address(lockupDynamic) });
Expand Down
10 changes: 6 additions & 4 deletions test/integration/concrete/lockup-dynamic/create-with-timestamps/createWithTimestamps.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;

import { Address } from "@openzeppelin/contracts/utils/Address.sol";
import { IERC721Errors } from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { UD60x18, ud, ZERO } from "@prb/math/src/UD60x18.sol";
import { stdError } from "forge-std/src/StdError.sol";
Expand Down Expand Up @@ -33,8 +35,8 @@ contract CreateWithTimestamps_LockupDynamic_Integration_Concrete_Test is
}

function test_RevertWhen_RecipientZeroAddress() external whenNotDelegateCalled {
vm.expectRevert("ERC721: mint to the zero address");
address recipient = address(0);
vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidReceiver.selector, recipient));
createDefaultStreamWithRecipient(recipient);
}

Expand Down Expand Up @@ -295,7 +297,7 @@ contract CreateWithTimestamps_LockupDynamic_Integration_Concrete_Test is
changePrank({ msgSender: users.sender });

// Run the test.
vm.expectRevert("Address: call to non-contract");
vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, nonContract));
createDefaultStreamWithAsset(IERC20(nonContract));
}

Expand Down Expand Up @@ -348,15 +350,15 @@ contract CreateWithTimestamps_LockupDynamic_Integration_Concrete_Test is
asset: IERC20(asset),
from: funder,
to: address(lockupDynamic),
amount: defaults.DEPOSIT_AMOUNT() + defaults.PROTOCOL_FEE_AMOUNT()
value: defaults.DEPOSIT_AMOUNT() + defaults.PROTOCOL_FEE_AMOUNT()
});

// Expect the broker fee to be paid to the broker.
expectCallToTransferFrom({
asset: IERC20(asset),
from: funder,
to: users.broker,
amount: defaults.BROKER_FEE_AMOUNT()
value: defaults.BROKER_FEE_AMOUNT()
});

// Expect the relevant events to be emitted.
Expand Down
3 changes: 2 additions & 1 deletion test/integration/concrete/lockup-dynamic/token-uri/tokenURI.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// solhint-disable max-line-length,no-console,quotes
pragma solidity >=0.8.22 <0.9.0;

import { IERC721Errors } from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";
import { console2 } from "forge-std/src/console2.sol";
import { LibString } from "solady/src/utils/LibString.sol";
import { StdStyle } from "forge-std/src/StdStyle.sol";
Expand Down Expand Up @@ -33,7 +34,7 @@ contract TokenURI_LockupDynamic_Integration_Concrete_Test is LockupDynamic_Integ

function test_RevertGiven_NFTDoesNotExist() external {
uint256 nullStreamId = 1729;
vm.expectRevert("ERC721: invalid token ID");
vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721NonexistentToken.selector, nullStreamId));
lockupDynamic.tokenURI({ tokenId: nullStreamId });
}

Expand Down
4 changes: 2 additions & 2 deletions test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,11 @@ contract CreateWithDurations_LockupLinear_Integration_Concrete_Test is
expectCallToTransferFrom({
from: funder,
to: address(lockupLinear),
amount: defaults.DEPOSIT_AMOUNT() + defaults.PROTOCOL_FEE_AMOUNT()
value: defaults.DEPOSIT_AMOUNT() + defaults.PROTOCOL_FEE_AMOUNT()
});

// Expect the broker fee to be paid to the broker.
expectCallToTransferFrom({ from: funder, to: users.broker, amount: defaults.BROKER_FEE_AMOUNT() });
expectCallToTransferFrom({ from: funder, to: users.broker, value: defaults.BROKER_FEE_AMOUNT() });

// Expect the relevant events to be emitted.
vm.expectEmit({ emitter: address(lockupLinear) });
Expand Down
13 changes: 8 additions & 5 deletions test/integration/concrete/lockup-linear/create-with-timestamps/createWithTimestamps.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;

import { Address } from "@openzeppelin/contracts/utils/Address.sol";
import { IERC721Errors } from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { UD60x18, ud } from "@prb/math/src/UD60x18.sol";

Expand Down Expand Up @@ -32,8 +34,9 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
}

function test_RevertWhen_RecipientZeroAddress() external whenNotDelegateCalled {
vm.expectRevert("ERC721: mint to the zero address");
createDefaultStreamWithRecipient({ recipient: address(0) });
address recipient = address(0);
vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidReceiver.selector, recipient));
createDefaultStreamWithRecipient(recipient);
}

/// @dev It is not possible to obtain a zero deposit amount from a non-zero total amount, because the
Expand Down Expand Up @@ -143,7 +146,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
whenBrokerFeeNotTooHigh
{
address nonContract = address(8128);
vm.expectRevert("Address: call to non-contract");
vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, nonContract));
createDefaultStreamWithAsset(IERC20(nonContract));
}

Expand Down Expand Up @@ -187,15 +190,15 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
asset: IERC20(asset),
from: funder,
to: address(lockupLinear),
amount: defaults.DEPOSIT_AMOUNT() + defaults.PROTOCOL_FEE_AMOUNT()
value: defaults.DEPOSIT_AMOUNT() + defaults.PROTOCOL_FEE_AMOUNT()
});

// Expect the broker fee to be paid to the broker.
expectCallToTransferFrom({
asset: IERC20(asset),
from: funder,
to: users.broker,
amount: defaults.BROKER_FEE_AMOUNT()
value: defaults.BROKER_FEE_AMOUNT()
});

// Expect the relevant events to be emitted.
Expand Down
3 changes: 2 additions & 1 deletion test/integration/concrete/lockup-linear/token-uri/tokenURI.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// solhint-disable max-line-length,no-console,quotes
pragma solidity >=0.8.22 <0.9.0;

import { IERC721Errors } from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";
import { console2 } from "forge-std/src/console2.sol";
import { LibString } from "solady/src/utils/LibString.sol";
import { StdStyle } from "forge-std/src/StdStyle.sol";
Expand Down Expand Up @@ -33,7 +34,7 @@ contract TokenURI_LockupLinear_Integration_Concrete_Test is LockupLinear_Integra

function test_RevertGiven_NFTDoesNotExist() external {
uint256 nullStreamId = 1729;
vm.expectRevert("ERC721: invalid token ID");
vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721NonexistentToken.selector, nullStreamId));
lockupLinear.tokenURI({ tokenId: nullStreamId });
}

Expand Down
Loading

0 comments on commit 45980e7

Please sign in to comment.