Refactor ERC165 to use function overriding instead of storage (#2505)

Co-authored-by: Francisco Giordano <[email protected]>

CHANGELOG.md

@@ -8,6 +8,7 @@
  * `Strings`: addition of a `toHexString` function.  ([#2504](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2504))
  * `EnumerableMap`: change implementation to optimize for `key → value` lookups instead of enumeration. ([#2518](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2518))
  * `GSN`: Deprecate GSNv1 support in favor of upcomming support for GSNv2. ([#2521](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2521))
+ * `ERC165`: Remove uses of storage in the base ERC165 implementation. ERC165 based contracts now use storage-less virtual functions. Old behaviour remains available in the `ERC165Storage` extension. ([#2505](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2505))
 
 ## 3.4.0 (2021-02-02)
 

contracts/introspection/ERC165.sol

@@ -7,43 +7,22 @@ import "./IERC165.sol";
 /**
  * @dev Implementation of the {IERC165} interface.
  *
- * Contracts may inherit from this and call {_registerInterface} to declare
- * their support of an interface.
+ * Contracts that want to implement ERC165 should inherit from this contract and override {supportsInterface} to check
+ * for the additional interface id that will be supported. For example:
+ *
+ * ```solidity
+ * function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
+ *     return interfaceId == type(MyInterface).interfaceId || super.supportsInterface(interfaceId);
+ * }
+ * ```
+ *
+ * Alternatively, {ERC165Storage} provides an easier to use but more expensive implementation.
  */
 abstract contract ERC165 is IERC165 {
-    /**
-     * @dev Mapping of interface ids to whether or not it's supported.
-     */
-    mapping(bytes4 => bool) private _supportedInterfaces;
-
-    constructor () {
-        // Derived contracts need only register support for their own interfaces,
-        // we register support for ERC165 itself here
-        _registerInterface(type(IERC165).interfaceId);
-    }
-
     /**
      * @dev See {IERC165-supportsInterface}.
-     *
-     * Time complexity O(1), guaranteed to always use less than 30 000 gas.
      */
     function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
-        return _supportedInterfaces[interfaceId];
-    }
-
-    /**
-     * @dev Registers the contract as an implementer of the interface defined by
-     * `interfaceId`. Support of the actual ERC165 interface is automatic and
-     * registering its interface id is not required.
-     *
-     * See {IERC165-supportsInterface}.
-     *
-     * Requirements:
-     *
-     * - `interfaceId` cannot be the ERC165 invalid interface (`0xffffffff`).
-     */
-    function _registerInterface(bytes4 interfaceId) internal virtual {
-        require(interfaceId != 0xffffffff, "ERC165: invalid interface id");
-        _supportedInterfaces[interfaceId] = true;
+        return interfaceId == type(IERC165).interfaceId;
     }
 }

contracts/introspection/ERC165Storage.sol

@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+import "./ERC165.sol";
+
+/**
+ * @dev Storage based implementation of the {IERC165} interface.
+ *
+ * Contracts may inherit from this and call {_registerInterface} to declare
+ * their support of an interface.
+ */
+abstract contract ERC165Storage is ERC165 {
+    /**
+     * @dev Mapping of interface ids to whether or not it's supported.
+     */
+    mapping(bytes4 => bool) private _supportedInterfaces;
+
+    /**
+     * @dev See {IERC165-supportsInterface}.
+     */
+    function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
+        return super.supportsInterface(interfaceId) || _supportedInterfaces[interfaceId];
+    }
+
+    /**
+     * @dev Registers the contract as an implementer of the interface defined by
+     * `interfaceId`. Support of the actual ERC165 interface is automatic and
+     * registering its interface id is not required.
+     *
+     * See {IERC165-supportsInterface}.
+     *
+     * Requirements:
+     *
+     * - `interfaceId` cannot be the ERC165 invalid interface (`0xffffffff`).
+     */
+    function _registerInterface(bytes4 interfaceId) internal virtual {
+        require(interfaceId != 0xffffffff, "ERC165: invalid interface id");
+        _supportedInterfaces[interfaceId] = true;
+    }
+}

contracts/introspection/README.adoc

@@ -20,6 +20,8 @@ Note that, in all cases, accounts simply _declare_ their interfaces, but they ar
 
 {{ERC165}}
 
+{{ERC165Storage}}
+
 {{ERC165Checker}}
 
 == Global

contracts/mocks/ERC1155ReceiverMock.sol

@@ -2,10 +2,10 @@
 
 pragma solidity ^0.8.0;
 
+import "../introspection/ERC165.sol";
 import "../token/ERC1155/IERC1155Receiver.sol";
-import "./ERC165Mock.sol";
 
-contract ERC1155ReceiverMock is IERC1155Receiver, ERC165Mock {
+contract ERC1155ReceiverMock is IERC1155Receiver, ERC165 {
     bytes4 private _recRetval;
     bool private _recReverts;
     bytes4 private _batRetval;

contracts/mocks/ERC165Mock.sol

@@ -5,7 +5,4 @@ pragma solidity ^0.8.0;
 import "../introspection/ERC165.sol";
 
 contract ERC165Mock is ERC165 {
-    function registerInterface(bytes4 interfaceId) public {
-        _registerInterface(interfaceId);
-    }
 }

contracts/mocks/ERC165StorageMock.sol

@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+import "../introspection/ERC165Storage.sol";
+
+contract ERC165StorageMock is ERC165Storage {
+    function registerInterface(bytes4 interfaceId) public {
+        _registerInterface(interfaceId);
+    }
+}

contracts/token/ERC1155/ERC1155.sol

@@ -34,12 +34,15 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI {
      */
     constructor (string memory uri_) {
         _setURI(uri_);
+    }
 
-        // register the supported interfaces to conform to ERC1155 via ERC165
-        _registerInterface(type(IERC1155).interfaceId);
-
-        // register the supported interfaces to conform to ERC1155MetadataURI via ERC165
-        _registerInterface(type(IERC1155MetadataURI).interfaceId);
+    /**
+     * @dev See {IERC165-supportsInterface}.
+     */
+    function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) {
+        return interfaceId == type(IERC1155).interfaceId
+            || interfaceId == type(IERC1155MetadataURI).interfaceId
+            || super.supportsInterface(interfaceId);
     }
 
     /**

contracts/token/ERC1155/ERC1155Receiver.sol

@@ -9,7 +9,11 @@ import "../../introspection/ERC165.sol";
  * @dev _Available since v3.1._
  */
 abstract contract ERC1155Receiver is ERC165, IERC1155Receiver {
-    constructor() {
-        _registerInterface(type(IERC1155Receiver).interfaceId);
+    /**
+     * @dev See {IERC165-supportsInterface}.
+     */
+    function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) {
+        return interfaceId == type(IERC1155Receiver).interfaceId
+            || super.supportsInterface(interfaceId);
     }
 }

contracts/token/ERC721/ERC721.sol

@@ -53,11 +53,16 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
     constructor (string memory name_, string memory symbol_) {
         _name = name_;
         _symbol = symbol_;
+    }
 
-        // register the supported interfaces to conform to ERC721 via ERC165
-        _registerInterface(type(IERC721).interfaceId);
-        _registerInterface(type(IERC721Metadata).interfaceId);
-        _registerInterface(type(IERC721Enumerable).interfaceId);
+    /**
+     * @dev See {IERC165-supportsInterface}.
+     */
+    function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) {
+        return interfaceId == type(IERC721).interfaceId
+            || interfaceId == type(IERC721Metadata).interfaceId
+            || interfaceId == type(IERC721Enumerable).interfaceId
+            || super.supportsInterface(interfaceId);
     }
 
     /**

test/introspection/ERC165.test.js

@@ -1,5 +1,3 @@
-const { expectRevert } = require('@openzeppelin/test-helpers');
-
 const { shouldSupportInterfaces } = require('./SupportsInterface.behavior');
 
 const ERC165Mock = artifacts.require('ERC165Mock');
@@ -9,10 +7,6 @@ contract('ERC165', function (accounts) {
     this.mock = await ERC165Mock.new();
   });
 
-  it('does not allow 0xffffffff', async function () {
-    await expectRevert(this.mock.registerInterface('0xffffffff'), 'ERC165: invalid interface id');
-  });
-
   shouldSupportInterfaces([
     'ERC165',
   ]);

test/introspection/ERC165Storage.test.js

@@ -0,0 +1,25 @@
+const { expectRevert } = require('@openzeppelin/test-helpers');
+
+const { shouldSupportInterfaces } = require('./SupportsInterface.behavior');
+
+const ERC165Mock = artifacts.require('ERC165StorageMock');
+
+contract('ERC165Storage', function (accounts) {
+  beforeEach(async function () {
+    this.mock = await ERC165Mock.new();
+  });
+
+  it('register interface', async function () {
+    expect(await this.mock.supportsInterface('0x00000001')).to.be.equal(false);
+    await this.mock.registerInterface('0x00000001');
+    expect(await this.mock.supportsInterface('0x00000001')).to.be.equal(true);
+  });
+
+  it('does not allow 0xffffffff', async function () {
+    await expectRevert(this.mock.registerInterface('0xffffffff'), 'ERC165: invalid interface id');
+  });
+
+  shouldSupportInterfaces([
+    'ERC165',
+  ]);
+});