fix(ctp): bugs in Mintable ERC721 (#2864)

Fixes two minor bugs in the Optimism Mintable ERC721 implementation. Includes a new required input (remoteChainId) and fixes two issues with the generated baseURI. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ethereum-optimism · Jun 29, 2022 · 8a335b7 · 8a335b7
1 parent a350046
commit 8a335b7
Show file tree

Hide file tree

Showing 9 changed files with 59 additions and 27 deletions.
diff --git a/.changeset/strange-pets-draw.md b/.changeset/strange-pets-draw.md
@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': minor
+---
+
+Fixes a bug in the OptimismMintableERC721. Requires an interface change, so this is a minor and not patch.
diff --git a/integration-tests/test/nft-bridge.spec.ts b/integration-tests/test/nft-bridge.spec.ts
@@ -1,6 +1,7 @@
 /* Imports: External */
 import { Contract, ContractFactory, utils, Wallet } from 'ethers'
 import { ethers } from 'hardhat'
+import { getChainId } from '@eth-optimism/core-utils'
 import { predeploys } from '@eth-optimism/contracts'
 import Artifact__TestERC721 from '@eth-optimism/contracts-periphery/artifacts/contracts/testing/helpers/TestERC721.sol/TestERC721.json'
 import Artifact__L1ERC721Bridge from '@eth-optimism/contracts-periphery/artifacts/contracts/L1/messaging/L1ERC721Bridge.sol/L1ERC721Bridge.json'
@@ -99,7 +100,8 @@ describe('ERC721 Bridge', () => {
 
     OptimismMintableERC721Factory =
       await Factory__OptimismMintableERC721Factory.deploy(
-        L2ERC721Bridge.address
+        L2ERC721Bridge.address,
+        await getChainId(env.l1Wallet.provider)
       )
     await OptimismMintableERC721Factory.deployed()
 
@@ -268,7 +270,10 @@ describe('ERC721 Bridge', () => {
       // - Returns the address of the legitimate L1 token from its l1Token() getter.
       // - Allows the L2 bridge to call its burn() function.
       const FakeOptimismMintableERC721 = await (
-        await ethers.getContractFactory('FakeOptimismMintableERC721', bobWalletL2)
+        await ethers.getContractFactory(
+          'FakeOptimismMintableERC721',
+          bobWalletL2
+        )
       ).deploy(L1ERC721.address, L2ERC721Bridge.address)
       await FakeOptimismMintableERC721.deployed()
 

diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol b/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol
@@ -25,6 +25,11 @@ interface IOptimismMintableERC721 is IERC721 {
      */
     event Burn(address indexed account, uint256 tokenId);
 
+    /**
+     * @notice Chain ID of the chain where the remote token is deployed.
+     */
+    function remoteChainId() external returns (uint256);
+
     /**
      * @notice Address of the token on the remote domain.
      */

diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol
@@ -16,30 +16,38 @@ contract OptimismMintableERC721 is ERC721, IOptimismMintableERC721 {
     /**
      * @inheritdoc IOptimismMintableERC721
      */
-    address public remoteToken;
+    uint256 public immutable remoteChainId;
 
     /**
      * @inheritdoc IOptimismMintableERC721
      */
-    address public bridge;
+    address public immutable remoteToken;
+
+    /**
+     * @inheritdoc IOptimismMintableERC721
+     */
+    address public immutable bridge;
 
     /**
      * @notice Base token URI for this token.
      */
     string public baseTokenURI;
 
     /**
-     * @param _bridge      Address of the bridge on this network.
-     * @param _remoteToken Address of the corresponding token on the other network.
-     * @param _name        ERC721 name.
-     * @param _symbol      ERC721 symbol.
+     * @param _bridge        Address of the bridge on this network.
+     * @param _remoteChainId Chain ID where the remote token is deployed.
+     * @param _remoteToken   Address of the corresponding token on the other network.
+     * @param _name          ERC721 name.
+     * @param _symbol        ERC721 symbol.
      */
     constructor(
         address _bridge,
+        uint256 _remoteChainId,
         address _remoteToken,
         string memory _name,
         string memory _symbol
     ) ERC721(_name, _symbol) {
+        remoteChainId = _remoteChainId;
         remoteToken = _remoteToken;
         bridge = _bridge;
 
@@ -48,9 +56,9 @@ contract OptimismMintableERC721 is ERC721, IOptimismMintableERC721 {
         baseTokenURI = string(
             abi.encodePacked(
                 "ethereum:",
-                Strings.toHexString(uint160(_remoteToken)),
+                Strings.toHexString(uint160(_remoteToken), 20),
                 "@",
-                Strings.toString(block.chainid),
+                Strings.toString(_remoteChainId),
                 "/tokenURI?uint256="
             )
         );

diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol
@@ -14,7 +14,7 @@ contract OptimismMintableERC721Factory is OwnableUpgradeable {
     /**
      * @notice Contract version number.
      */
-    uint8 public constant VERSION = 1;
+    uint8 public constant VERSION = 2;
 
     /**
      * @notice Emitted whenever a new OptimismMintableERC721 contract is created.
@@ -29,6 +29,11 @@ contract OptimismMintableERC721Factory is OwnableUpgradeable {
      */
     address public bridge;
 
+    /**
+     * @notice Chain ID for the remote network.
+     */
+    uint256 public remoteChainId;
+
     /**
      * @notice Tracks addresses created by this factory.
      */
@@ -37,17 +42,18 @@ contract OptimismMintableERC721Factory is OwnableUpgradeable {
     /**
      * @param _bridge Address of the ERC721 bridge on this network.
      */
-    constructor(address _bridge) {
-        initialize(_bridge);
+    constructor(address _bridge, uint256 _remoteChainId) {
+        initialize(_bridge, _remoteChainId);
     }
 
     /**
      * @notice Initializes the factory.
      *
      * @param _bridge Address of the ERC721 bridge on this network.
      */
-    function initialize(address _bridge) public reinitializer(VERSION) {
+    function initialize(address _bridge, uint256 _remoteChainId) public reinitializer(VERSION) {
         bridge = _bridge;
+        remoteChainId = _remoteChainId;
 
         // Initialize upgradable OZ contracts
         __Ownable_init();
@@ -57,8 +63,8 @@ contract OptimismMintableERC721Factory is OwnableUpgradeable {
      * @notice Creates an instance of the standard ERC721.
      *
      * @param _remoteToken Address of the corresponding token on the other domain.
-     * @param _name ERC721 name.
-     * @param _symbol ERC721 symbol.
+     * @param _name        ERC721 name.
+     * @param _symbol      ERC721 symbol.
      */
     function createStandardOptimismMintableERC721(
         address _remoteToken,
@@ -69,13 +75,15 @@ contract OptimismMintableERC721Factory is OwnableUpgradeable {
             _remoteToken != address(0),
             "OptimismMintableERC721Factory: L1 token address cannot be address(0)"
         );
+
         require(
             bridge != address(0),
             "OptimismMintableERC721Factory: bridge address must be initialized"
         );
 
         OptimismMintableERC721 localToken = new OptimismMintableERC721(
             bridge,
+            remoteChainId,
             _remoteToken,
             _name,
             _symbol

diff --git a/...ages/contracts-periphery/deploy/nft-bridge/OptimismMintableERC721FactoryImplementation.ts b/...ages/contracts-periphery/deploy/nft-bridge/OptimismMintableERC721FactoryImplementation.ts
@@ -7,7 +7,7 @@ const deployFn: DeployFunction = async (hre) => {
 
   await hre.deployments.deploy('OptimismMintableERC721Factory', {
     from: deployer,
-    args: [ethers.constants.AddressZero],
+    args: [ethers.constants.AddressZero, 0],
     log: true,
   })
 }

diff --git a/packages/contracts-periphery/test/contracts/L2/messaging/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/messaging/L2ERC721Bridge.spec.ts
@@ -60,6 +60,7 @@ describe('L2ERC721Bridge', () => {
       await ethers.getContractFactory('OptimismMintableERC721')
     ).deploy(
       L2ERC721Bridge.address,
+      100,
       DUMMY_L1ERC721_ADDRESS,
       'L2Token',
       'L2T',
@@ -225,6 +226,7 @@ describe('L2ERC721Bridge', () => {
         await smock.mock('OptimismMintableERC721')
       ).deploy(
         L2ERC721Bridge.address,
+        100,
         DUMMY_L1ERC721_ADDRESS,
         'L2Token',
         'L2T',

diff --git a/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts b/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts
@@ -8,7 +8,7 @@ import { expect } from '../../setup'
 
 const TOKEN_ID = 10
 const DUMMY_L1ERC721_ADDRESS: string =
-  '0x2234223412342234223422342234223422342234'
+  '0x0034223412342234223422342234223422342234'
 
 describe('OptimismMintableERC721', () => {
   let l2BridgeImpersonator: Signer
@@ -18,26 +18,26 @@ describe('OptimismMintableERC721', () => {
   let l2BridgeImpersonatorAddress: string
   let aliceAddress: string
   let baseUri: string
-  let chainId: number
+  const remoteChainId = 100
 
   before(async () => {
     ;[l2BridgeImpersonator, alice] = await ethers.getSigners()
     l2BridgeImpersonatorAddress = await l2BridgeImpersonator.getAddress()
     aliceAddress = await alice.getAddress()
 
-    chainId = await alice.getChainId()
     baseUri = ''.concat(
       'ethereum:',
       DUMMY_L1ERC721_ADDRESS,
       '@',
-      chainId.toString(),
+      remoteChainId.toString(),
       '/tokenURI?uint256='
     )
 
     OptimismMintableERC721 = await (
       await ethers.getContractFactory('OptimismMintableERC721')
     ).deploy(
       l2BridgeImpersonatorAddress,
+      remoteChainId,
       DUMMY_L1ERC721_ADDRESS,
       'L2ERC721',
       'ERC',
@@ -101,8 +101,8 @@ describe('OptimismMintableERC721', () => {
       expect(await OptimismMintableERC721.supportsInterface(0x01ffc9a7)).to.be
         .true
 
-      // OptimismMintablERC721
-      expect(await OptimismMintableERC721.supportsInterface(0xec4fc8e3)).to.be
+      // OptimismMintableERC721
+      expect(await OptimismMintableERC721.supportsInterface(0x051e4975)).to.be
         .true
 
       // ERC721

diff --git a/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721Factory.spec.ts b/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721Factory.spec.ts
@@ -20,7 +20,7 @@ describe('OptimismMintableERC721Factory', () => {
   let L1ERC721: MockContract<Contract>
   let OptimismMintableERC721Factory: Contract
   let baseURI: string
-  let chainId: number
+  const remoteChainId = 100
 
   beforeEach(async () => {
     ;[signer] = await ethers.getSigners()
@@ -33,14 +33,13 @@ describe('OptimismMintableERC721Factory', () => {
 
     OptimismMintableERC721Factory = await (
       await ethers.getContractFactory('OptimismMintableERC721Factory')
-    ).deploy(DUMMY_L2_BRIDGE_ADDRESS)
+    ).deploy(DUMMY_L2_BRIDGE_ADDRESS, remoteChainId)
 
-    chainId = await signer.getChainId()
     baseURI = ''.concat(
       'ethereum:',
       L1ERC721.address.toLowerCase(),
       '@',
-      chainId.toString(),
+      remoteChainId.toString(),
       '/tokenURI?uint256='
     )
   })