From 9c3386a2be8593e061bfdb008ddad9d44b5c62ab Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Sun, 6 Aug 2023 02:30:49 -0300 Subject: [PATCH] I remove redundant check in BeforeTokenTransferHook to see if hook should get used. Implementers can include that logic in their versions of the hook. Removing the make the hooks as minimal and unbiased as possible. --- src/ERC721ACH.sol | 131 +++++++++++------- src/interfaces/IBeforeTokenTransfersHook.sol | 41 ++---- src/interfaces/IERC721ACH.sol | 24 ++-- test/hooks/BeforeTokenTransfers.t.sol | 75 ++++++---- .../hooks/BeforeTokenTransfersHookMock.sol | 23 +-- 5 files changed, 151 insertions(+), 143 deletions(-) diff --git a/src/ERC721ACH.sol b/src/ERC721ACH.sol index 666ddef..b740905 100644 --- a/src/ERC721ACH.sol +++ b/src/ERC721ACH.sol @@ -11,26 +11,24 @@ import {IERC721ACH} from "./interfaces/IERC721ACH.sol"; /** * @title ERC721ACH * @author Cre8ors Inc. - * @notice Extends Limit Break's ERC721-AC implementation with Hook functionality, which - * allows the contract owner to override hooks associated with core ERC721 functions. + * @notice This contract extends Limit Break's ERC721-AC implementation with hook functionality. + * It allows the contract owner to set hooks that modify the behavior of core ERC721 + * functions. Each hook type can be associated with a contract that implements the + * corresponding hook's logic. Only the contract owner can set or change these hooks. */ contract ERC721ACH is ERC721AC, IERC721ACH { - - /** - * @dev Mapping of hook types to their respective contract addresses. - * Each hook type can be associated with a contract that implements the hook's logic. - * Only the contract owner can set or update these hooks. - */ + * @dev This mapping associates hook types with their corresponding contract addresses. + * Each hook type can be associated with a contract that implements the hook's logic. + * Only the contract owner can set or change these hooks. + */ mapping(HookType => address) public hooks; - - event UpdatedHook(address indexed setter, HookType hookType, address indexed hookAddress); - - - /// @notice Contract constructor - /// @param _contractName The name for the token contract - /// @param _contractSymbol The symbol for the token contract + /** + * @dev Contract constructor. + * @param _contractName The name of the token contract. + * @param _contractSymbol The symbol of the token contract. + */ constructor( string memory _contractName, string memory _contractSymbol @@ -47,9 +45,14 @@ contract ERC721ACH is ERC721AC, IERC721ACH { /// ERC721 overrides ///////////////////////////////////////////////// - - - /// TODO + /** + * @notice Before token transfer hook. This function is called before any token transfer. + * This includes minting and burning. + * @param from The source address. + * @param to The destination address. + * @param startTokenId The ID of the first token to be transferred. + * @param quantity The number of tokens to be transferred. + */ function _beforeTokenTransfers( address from, address to, @@ -57,33 +60,40 @@ contract ERC721ACH is ERC721AC, IERC721ACH { uint256 quantity ) internal virtual override { super._beforeTokenTransfers(from, to, startTokenId, quantity); - IBeforeTokenTransfersHook beforeTokenTransfersHook = IBeforeTokenTransfersHook(hooks[HookType.BeforeTokenTransfers]); - if ( - address(beforeTokenTransfersHook) != address(0) && - beforeTokenTransfersHook.useBeforeTokenTransfersHook(from, to, startTokenId, quantity) - ) { - beforeTokenTransfersHook.beforeTokenTransfersOverrideHook( - from, - to, - startTokenId, - quantity - ); - } + IBeforeTokenTransfersHook hook = IBeforeTokenTransfersHook( + hooks[HookType.BeforeTokenTransfers] + ); + if (address(hook) != address(0)) { + hook.beforeTokenTransfersHook(from, to, startTokenId, quantity); + } } - /// TODO + /** + * @notice After token transfer hook. This function is called after any token transfer. + * This includes minting and burning. + * @param from The source address. + * @param to The destination address. + * @param startTokenId The ID of the first token to be transferred. + * @param quantity The number of tokens to be transferred. + */ function _afterTokenTransfers( address from, address to, uint256 startTokenId, uint256 quantity ) internal virtual override { - super._afterTokenTransfers(from, to, startTokenId, quantity); - IAfterTokenTransfersHook afterTokenTransfersHook = IAfterTokenTransfersHook(hooks[HookType.AfterTokenTransfers]); + IAfterTokenTransfersHook afterTokenTransfersHook = IAfterTokenTransfersHook( + hooks[HookType.AfterTokenTransfers] + ); if ( address(afterTokenTransfersHook) != address(0) && - afterTokenTransfersHook.useAfterTokenTransfersHook(from, to, startTokenId, quantity) + afterTokenTransfersHook.useAfterTokenTransfersHook( + from, + to, + startTokenId, + quantity + ) ) { afterTokenTransfersHook.afterTokenTransfersOverrideHook( from, @@ -91,11 +101,18 @@ contract ERC721ACH is ERC721AC, IERC721ACH { startTokenId, quantity ); - } + } } - function ownerOf(uint256 tokenId) public view virtual override returns (address) { - + /** + * @notice Returns the owner of the `tokenId` token. + * @dev The owner of a token is also its approver by default. + * @param tokenId The ID of the token to query. + * @return owner of the `tokenId` token. + */ + function ownerOf( + uint256 tokenId + ) public view virtual override returns (address) { IOwnerOfHook ownerOfHook = IOwnerOfHook(hooks[HookType.OwnerOf]); if ( @@ -103,17 +120,16 @@ contract ERC721ACH is ERC721AC, IERC721ACH { ownerOfHook.useOwnerOfHook(tokenId) ) { return ownerOfHook.ownerOfOverrideHook(tokenId); - } - + } + return super.ownerOf(tokenId); } - /** - * @notice Returns the contract address for a specified hook type. - * @param hookType The type of hook to retrieve, as defined in the HookType enum. - * @return The address of the contract implementing the hook interface. - */ + * @notice Returns the address of the contract that implements the logic for the given hook type. + * @param hookType The type of the hook to query. + * @return address of the contract that implements the hook's logic. + */ function getHook(HookType hookType) external view returns (address) { return hooks[hookType]; } @@ -122,7 +138,10 @@ contract ERC721ACH is ERC721AC, IERC721ACH { /// ERC721C Override ///////////////////////////////////////////////// - /// @notice Override the function to throw if caller is not contract owner + /** + * @notice This internal function is used to ensure that the caller is the contract owner. + * @dev Throws if called by any account other than the owner. + */ function _requireCallerIsContractOwner() internal view virtual override {} ///////////////////////////////////////////////// @@ -130,18 +149,24 @@ contract ERC721ACH is ERC721AC, IERC721ACH { ///////////////////////////////////////////////// /** - * @notice Sets the contract address for a specified hook type. - * @param hookType The type of hook to set, as defined in the HookType enum. - * @param hookAddress The address of the contract implementing the hook interface. - */ - function setHook(HookType hookType, address hookAddress) external virtual onlyOwner { + * @notice Updates the contract address for a specific hook type. + * @dev Throws if called by any account other than the owner. + * Emits a {UpdatedHook} event. + * @param hookType The type of the hook to set. + * @param hookAddress The address of the contract that implements the hook's logic. + */ + function setHook( + HookType hookType, + address hookAddress + ) external virtual onlyOwner { hooks[hookType] = hookAddress; emit UpdatedHook(msg.sender, hookType, hookAddress); } - - - /// TODO + /** + * @notice This modifier checks if the caller is the contract owner. + * @dev Throws if called by any account other than the owner. + */ modifier onlyOwner() { _requireCallerIsContractOwner(); diff --git a/src/interfaces/IBeforeTokenTransfersHook.sol b/src/interfaces/IBeforeTokenTransfersHook.sol index d7e6bfc..ac5df05 100644 --- a/src/interfaces/IBeforeTokenTransfersHook.sol +++ b/src/interfaces/IBeforeTokenTransfersHook.sol @@ -4,14 +4,12 @@ pragma solidity ^0.8.15; /// @title IBeforeTokenTransfersHook /// @dev Interface that defines hooks to be executed before token transfers. interface IBeforeTokenTransfersHook { - /** - @notice Emitted when the before token transfers hook is used. - @param from Address from which the tokens are being transferred. - @param to Address to which the tokens are being transferred. - @param startTokenId The starting ID of the tokens being transferred. - @param quantity The number of tokens being transferred. - + * @notice Emitted when the before token transfers hook is used. + * @param from Address from which the tokens are being transferred. + * @param to Address to which the tokens are being transferred. + * @param startTokenId The starting ID of the tokens being transferred. + * @param quantity The number of tokens being transferred. */ event BeforeTokenTransfersHookUsed( address from, @@ -21,30 +19,13 @@ interface IBeforeTokenTransfersHook { ); /** - @notice Checks if the token transfers function should use the custom hook. - @param from Address from which the tokens are being transferred. - @param to Address to which the tokens are being transferred. - @param startTokenId The starting ID of the tokens being transferred. - @param quantity The number of tokens being transferred. - @return A boolean indicating whether or not to use the custom hook for the token transfers function. - - */ - function useBeforeTokenTransfersHook( - address from, - address to, - uint256 startTokenId, - uint256 quantity - ) external view returns (bool); - - /** - @notice Provides a custom implementation for the token transfers process. - @param from Address from which the tokens are being transferred. - @param to Address to which the tokens are being transferred. - @param startTokenId The starting ID of the tokens being transferred. - @param quantity The number of tokens being transferred. - + * @notice Provides a custom implementation for the token transfers process. + * @param from Address from which the tokens are being transferred. + * @param to Address to which the tokens are being transferred. + * @param startTokenId The starting ID of the tokens being transferred. + * @param quantity The number of tokens being transferred. */ - function beforeTokenTransfersOverrideHook( + function beforeTokenTransfersHook( address from, address to, uint256 startTokenId, diff --git a/src/interfaces/IERC721ACH.sol b/src/interfaces/IERC721ACH.sol index a0f7493..06b879d 100644 --- a/src/interfaces/IERC721ACH.sol +++ b/src/interfaces/IERC721ACH.sol @@ -1,23 +1,30 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.15; - interface IERC721ACH { - - /** - * @dev Enumerated list of all available hook types for the ERC721ACH contract. - */ - enum HookType { - + * @dev Enumerated list of all available hook types for the ERC721ACH contract. + */ + enum HookType { /// @notice Hook for custom logic before a token transfer occurs. BeforeTokenTransfers, /// @notice Hook for custom logic after a token transfer occurs. AfterTokenTransfers, /// @notice Hook for custom logic for ownerOf() function. - OwnerOf + OwnerOf } + /** + * @notice An event that gets emitted when a hook is updated. + * @param setter The address that set the hook. + * @param hookType The type of the hook that was set. + * @param hookAddress The address of the contract that implements the hook. + */ + event UpdatedHook( + address indexed setter, + HookType hookType, + address indexed hookAddress + ); /** * @notice Sets the contract address for a specified hook type. @@ -32,5 +39,4 @@ interface IERC721ACH { * @return The address of the contract implementing the hook interface. */ function getHook(HookType hookType) external view returns (address); - } diff --git a/test/hooks/BeforeTokenTransfers.t.sol b/test/hooks/BeforeTokenTransfers.t.sol index 88f3d92..7498c6c 100644 --- a/test/hooks/BeforeTokenTransfers.t.sol +++ b/test/hooks/BeforeTokenTransfers.t.sol @@ -8,7 +8,6 @@ import {IERC721A} from "lib/ERC721A/contracts/IERC721A.sol"; import {BeforeTokenTransfersHookMock} from "../utils/hooks/BeforeTokenTransfersHookMock.sol"; import {IERC721ACH} from "../../src/interfaces/IERC721ACH.sol"; - contract BeforeTokenTransfersHookTest is DSTest { Vm public constant vm = Vm(HEVM_ADDRESS); address public constant DEFAULT_OWNER_ADDRESS = address(0xC0FFEE); @@ -16,30 +15,36 @@ contract BeforeTokenTransfersHookTest is DSTest { ERC721ACHMock erc721Mock; BeforeTokenTransfersHookMock hookMock; - IERC721ACH.HookType constant BeforeTokenTransfers = IERC721ACH.HookType.BeforeTokenTransfers; + IERC721ACH.HookType constant BeforeTokenTransfers = + IERC721ACH.HookType.BeforeTokenTransfers; function setUp() public { erc721Mock = new ERC721ACHMock(DEFAULT_OWNER_ADDRESS); hookMock = new BeforeTokenTransfersHookMock(); } - function test_beforeTokenTransfersHook() public { + function test_getHook_BeforeTokenTransfers() public { assertEq(address(0), address(erc721Mock.getHook(BeforeTokenTransfers))); } function test_setBeforeTokenTransfersHook() public { assertEq(address(0), address(erc721Mock.getHook(BeforeTokenTransfers))); - // calling an admin function without being the contract owner should revert + // calling an admin function without being the contract owner should revert vm.expectRevert(); erc721Mock.setHook(BeforeTokenTransfers, address(hookMock)); - + vm.prank(DEFAULT_OWNER_ADDRESS); erc721Mock.setHook(BeforeTokenTransfers, address(hookMock)); - assertEq(address(hookMock), address(erc721Mock.getHook(BeforeTokenTransfers))); + assertEq( + address(hookMock), + address(erc721Mock.getHook(BeforeTokenTransfers)) + ); } function test_beforeTokenTransfersHook( + address _firstOwner, + address _secondOwner, uint256 startTokenId, uint256 quantity ) public { @@ -47,37 +52,47 @@ contract BeforeTokenTransfersHookTest is DSTest { vm.assume(startTokenId > 0); vm.assume(quantity < 10_000); vm.assume(quantity >= startTokenId); - + _assumeNotBurn(_firstOwner); // Mint some tokens first - test_setBeforeTokenTransfersHook(); - erc721Mock.mint(DEFAULT_BUYER_ADDRESS, quantity); - - - vm.prank(DEFAULT_BUYER_ADDRESS); - erc721Mock.transferFrom( - DEFAULT_BUYER_ADDRESS, - DEFAULT_OWNER_ADDRESS, - startTokenId - ); - - assertEq(DEFAULT_OWNER_ADDRESS, erc721Mock.ownerOf(startTokenId)); + erc721Mock.mint(_firstOwner, quantity); + _assertNormalTransfer(_firstOwner, _secondOwner, startTokenId); // Verify hook override - hookMock.setHooksEnabled(true); - vm.expectRevert( - BeforeTokenTransfersHookMock.BeforeTokenTransfersHook_Executed.selector - ); - vm.prank(DEFAULT_OWNER_ADDRESS); - erc721Mock.transferFrom( - DEFAULT_OWNER_ADDRESS, - DEFAULT_BUYER_ADDRESS, - startTokenId - ); + test_setBeforeTokenTransfersHook(); + _assertHookRevert(_secondOwner, _firstOwner, startTokenId); + } + function _assumeNotBurn(address _wallet) internal { + vm.assume(_wallet != address(0)); + } + + function _assertNormalTransfer( + address _from, + address _to, + uint256 _tokenId + ) public { + vm.prank(_from); + erc721Mock.transferFrom(_from, _to, _tokenId); + _assertOwner(_to, _tokenId); } - + function _assertOwner(address _owner, uint256 _token) internal { + assertEq(_owner, erc721Mock.ownerOf(_token)); + } + function _assertHookRevert( + address _from, + address _to, + uint256 _tokenId + ) internal { + vm.prank(_from); + vm.expectRevert( + BeforeTokenTransfersHookMock + .BeforeTokenTransfersHook_Executed + .selector + ); + erc721Mock.transferFrom(_from, _to, _tokenId); + } } diff --git a/test/utils/hooks/BeforeTokenTransfersHookMock.sol b/test/utils/hooks/BeforeTokenTransfersHookMock.sol index 8c5e47e..118347d 100644 --- a/test/utils/hooks/BeforeTokenTransfersHookMock.sol +++ b/test/utils/hooks/BeforeTokenTransfersHookMock.sol @@ -7,32 +7,13 @@ contract BeforeTokenTransfersHookMock is IBeforeTokenTransfersHook { /// @notice hook was executed error BeforeTokenTransfersHook_Executed(); - bool public hooksEnabled; - - /// @notice toggle balanceOf hook. - function setHooksEnabled(bool _enabled) public { - hooksEnabled = _enabled; - } - - /// @notice Check if the beforeTokenTransfers function should use hook. - /// @dev Returns whether or not to use the hook for beforeTokenTransfers function - function useBeforeTokenTransfersHook( - address, - address, - uint256, - uint256 - ) external view override returns (bool) { - return hooksEnabled; - } - - /// @notice custom implementation for beforeTokenTransfers Hook. - function beforeTokenTransfersOverrideHook( + function beforeTokenTransfersHook( address, address, uint256, uint256 - ) external pure override { + ) external pure override { revert BeforeTokenTransfersHook_Executed(); } }