Adopt new Solidity features interfaceId, try/catch, keccak constants (#2487)

* Clean code

-  using type().interfaceId to improve readeability of ERC165 registration
- hardcoding some keccak256 that are otherwise computed at construction.

* hardcode keccak256 result

* Improve code readeability using try/catch

* Remove hardcoded hash 

tests show that solc 0.8.0 does the optimization as expected

* Use try/catch to improve readability

* ERC165Checker: Do not revert when returndata is empty + new test

* Address PR comments

* improve testing of ERC721Receiver errors

* put back comment about invalid interface id

* coverage does not support 0.8.1. Reverting back to 0.8.0

* bubble all data with length > 0 if onERC721Receive fails.

* Fix test: revert without message trigger is bubble with the default message

* using enum object to improve readability

contracts/access/TimelockController.sol

@@ -20,7 +20,6 @@ import "./AccessControl.sol";
  * _Available since v3.3._
  */
 contract TimelockController is AccessControl {
-
     bytes32 public constant TIMELOCK_ADMIN_ROLE = keccak256("TIMELOCK_ADMIN_ROLE");
     bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE");
     bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");

contracts/introspection/ERC165.sol

@@ -11,11 +11,6 @@ import "./IERC165.sol";
  * their support of an interface.
  */
 abstract contract ERC165 is IERC165 {
-    /*
-     * bytes4(keccak256('supportsInterface(bytes4)')) == 0x01ffc9a7
-     */
-    bytes4 private constant _INTERFACE_ID_ERC165 = 0x01ffc9a7;
-
     /**
      * @dev Mapping of interface ids to whether or not it's supported.
      */
@@ -24,7 +19,7 @@ abstract contract ERC165 is IERC165 {
     constructor () {
         // Derived contracts need only register support for their own interfaces,
         // we register support for ERC165 itself here
-        _registerInterface(_INTERFACE_ID_ERC165);
+        _registerInterface(type(IERC165).interfaceId);
     }
 
     /**

contracts/introspection/ERC165Checker.sol

@@ -2,6 +2,8 @@
 
 pragma solidity ^0.8.0;
 
+import "./IERC165.sol";
+
 /**
  * @dev Library used to query support of an interface declared via {IERC165}.
  *
@@ -13,18 +15,13 @@ library ERC165Checker {
     // As per the EIP-165 spec, no interface should ever match 0xffffffff
     bytes4 private constant _INTERFACE_ID_INVALID = 0xffffffff;
 
-    /*
-     * bytes4(keccak256('supportsInterface(bytes4)')) == 0x01ffc9a7
-     */
-    bytes4 private constant _INTERFACE_ID_ERC165 = 0x01ffc9a7;
-
     /**
      * @dev Returns true if `account` supports the {IERC165} interface,
      */
     function supportsERC165(address account) internal view returns (bool) {
         // Any contract that implements ERC165 must explicitly indicate support of
         // InterfaceId_ERC165 and explicitly indicate non-support of InterfaceId_Invalid
-        return _supportsERC165Interface(account, _INTERFACE_ID_ERC165) &&
+        return _supportsERC165Interface(account, type(IERC165).interfaceId) &&
             !_supportsERC165Interface(account, _INTERFACE_ID_INVALID);
     }
 
@@ -101,29 +98,9 @@ library ERC165Checker {
      * Interface identification is specified in ERC-165.
      */
     function _supportsERC165Interface(address account, bytes4 interfaceId) private view returns (bool) {
-        // success determines whether the staticcall succeeded and result determines
-        // whether the contract at account indicates support of _interfaceId
-        (bool success, bool result) = _callERC165SupportsInterface(account, interfaceId);
-
-        return (success && result);
-    }
-
-    /**
-     * @notice Calls the function with selector 0x01ffc9a7 (ERC165) and suppresses throw
-     * @param account The address of the contract to query for support of an interface
-     * @param interfaceId The interface identifier, as specified in ERC-165
-     * @return success true if the STATICCALL succeeded, false otherwise
-     * @return result true if the STATICCALL succeeded and the contract at account
-     * indicates support of the interface with identifier interfaceId, false otherwise
-     */
-    function _callERC165SupportsInterface(address account, bytes4 interfaceId)
-        private
-        view
-        returns (bool, bool)
-    {
-        bytes memory encodedParams = abi.encodeWithSelector(_INTERFACE_ID_ERC165, interfaceId);
+        bytes memory encodedParams = abi.encodeWithSelector(IERC165(account).supportsInterface.selector, interfaceId);
         (bool success, bytes memory result) = account.staticcall{ gas: 30000 }(encodedParams);
-        if (result.length < 32) return (false, false);
-        return (success, abi.decode(result, (bool)));
+        if (result.length < 32) return false;
+        return success && abi.decode(result, (bool));
     }
 }

contracts/introspection/ERC1820Implementer.sol

@@ -13,7 +13,7 @@ import "./IERC1820Implementer.sol";
  * registration to be complete.
  */
 contract ERC1820Implementer is IERC1820Implementer {
-    bytes32 constant private _ERC1820_ACCEPT_MAGIC = keccak256(abi.encodePacked("ERC1820_ACCEPT_MAGIC"));
+    bytes32 private constant _ERC1820_ACCEPT_MAGIC = keccak256("ERC1820_ACCEPT_MAGIC");
 
     mapping(bytes32 => mapping(address => bool)) private _supportedInterfaces;
 

contracts/mocks/ERC165/ERC165MissingData.sol

+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+contract ERC165MissingData {
+    function supportsInterface(bytes4 interfaceId) public view {} // missing return
+}

contracts/mocks/ERC721ReceiverMock.sol

@@ -5,20 +5,34 @@ pragma solidity ^0.8.0;
 import "../token/ERC721/IERC721Receiver.sol";
 
 contract ERC721ReceiverMock is IERC721Receiver {
-    bytes4 private _retval;
-    bool private _reverts;
+    enum Error {
+        None,
+        RevertWithMessage,
+        RevertWithoutMessage,
+        Panic
+    }
+
+    bytes4 private immutable _retval;
+    Error private immutable _error;
 
     event Received(address operator, address from, uint256 tokenId, bytes data, uint256 gas);
 
-    constructor (bytes4 retval, bool reverts) {
+    constructor (bytes4 retval, Error error) {
         _retval = retval;
-        _reverts = reverts;
+        _error = error;
     }
 
     function onERC721Received(address operator, address from, uint256 tokenId, bytes memory data)
         public override returns (bytes4)
     {
-        require(!_reverts, "ERC721ReceiverMock: reverting");
+        if (_error == Error.RevertWithMessage) {
+            revert("ERC721ReceiverMock: reverting");
+        } else if (_error == Error.RevertWithoutMessage) {
+            revert();
+        } else if (_error == Error.Panic) {
+            uint256 a = uint256(0) / uint256(0);
+            a;
+        }
         emit Received(operator, from, tokenId, data, gasleft());
         return _retval;
     }

contracts/token/ERC1155/ERC1155.sol

@@ -29,24 +29,6 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI {
     // Used as the URI for all token types by relying on ID substitution, e.g. https://token-cdn-domain/{id}.json
     string private _uri;
 
-    /*
-     *     bytes4(keccak256('balanceOf(address,uint256)')) == 0x00fdd58e
-     *     bytes4(keccak256('balanceOfBatch(address[],uint256[])')) == 0x4e1273f4
-     *     bytes4(keccak256('setApprovalForAll(address,bool)')) == 0xa22cb465
-     *     bytes4(keccak256('isApprovedForAll(address,address)')) == 0xe985e9c5
-     *     bytes4(keccak256('safeTransferFrom(address,address,uint256,uint256,bytes)')) == 0xf242432a
-     *     bytes4(keccak256('safeBatchTransferFrom(address,address,uint256[],uint256[],bytes)')) == 0x2eb2c2d6
-     *
-     *     => 0x00fdd58e ^ 0x4e1273f4 ^ 0xa22cb465 ^
-     *        0xe985e9c5 ^ 0xf242432a ^ 0x2eb2c2d6 == 0xd9b67a26
-     */
-    bytes4 private constant _INTERFACE_ID_ERC1155 = 0xd9b67a26;
-
-    /*
-     *     bytes4(keccak256('uri(uint256)')) == 0x0e89341c
-     */
-    bytes4 private constant _INTERFACE_ID_ERC1155_METADATA_URI = 0x0e89341c;
-
     /**
      * @dev See {_setURI}.
      */
@@ -54,10 +36,10 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI {
         _setURI(uri_);
 
         // register the supported interfaces to conform to ERC1155 via ERC165
-        _registerInterface(_INTERFACE_ID_ERC1155);
+        _registerInterface(type(IERC1155).interfaceId);
 
         // register the supported interfaces to conform to ERC1155MetadataURI via ERC165
-        _registerInterface(_INTERFACE_ID_ERC1155_METADATA_URI);
+        _registerInterface(type(IERC1155MetadataURI).interfaceId);
     }
 
     /**

contracts/token/ERC1155/ERC1155Receiver.sol

@@ -10,9 +10,6 @@ import "../../introspection/ERC165.sol";
  */
 abstract contract ERC1155Receiver is ERC165, IERC1155Receiver {
     constructor() {
-        _registerInterface(
-            ERC1155Receiver(address(0)).onERC1155Received.selector ^
-            ERC1155Receiver(address(0)).onERC1155BatchReceived.selector
-        );
+        _registerInterface(type(IERC1155Receiver).interfaceId);
     }
 }

contracts/token/ERC721/ERC721.sol

@@ -23,10 +23,6 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
     using EnumerableMap for EnumerableMap.UintToAddressMap;
     using Strings for uint256;
 
-    // Equals to `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
-    // which can be also obtained as `IERC721Receiver(0).onERC721Received.selector`
-    bytes4 private constant _ERC721_RECEIVED = 0x150b7a02;
-
     // Mapping from holder address to their (enumerable) set of owned tokens
     mapping (address => EnumerableSet.UintSet) private _holderTokens;
 
@@ -51,40 +47,6 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
     // Base URI
     string private _baseURI;
 
-    /*
-     *     bytes4(keccak256('balanceOf(address)')) == 0x70a08231
-     *     bytes4(keccak256('ownerOf(uint256)')) == 0x6352211e
-     *     bytes4(keccak256('approve(address,uint256)')) == 0x095ea7b3
-     *     bytes4(keccak256('getApproved(uint256)')) == 0x081812fc
-     *     bytes4(keccak256('setApprovalForAll(address,bool)')) == 0xa22cb465
-     *     bytes4(keccak256('isApprovedForAll(address,address)')) == 0xe985e9c5
-     *     bytes4(keccak256('transferFrom(address,address,uint256)')) == 0x23b872dd
-     *     bytes4(keccak256('safeTransferFrom(address,address,uint256)')) == 0x42842e0e
-     *     bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)')) == 0xb88d4fde
-     *
-     *     => 0x70a08231 ^ 0x6352211e ^ 0x095ea7b3 ^ 0x081812fc ^
-     *        0xa22cb465 ^ 0xe985e9c5 ^ 0x23b872dd ^ 0x42842e0e ^ 0xb88d4fde == 0x80ac58cd
-     */
-    bytes4 private constant _INTERFACE_ID_ERC721 = 0x80ac58cd;
-
-    /*
-     *     bytes4(keccak256('name()')) == 0x06fdde03
-     *     bytes4(keccak256('symbol()')) == 0x95d89b41
-     *     bytes4(keccak256('tokenURI(uint256)')) == 0xc87b56dd
-     *
-     *     => 0x06fdde03 ^ 0x95d89b41 ^ 0xc87b56dd == 0x5b5e139f
-     */
-    bytes4 private constant _INTERFACE_ID_ERC721_METADATA = 0x5b5e139f;
-
-    /*
-     *     bytes4(keccak256('totalSupply()')) == 0x18160ddd
-     *     bytes4(keccak256('tokenOfOwnerByIndex(address,uint256)')) == 0x2f745c59
-     *     bytes4(keccak256('tokenByIndex(uint256)')) == 0x4f6ccce7
-     *
-     *     => 0x18160ddd ^ 0x2f745c59 ^ 0x4f6ccce7 == 0x780e9d63
-     */
-    bytes4 private constant _INTERFACE_ID_ERC721_ENUMERABLE = 0x780e9d63;
-
     /**
      * @dev Initializes the contract by setting a `name` and a `symbol` to the token collection.
      */
@@ -93,9 +55,9 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
         _symbol = symbol_;
 
         // register the supported interfaces to conform to ERC721 via ERC165
-        _registerInterface(_INTERFACE_ID_ERC721);
-        _registerInterface(_INTERFACE_ID_ERC721_METADATA);
-        _registerInterface(_INTERFACE_ID_ERC721_ENUMERABLE);
+        _registerInterface(type(IERC721).interfaceId);
+        _registerInterface(type(IERC721Metadata).interfaceId);
+        _registerInterface(type(IERC721Enumerable).interfaceId);
     }
 
     /**
@@ -433,18 +395,22 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
     function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory _data)
         private returns (bool)
     {
-        if (!to.isContract()) {
+        if (to.isContract()) {
+            try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data) returns (bytes4 retval) {
+                return retval == IERC721Receiver(to).onERC721Received.selector;
+            } catch (bytes memory reason) {
+                if (reason.length == 0) {
+                    revert("ERC721: transfer to non ERC721Receiver implementer");
+                } else {
+                    // solhint-disable-next-line no-inline-assembly
+                    assembly {
+                        revert(add(32, reason), mload(reason))
+                    }
+                }
+            }
+        } else {
             return true;
         }
-        bytes memory returndata = to.functionCall(abi.encodeWithSelector(
-            IERC721Receiver(to).onERC721Received.selector,
-            _msgSender(),
-            from,
-            tokenId,
-            _data
-        ), "ERC721: transfer to non ERC721Receiver implementer");
-        bytes4 retval = abi.decode(returndata, (bytes4));
-        return (retval == _ERC721_RECEIVED);
     }
 
     function _approve(address to, uint256 tokenId) private {

contracts/token/ERC777/ERC777.sol

@@ -37,16 +37,8 @@ contract ERC777 is Context, IERC777, IERC20 {
     string private _name;
     string private _symbol;
 
-    // We inline the result of the following hashes because Solidity doesn't resolve them at compile time.
-    // See https://github.com/ethereum/solidity/issues/4024.
-
-    // keccak256("ERC777TokensSender")
-    bytes32 constant private _TOKENS_SENDER_INTERFACE_HASH =
-        0x29ddb589b1fb5fc7cf394961c1adf5f8c6454761adf795e67fe149f658abe895;
-
-    // keccak256("ERC777TokensRecipient")
-    bytes32 constant private _TOKENS_RECIPIENT_INTERFACE_HASH =
-        0xb281fc8c12954d22544db45de3159a39272895b169a852b314f9cc762e44c53b;
+    bytes32 private constant _TOKENS_SENDER_INTERFACE_HASH = keccak256("ERC777TokensSender");
+    bytes32 private constant _TOKENS_RECIPIENT_INTERFACE_HASH = keccak256("ERC777TokensRecipient");
 
     // This isn't ever read from - it's only used to respond to the defaultOperators query.
     address[] private _defaultOperatorsArray;

test/introspection/ERC165Checker.test.js

@@ -3,6 +3,7 @@ require('@openzeppelin/test-helpers');
 const { expect } = require('chai');
 
 const ERC165CheckerMock = artifacts.require('ERC165CheckerMock');
+const ERC165MissingData = artifacts.require('ERC165MissingData');
 const ERC165NotSupported = artifacts.require('ERC165NotSupported');
 const ERC165InterfacesSupported = artifacts.require('ERC165InterfacesSupported');
 
@@ -18,6 +19,33 @@ contract('ERC165Checker', function (accounts) {
     this.mock = await ERC165CheckerMock.new();
   });
 
+  context('ERC165 missing return data', function () {
+    beforeEach(async function () {
+      this.target = await ERC165MissingData.new();
+    });
+
+    it('does not support ERC165', async function () {
+      const supported = await this.mock.supportsERC165(this.target.address);
+      expect(supported).to.equal(false);
+    });
+
+    it('does not support mock interface via supportsInterface', async function () {
+      const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID);
+      expect(supported).to.equal(false);
+    });
+
+    it('does not support mock interface via supportsAllInterfaces', async function () {
+      const supported = await this.mock.supportsAllInterfaces(this.target.address, [DUMMY_ID]);
+      expect(supported).to.equal(false);
+    });
+
+    it('does not support mock interface via getSupportedInterfaces', async function () {
+      const supported = await this.mock.getSupportedInterfaces(this.target.address, [DUMMY_ID]);
+      expect(supported.length).to.equal(1);
+      expect(supported[0]).to.equal(false);
+    });
+  });
+
   context('ERC165 not supported', function () {
     beforeEach(async function () {
       this.target = await ERC165NotSupported.new();

test/token/ERC721/ERC721.test.js

@@ -8,6 +8,9 @@ const { shouldSupportInterfaces } = require('../../introspection/SupportsInterfa
 const ERC721Mock = artifacts.require('ERC721Mock');
 const ERC721ReceiverMock = artifacts.require('ERC721ReceiverMock');
 
+const Error = [ 'None', 'RevertWithMessage', 'RevertWithoutMessage', 'Panic' ]
+  .reduce((acc, entry, idx) => Object.assign({ [entry]: idx }, acc), {});
+
 contract('ERC721', function (accounts) {
   const [owner, newOwner, approved, anotherApproved, operator, other] = accounts;
 
@@ -324,7 +327,7 @@ contract('ERC721', function (accounts) {
 
           describe('to a valid receiver contract', function () {
             beforeEach(async function () {
-              this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, false);
+              this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.None);
               this.toWhom = this.receiver.address;
             });
 
@@ -379,7 +382,7 @@ contract('ERC721', function (accounts) {
 
         describe('to a receiver contract returning unexpected value', function () {
           it('reverts', async function () {
-            const invalidReceiver = await ERC721ReceiverMock.new('0x42', false);
+            const invalidReceiver = await ERC721ReceiverMock.new('0x42', Error.None);
             await expectRevert(
               this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }),
               'ERC721: transfer to non ERC721Receiver implementer',
@@ -387,9 +390,9 @@ contract('ERC721', function (accounts) {
           });
         });
 
-        describe('to a receiver contract that throws', function () {
+        describe('to a receiver contract that reverts with message', function () {
           it('reverts', async function () {
-            const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
+            const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.RevertWithMessage);
             await expectRevert(
               this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }),
               'ERC721ReceiverMock: reverting',
@@ -397,6 +400,25 @@ contract('ERC721', function (accounts) {
           });
         });
 
+        describe('to a receiver contract that reverts without message', function () {
+          it('reverts', async function () {
+            const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.RevertWithoutMessage);
+            await expectRevert(
+              this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }),
+              'ERC721: transfer to non ERC721Receiver implementer',
+            );
+          });
+        });
+
+        describe('to a receiver contract that panics', function () {
+          it('reverts', async function () {
+            const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.Panic);
+            await expectRevert.unspecified(
+              this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }),
+            );
+          });
+        });
+
         describe('to a contract that does not implement the required function', function () {
           it('reverts', async function () {
             const nonReceiver = this.token;
@@ -416,7 +438,7 @@ contract('ERC721', function (accounts) {
 
       describe('via safeMint', function () { // regular minting is tested in ERC721Mintable.test.js and others
         it('calls onERC721Received — with data', async function () {
-          this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, false);
+          this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.None);
           const receipt = await this.token.safeMint(this.receiver.address, tokenId, data);
 
           await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', {
@@ -427,7 +449,7 @@ contract('ERC721', function (accounts) {
         });
 
         it('calls onERC721Received — without data', async function () {
-          this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, false);
+          this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.None);
           const receipt = await this.token.safeMint(this.receiver.address, tokenId);
 
           await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', {
@@ -438,7 +460,7 @@ contract('ERC721', function (accounts) {
 
         context('to a receiver contract returning unexpected value', function () {
           it('reverts', async function () {
-            const invalidReceiver = await ERC721ReceiverMock.new('0x42', false);
+            const invalidReceiver = await ERC721ReceiverMock.new('0x42', Error.None);
             await expectRevert(
               this.token.safeMint(invalidReceiver.address, tokenId),
               'ERC721: transfer to non ERC721Receiver implementer',
@@ -446,9 +468,9 @@ contract('ERC721', function (accounts) {
           });
         });
 
-        context('to a receiver contract that throws', function () {
+        context('to a receiver contract that reverts with message', function () {
           it('reverts', async function () {
-            const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
+            const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.RevertWithMessage);
             await expectRevert(
               this.token.safeMint(revertingReceiver.address, tokenId),
               'ERC721ReceiverMock: reverting',
@@ -456,6 +478,25 @@ contract('ERC721', function (accounts) {
           });
         });
 
+        context('to a receiver contract that reverts without message', function () {
+          it('reverts', async function () {
+            const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.RevertWithoutMessage);
+            await expectRevert(
+              this.token.safeMint(revertingReceiver.address, tokenId),
+              'ERC721: transfer to non ERC721Receiver implementer',
+            );
+          });
+        });
+
+        context('to a receiver contract that panics', function () {
+          it('reverts', async function () {
+            const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.Panic);
+            await expectRevert.unspecified(
+              this.token.safeMint(revertingReceiver.address, tokenId),
+            );
+          });
+        });
+
         context('to a contract that does not implement the required function', function () {
           it('reverts', async function () {
             const nonReceiver = this.token;