Inherit ERC721 from OZ

Author: telome

.gitmodules

@@ -1,3 +1,6 @@
 [submodule "lib/dss-test"]
 	path = lib/dss-test
 	url = https://github.com/makerdao/dss-test
+[submodule "lib/openzeppelin-contracts"]
+	path = lib/openzeppelin-contracts
+	url = https://github.com/OpenZeppelin/openzeppelin-contracts

README.md

@@ -12,3 +12,7 @@ Below is a non-exhaustive list of deviations from [laniakea-docs](https://github
 - The spec requires complete withdrawal only for the queue. This implementation supports partial withdrawals.
 - The spec calls the minting operation `claim`. This implementation uses `issue` to avoid confusion with the redeem-side concept of claiming funded amounts.
 - The spec tracks queue deposits through a shares-based accounting system. Since the deposited asset earns no yield while queued, shares would always be 1:1 with the underlying, so this implementation tracks deposits as direct balances instead.
+
+## Notes
+
+- The contract inherits OpenZeppelin's ERC721 which advertises support for the ERC721Metadata extension via `supportsInterface`. However, no base URI is configured, so `tokenURI()` returns an empty string for all tokens.

lib/openzeppelin-contracts

@@ -16,6 +16,8 @@
 
 pragma solidity ^0.8.24;
 
+import { ERC721 } from "openzeppelin-contracts/contracts/token/ERC721/ERC721.sol";
+
 interface GemLike {
     function transferFrom(address from, address to, uint256 amount) external;
     function transfer(address to, uint256 amount) external;
@@ -25,19 +27,10 @@ interface IdentityNetworkLike {
     function isMember(address account) external view returns (bool);
 }
 
-interface ERC721ReceiverLike {
-    function onERC721Received(
-        address operator,
-        address from,
-        uint256 tokenId,
-        bytes calldata data
-    ) external returns (bytes4);
-}
-
 /// @title NFATFacility
 /// @notice Non-Fungible Allocation Token Facility for bespoke capital deployment deals
 /// @dev Implements queue-based deposits and ERC-721 NFAT minting
-contract NFATFacility {
+contract NFATFacility is ERC721 {
 
     // --- Immutables ---
 
@@ -62,13 +55,7 @@ contract NFATFacility {
 
     // --- NFAT Storage ---
 
-    string  public name;
-    string  public symbol;
     uint256 public nextTokenId;
-    mapping(uint256 tokenId => address owner)                              internal _owners;
-    mapping(address owner   => uint256 count)                              internal _balances;
-    mapping(uint256 tokenId => address approved)                           internal _tokenApprovals;
-    mapping(address owner   => mapping(address operator => bool approved)) internal _operatorApprovals;
 
     // --- Events: Access Control ---
 
@@ -93,12 +80,6 @@ contract NFATFacility {
     event Fund(uint256 indexed tokenId, address indexed funder, uint256 amount);
     event Redeem(uint256 indexed tokenId, uint256 amount);
 
-    // --- Events: ERC-721 ---
-
-    event Transfer(address indexed from, address indexed to, uint256 indexed tokenId);
-    event Approval(address indexed owner, address indexed approved, uint256 indexed tokenId);
-    event ApprovalForAll(address indexed owner, address indexed operator, bool approved);
-
     // --- Modifiers ---
 
     modifier auth() {
@@ -123,11 +104,11 @@ contract NFATFacility {
 
     // --- Constructor ---
 
-    constructor(address gem_, address almProxy_, string memory name_, string memory symbol_) {
+    constructor(address gem_, address almProxy_, string memory name_, string memory symbol_)
+        ERC721(name_, symbol_)
+    {
         gem = GemLike(gem_);
         almProxy = almProxy_;
-        name   = name_;
-        symbol = symbol_;
         wards[msg.sender] = 1;
         emit Rely(msg.sender);
     }
@@ -218,22 +199,18 @@ contract NFATFacility {
         require(amount > 0, "NFATFacility/zero-amount");
         require(deposits[target] >= amount, "NFATFacility/insufficient-deposits");
 
-        require(identityNetwork == address(0) || IdentityNetworkLike(identityNetwork).isMember(target), "NFATFacility/target-not-member");
-
         uint256 tokenId = nextTokenId++;
 
         // Effects - Queue
         unchecked { deposits[target] -= amount; }
 
-        // Effects - NFAT
-        _owners[tokenId] = target;
-        _balances[target] += 1;
+        // Effects - NFAT (identity network check in _update)
+        _mint(target, tokenId);
 
         // Interactions
         gem.transfer(almProxy, amount);
 
         emit Issue(target, tokenId, amount);
-        emit Transfer(address(0), target, tokenId);
     }
 
     // --- Redeem Functions ---
@@ -242,7 +219,7 @@ contract NFATFacility {
     /// @param tokenId The NFAT to fund
     /// @param amount The amount of gem to deposit
     function fund(uint256 tokenId, uint256 amount) external {
-        require(_owners[tokenId] != address(0), "NFATFacility/invalid-token");
+        require(_ownerOf(tokenId) != address(0), "NFATFacility/invalid-token");
         require(amount > 0, "NFATFacility/zero-amount");
 
         // Effects
@@ -261,7 +238,7 @@ contract NFATFacility {
         require(amount > 0, "NFATFacility/zero-amount");
         require(funded[tokenId] >= amount, "NFATFacility/insufficient-funded");
 
-        address owner = _owners[tokenId];
+        address owner = _ownerOf(tokenId);
         require(msg.sender == owner, "NFATFacility/not-owner");
         require(identityNetwork == address(0) || IdentityNetworkLike(identityNetwork).isMember(owner), "NFATFacility/not-member");
 
@@ -274,89 +251,15 @@ contract NFATFacility {
         emit Redeem(tokenId, amount);
     }
 
-    // --- ERC-721 Functions ---
-
-    function ownerOf(uint256 tokenId) public view returns (address) {
-        address owner = _owners[tokenId];
-        require(owner != address(0), "NFATFacility/invalid-token");
-        return owner;
-    }
-
-    function balanceOf(address owner) external view returns (uint256) {
-        require(owner != address(0), "NFATFacility/zero-address");
-        return _balances[owner];
-    }
-
-    function approve(address to, uint256 tokenId) external {
-        address owner = ownerOf(tokenId);
-        require(msg.sender == owner || _operatorApprovals[owner][msg.sender], "NFATFacility/not-authorized");
-        _tokenApprovals[tokenId] = to;
-        emit Approval(owner, to, tokenId);
-    }
-
-    function getApproved(uint256 tokenId) external view returns (address) {
-        require(_owners[tokenId] != address(0), "NFATFacility/invalid-token");
-        return _tokenApprovals[tokenId];
-    }
-
-    function setApprovalForAll(address operator, bool approved) external {
-        require(operator != msg.sender, "NFATFacility/self-approval");
-        _operatorApprovals[msg.sender][operator] = approved;
-        emit ApprovalForAll(msg.sender, operator, approved);
-    }
-
-    function isApprovedForAll(address owner, address operator) external view returns (bool) {
-        return _operatorApprovals[owner][operator];
-    }
-
-    function transferFrom(address from, address to, uint256 tokenId) public {
-        require(_isApprovedOrOwner(msg.sender, tokenId), "NFATFacility/not-authorized");
-        require(ownerOf(tokenId) == from, "NFATFacility/wrong-from");
-        require(to != address(0), "NFATFacility/zero-address");
-
-        require(identityNetwork == address(0) || IdentityNetworkLike(identityNetwork).isMember(to), "NFATFacility/to-not-member");
-
-        // Clear approval
-        _tokenApprovals[tokenId] = address(0);
-
-        // Effects
-        _balances[from] -= 1;
-        _balances[to] += 1;
-        _owners[tokenId] = to;
-
-        emit Transfer(from, to, tokenId);
-    }
-
-    function safeTransferFrom(address from, address to, uint256 tokenId) external {
-        safeTransferFrom(from, to, tokenId, "");
-    }
-
-    function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public {
-        transferFrom(from, to, tokenId);
-        require(_checkOnERC721Received(from, to, tokenId, data), "NFATFacility/unsafe-recipient");
-    }
-
-    function _isApprovedOrOwner(address spender, uint256 tokenId) internal view returns (bool) {
-        address owner = ownerOf(tokenId);
-        return (spender == owner || _tokenApprovals[tokenId] == spender || _operatorApprovals[owner][spender]);
-    }
-
-    function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory data) internal returns (bool) {
-        if (to.code.length == 0) {
-            return true;
-        }
-        try ERC721ReceiverLike(to).onERC721Received(msg.sender, from, tokenId, data) returns (bytes4 retval) {
-            return retval == ERC721ReceiverLike.onERC721Received.selector;
-        } catch {
-            return false;
-        }
-    }
-
-    // --- ERC-165 ---
+    // --- ERC-721 Overrides ---
 
-    function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
-        return
-            interfaceId == 0x01ffc9a7 || // ERC-165
-            interfaceId == 0x80ac58cd;   // ERC-721
+    /// @dev OZ's _mint and transferFrom both revert before calling _update when to == address(0),
+    ///      and _burn is never invoked, so `to` is guaranteed to be non-zero here.
+    function _update(address to, uint256 tokenId, address auth_) internal override returns (address) {
+        require(
+            identityNetwork == address(0) || IdentityNetworkLike(identityNetwork).isMember(to),
+            "NFATFacility/not-member"
+        );
+        return super._update(to, tokenId, auth_);
     }
 }

src/NFATFacility.sol

@@ -5,6 +5,7 @@ import "dss-test/DssTest.sol";
 import { NFATFacility } from "src/NFATFacility.sol";
 import { NFATDeploy } from "deploy/NFATDeploy.sol";
 import { NFATInit, NFATConfig } from "deploy/NFATInit.sol";
+import { IERC721Errors } from "openzeppelin-contracts/contracts/interfaces/draft-IERC6093.sol";
 
 interface SUsdsLike {
     function balanceOf(address) external view returns (uint256);
@@ -265,9 +266,9 @@ contract NFATFacilityTest is DssTest {
 
         // First issue
         vm.expectEmit(true, true, true, true);
-        emit Issue(prime1, 0, 60 ether);
-        vm.expectEmit(true, true, true, true);
         emit Transfer(address(0), prime1, 0);
+        vm.expectEmit(true, true, true, true);
+        emit Issue(prime1, 0, 60 ether);
         uint256 tokenId = _issue(prime1, 60 ether);
 
         assertEq(tokenId, 0);
@@ -322,7 +323,7 @@ contract NFATFacilityTest is DssTest {
 
         _subscribe(prime1, 100 ether);
 
-        vm.expectRevert("NFATFacility/target-not-member");
+        vm.expectRevert("NFATFacility/not-member");
         vm.prank(operator); facility.issue(prime1, 50 ether);
     }
 
@@ -465,23 +466,23 @@ contract NFATFacilityTest is DssTest {
         _subscribe(prime1, 100 ether);
         uint256 tokenId = _issue(prime1, 100 ether);
 
-        vm.expectRevert("NFATFacility/not-authorized");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InsufficientApproval.selector, prime2, tokenId));
         vm.prank(prime2); facility.transferFrom(prime1, prime2, tokenId);
     }
 
     function testRevertTransferFromWrongFrom() public {
         _subscribe(prime1, 100 ether);
         uint256 tokenId = _issue(prime1, 100 ether);
 
-        vm.expectRevert("NFATFacility/wrong-from");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721IncorrectOwner.selector, prime2, tokenId, prime1));
         vm.prank(prime1); facility.transferFrom(prime2, prime2, tokenId);
     }
 
     function testRevertTransferFromZeroAddress() public {
         _subscribe(prime1, 100 ether);
         uint256 tokenId = _issue(prime1, 100 ether);
 
-        vm.expectRevert("NFATFacility/zero-address");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidReceiver.selector, address(0)));
         vm.prank(prime1); facility.transferFrom(prime1, address(0), tokenId);
     }
 
@@ -495,7 +496,7 @@ contract NFATFacilityTest is DssTest {
 
         // Reverts when `to` is not a member
         idNet.setMember(prime2, false);
-        vm.expectRevert("NFATFacility/to-not-member");
+        vm.expectRevert("NFATFacility/not-member");
         vm.prank(prime1); facility.transferFrom(prime1, prime2, tokenId);
 
         // Succeeds when `to` is a member
@@ -528,12 +529,12 @@ contract NFATFacilityTest is DssTest {
 
         // Bad return value
         uint256 tokenId0 = _issue(prime1, 50 ether);
-        vm.expectRevert("NFATFacility/unsafe-recipient");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidReceiver.selector, address(badReceiver)));
         vm.prank(prime1); facility.safeTransferFrom(prime1, address(badReceiver), tokenId0);
 
         // No onERC721Received at all
         uint256 tokenId1 = _issue(prime1, 50 ether);
-        vm.expectRevert("NFATFacility/unsafe-recipient");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidReceiver.selector, address(facility)));
         vm.prank(prime1); facility.safeTransferFrom(prime1, address(facility), tokenId1);
     }
 
@@ -557,7 +558,7 @@ contract NFATFacilityTest is DssTest {
         _subscribe(prime1, 100 ether);
         uint256 tokenId = _issue(prime1, 100 ether);
 
-        vm.expectRevert("NFATFacility/not-authorized");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidApprover.selector, prime2));
         vm.prank(prime2); facility.approve(prime2, tokenId);
     }
 
@@ -569,23 +570,23 @@ contract NFATFacilityTest is DssTest {
         assertTrue(facility.isApprovedForAll(prime1, prime2));
     }
 
-    function testRevertSetApprovalForAllSelf() public {
-        vm.expectRevert("NFATFacility/self-approval");
-        vm.prank(prime1); facility.setApprovalForAll(prime1, true);
+    function testRevertSetApprovalForAllZeroAddress() public {
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidOperator.selector, address(0)));
+        vm.prank(prime1); facility.setApprovalForAll(address(0), true);
     }
 
     function testRevertOwnerOfInvalidToken() public {
-        vm.expectRevert("NFATFacility/invalid-token");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721NonexistentToken.selector, uint256(999)));
         facility.ownerOf(999);
     }
 
     function testRevertBalanceOfZeroAddress() public {
-        vm.expectRevert("NFATFacility/zero-address");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721InvalidOwner.selector, address(0)));
         facility.balanceOf(address(0));
     }
 
     function testRevertGetApprovedInvalidToken() public {
-        vm.expectRevert("NFATFacility/invalid-token");
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721NonexistentToken.selector, uint256(999)));
         facility.getApproved(999);
     }