feat(ctb): remove unnecessary ERC721 factory code

First removes the legacy bridge and remoteChainId getter functions. Since the ERC721 factory is being deployed at a new address, it's not necessary to maintain ABI compatibility. We can safely remove these functions. Also removes constructor checks that aren't really necessary since we can always upgrade if needed and the odds of getting something like that wrong don't seem significant enough to warrant extra code.
sam-iamm · Jan 5, 2023 · 2d1d95e · 2d1d95e
1 parent ab8cf2e
commit 2d1d95e
Show file tree

Hide file tree

Showing 6 changed files with 8 additions and 112 deletions.
diff --git a/integration-tests/test/nft-bridge.spec.ts b/integration-tests/test/nft-bridge.spec.ts
@@ -108,7 +108,7 @@ describe('ERC721 Bridge', () => {
     expect(await L1ERC721Bridge.otherBridge()).to.equal(L2ERC721Bridge.address)
     expect(await L2ERC721Bridge.otherBridge()).to.equal(L1ERC721Bridge.address)
 
-    expect(await OptimismMintableERC721Factory.bridge()).to.equal(
+    expect(await OptimismMintableERC721Factory.BRIDGE()).to.equal(
       L2ERC721Bridge.address
     )
 

diff --git a/op-bindings/bindings/optimismmintableerc721factory.go b/op-bindings/bindings/optimismmintableerc721factory.go
diff --git a/op-bindings/bindings/optimismmintableerc721factory_more.go b/op-bindings/bindings/optimismmintableerc721factory_more.go
diff --git a/packages/contracts-bedrock/.gas-snapshot b/packages/contracts-bedrock/.gas-snapshot
@@ -236,11 +236,9 @@ OptimismMintableERC721_Test:test_constructor_succeeds() (gas: 24162)
 OptimismMintableERC721_Test:test_safeMint_notBridge_reverts() (gas: 11142)
 OptimismMintableERC721_Test:test_safeMint_succeeds() (gas: 140502)
 OptimismMintableERC721_Test:test_tokenURI_succeeds() (gas: 163420)
-OptimismMintableERC721Factory_Test:test_constructor_succeeds() (gas: 10005)
-OptimismMintableERC721Factory_Test:test_constructor_zeroBridge_reverts() (gas: 39114)
-OptimismMintableERC721Factory_Test:test_constructor_zeroRemoteChainId_reverts() (gas: 41287)
-OptimismMintableERC721Factory_Test:test_createOptimismMintableERC721_succeeds() (gas: 2276374)
-OptimismMintableERC721Factory_Test:test_createOptimismMintableERC721_zeroRemoteToken_reverts() (gas: 9373)
+OptimismMintableERC721Factory_Test:test_constructor_succeeds() (gas: 8262)
+OptimismMintableERC721Factory_Test:test_createOptimismMintableERC721_succeeds() (gas: 2276440)
+OptimismMintableERC721Factory_Test:test_createOptimismMintableERC721_zeroRemoteToken_reverts() (gas: 9395)
 OptimismPortalUpgradeable_Test:test_initialize_cannotInitImpl_reverts() (gas: 10791)
 OptimismPortalUpgradeable_Test:test_initialize_cannotInitProxy_reverts() (gas: 15833)
 OptimismPortalUpgradeable_Test:test_params_initValuesOnProxy_succeeds() (gas: 16011)

diff --git a/packages/contracts-bedrock/contracts/test/OptimismMintableERC721Factory.t.sol b/packages/contracts-bedrock/contracts/test/OptimismMintableERC721Factory.t.sol
@@ -27,22 +27,10 @@ contract OptimismMintableERC721Factory_Test is ERC721Bridge_Initializer {
     }
 
     function test_constructor_succeeds() external {
-        assertEq(factory.bridge(), address(L2Bridge));
-        assertEq(factory.remoteChainId(), 1);
         assertEq(factory.BRIDGE(), address(L2Bridge));
         assertEq(factory.REMOTE_CHAIN_ID(), 1);
     }
 
-    function test_constructor_zeroBridge_reverts() external {
-        vm.expectRevert("OptimismMintableERC721Factory: bridge cannot be address(0)");
-        new OptimismMintableERC721Factory(address(0), 1);
-    }
-
-    function test_constructor_zeroRemoteChainId_reverts() external {
-        vm.expectRevert("OptimismMintableERC721Factory: remote chain id cannot be zero");
-        new OptimismMintableERC721Factory(address(L2Bridge), 0);
-    }
-
     function test_createOptimismMintableERC721_succeeds() external {
         // Predict the address based on the factory address and nonce.
         address predicted = LibRLP.computeAddress(address(factory), 1);

diff --git a/packages/contracts-bedrock/contracts/universal/OptimismMintableERC721Factory.sol b/packages/contracts-bedrock/contracts/universal/OptimismMintableERC721Factory.sol
@@ -41,41 +41,13 @@ contract OptimismMintableERC721Factory is Semver {
      * @custom:semver 1.0.0
      *
      * @param _bridge Address of the ERC721 bridge on this network.
+     * @param _remoteChainId Chain ID for the remote network.
      */
     constructor(address _bridge, uint256 _remoteChainId) Semver(1, 0, 0) {
-        require(
-            _bridge != address(0),
-            "OptimismMintableERC721Factory: bridge cannot be address(0)"
-        );
-        require(
-            _remoteChainId != 0,
-            "OptimismMintableERC721Factory: remote chain id cannot be zero"
-        );
-
         BRIDGE = _bridge;
         REMOTE_CHAIN_ID = _remoteChainId;
     }
 
-    /**
-     * @custom:legacy
-     * @notice Legacy getter for ERC721 bridge address.
-     *
-     * @return Address of the ERC721 bridge on this network.
-     */
-    function bridge() external view returns (address) {
-        return BRIDGE;
-    }
-
-    /**
-     * @custom:legacy
-     * @notice Legacy getter for remote chain ID.
-     *
-     * @notice Chain ID for the remote network.
-     */
-    function remoteChainId() external view returns (uint256) {
-        return REMOTE_CHAIN_ID;
-    }
-
     /**
      * @notice Creates an instance of the standard ERC721.
      *