Add ERC165Query library

[ldub]

🚀 Description

The ERC165 spec contains useful code for calling the supportsInterface function on an unknown contract. I use this code and I expect many others need to use this as well. It would be helpful to have it available in OpenZeppelin.

I have added the example code from the spec to OpenZeppelin.

Modifications:

• changed to a library instead of contract
• changed constant function to view function
• added natspec comments
• formatted to fix all linter issues

Notes:

• would you want unit tests for this?
• would you want me to update the pragma for newer solidity version?
• the security/no-inline-assembly warning can't be addressed as the spec uses inline assembly

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

Fixes #1145

[nventuro]

Hey @ldub, thanks for your contribution! I'm not sure why we didn't think of adding these, considering we already have support for ERC1655.

There are some minor things that we should improve, but what worries me the most is the lack of tests. Could you create a couple of mock contracts and test this helper against them? Thanks!

[nventuro]

Please target ^0.4.24.

[ldub]

fixed

[nventuro]

Consider adding a comment mentioning that this equals bytes4(keccak256('supportsInterface(bytes4)').

[ldub]

fixed

[nventuro]

Please add spacing here between the || operator to comply with our code style.

[ldub]

fixed

[nventuro]

Must these be uint256? Can't we use bool?

[ldub]

So this code came from the spec: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md

I take it that the EIP authors used uint256 here as thats what the inline assembly returns by default when you mload.

We can change the noThrowCall code to return bools, but I would prefer the library code here to stay as close to the spec as possible.

[nventuro]

AFAIK, that code is not technically part of the specification, but a sample contract to test it. And according to the Solidity docs, staticcall returns a bool, as does supportsInterface according to the 165 spec, so it's not like we're doing anything weird.

Encoding success and result in uint256 is confusing IMO, since those are actually boolean values (as evidenced by the fact that they are only compared to 0 and 1). I'd make them bool from the get go to reduce noise and make our intent 100% clear.

Similarly, I'd add comments on the 3 noThrowCall calls, indicating what is being done on each one (check that the method exists, check that it returns false for invalid ids, and only then check the actual method that is being queried). I may even go as far as splitting that into two methods, one checking ERC165 support (the first two calls), and one only checking for the requested method (the last one), so that, when checking multiple methods, the first two calls are not needlessly duplicated (saving up to 60k gas per check).

That way, we'll end up with a cleaner query library :)

[nventuro]

I'm not an inline assembly expert, but should't we also update the free memory pointer at this point?

[ldub]

We would only need to update the free memory pointer if we wanted to allocate more stuff in memory. But we don't need to do that - we make a staticcall and thats it. So my understanding is we don't want to increment the free memory pointer as we'd just want to decrement it later.

[nventuro]

You're right, I somehow thought that was as internal call. Thanks!

[ldub]

Still figuring out how to write/run tests for this. Having some trouble getting tests to run - there is no "npm run test" script either. Do you have a wiki anywhere for how the testing is set up in this repo?

[nventuro]

@ldub that's weird about not being able to run the test suite - are you sure you tried from the project root directory? npm run test (or even npm test) should work (it's on the package.json.

Regarding the tests themselves, full coverage would be a bit tricky since there are 3 calls being made, and each returns 2 booleans (4 possible execution paths). To simplify this, I'd go for these three tests:

• Make an empty mock contract, and check that doesContractImplementInterface returns false
• Make an ERC165 compliant contract (e.g. inherit from SupportsInterfaceWithLookup) and check that doesContractImplementInterface returns true for supportsInterface(bytes4) but false for some other signature
• Make another ERC165 compliant contract that implements some custom method (whatever you want), and check that doesContractImplementInterface returns true for that signature.

Those contracts should be placed in contracts/mocks/ImplementsInterfaceMock.sol.

@shrugs recently worked on the ERC165 tests, so he may have some ideas on that front.

Thanks for your interest in this topic, and let me know if I can be of help!

[shrugs]

hi @ldub sorry I didn't see this earlier! I actually implemented most of this PR as part of #1024

can you cherry-pick the associated contract, tests, mocks, etc and add them here? I also prefer the API used by ERC165Checker (myAddress.supportsInterface(...)), if you're ok with that.

then we'll also be able to close #1145

[ldub]

I made all the changes requested, but the Travis build is now failing like so:

Error: Cannot find module 'chai-as-promised'
    at Function.Module._resolveFilename (module.js:547:15)
    at Function.Module._load (module.js:474:25)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/home/travis/build/OpenZeppelin/openzeppelin-solidity/test/introspection/ERC165Checker.test.js:9:8)

Strange because chai-as-promised is definitely in the package.json and it builds fine locally.

On my machine:

λ npm ls chai-as-promised
openzeppelin-solidity@1.10.0 /Users/lev/dev/openzeppelin-solidity
└── chai-as-promised@7.1.1

but travis' npm ls output indeed does not have chai-as-promised. I dont think any of my changes could have broken this, do you guys have any idea @shrugs @nventuro ?

[nventuro]

We've removed chai-as-promised (#1116) because it was somewhat finicky and didn't generally work. You can replace promise.should.eventually with (await promise).should and the result will be the same.

[shrugs]

@ldub @nventuro I'd prefer that the supportsInterface method also checks that the contract supports ERC165 as well. Primarily, I'd like developers to be able to write

require(myAddress.supportsInterface(InterfaceId_MyInterface))

and be certain that not only does it support 165 but that it also supports that interface.

Thoughts?

[nventuro]

@shrugs I requested that change here, my reasoning was that the split would allow for the general ERC165 check to only be done once. Do you think there's value in doing that (i.e. that a contract may call more than one method on some other contract in a single TX, and save gas by only checking for ERC165 support once)?

I'd definitely add a method like this one though:

function supportsERC165Interface(address contract, bytes4 interface) returns (bool) {
  return supportsERC165(contract) && supportsInterface(contract, interface);
}

(that name is awful and should be changed)

[shrugs]

@nventuro The contract should support that use-case yeah. Perhaps we can make it supportsERC165, _supportsInterface, and supportsInterface?

idk, I also arrived at the same name you did, supportsERC165Interface, but I'd prefer that supportsERC165Interface is the more specific one and supportsInterface covers both cases, just for external API reasons.

[nventuro]

Agreed, I think this reads very clearly:

function foo() {
  require(contract.supportsERC165());
  require(contract.supportsERC165Interface(someMethodID));
  require(contract.supportsERC165Interface(someOtherMethodID));
}

function bar() {
  require(contract.supportsInterface(yetAnotherMethodID));
}

[shrugs]

@nventuro what's our stance on indentation within comments? personally I prefer it, but I remember it being deemed unnecessary as a part of one of the other PRs?

[nventuro]

tbh I have no idea on whether or not they affect docgen. I'd remove all indentation for consistency, and tackle it in a separate PR if needed.

[shrugs]

nit: indentation

[shrugs]

LGTM, pending comment note

[nventuro]

This is in a great place @ldub, let's address the final nitpicky comments so that we can move along and merge this! 🎉

[nventuro]

Could you update this comment to include the '0x01ffc9a7' value? That way there'll be no doubt regarding where it comes from.

[shrugs]

actually, could we update this to follow the pattern we're using elsewhere for interface ids?

  bytes4 internal constant InterfaceId_ERC721 = 0x80ac58cd;
  /*
   * 0x80ac58cd ===
   *   bytes4(keccak256('balanceOf(address)')) ^
   *   bytes4(keccak256('ownerOf(uint256)')) ^
   *   bytes4(keccak256('approve(address,uint256)')) ^
   *   bytes4(keccak256('getApproved(uint256)')) ^
   *   bytes4(keccak256('setApprovalForAll(address,bool)')) ^
   *   bytes4(keccak256('isApprovedForAll(address,address)')) ^
   *   bytes4(keccak256('transferFrom(address,address,uint256)')) ^
   *   bytes4(keccak256('safeTransferFrom(address,address,uint256)')) ^
   *   bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)'))
   */

[ldub]

Ok, pushing a quick fix for this shortly.

Some strangeness/inconsistency here though

1. Why does the comment go after the field instead of before it?
2. Some places in this repo (link1) use a natspec-style double asterisk comment /** even though its after the field and thus not natspec
3. We are using triple equals in a solidity context (link1 link2)

[nventuro]

Hmm, agree with you on this. I opened #1182 to address it. Thanks!

requesting changes on interfaceid comment structure

[nventuro]

The coverage build is failing for some reason, I'm looking into that.

[frangio]

I think this can be done with abi.encodeWithSelector, and thus remove at least part of the assembly code. Also I'm not a fan of the x variable name. 😅

[ldub]

If i understand correctly, you would prefer something like:

  function noThrowCall (
    address _address,
    bytes4 _interfaceId
  ) 
    internal
    view
    returns (bool success, bool result)
  {
    // encode erc165 selector and first argument (_interfaceId)
    bytes memory encodedParams = abi.encodeWithSelector(InterfaceId_ERC165, _interfaceId);

    // solium-disable-next-line security/no-inline-assembly
    assembly {
        let freeMemoryPtr := mload(0x40)     // Find empty storage location using "free memory pointer"
        mstore(freeMemoryPtr, encodedParams) // Place encoded args at beginning of empty storage

        success := staticcall(
                  30000,         // 30k gas
                  _address,      // To addr
                  freeMemoryPtr, // Inputs are stored at free memory pointer
                  0x20,          // Inputs are 32 bytes long
                  freeMemoryPtr, // Store output over input (saves space)
                  0x20)          // Outputs are 32 bytes long

        result := mload(freeMemoryPtr) // Load the result
    }
  }

I just tested and this does not work (tests fail). The documentation implies that this should be equivalent to the previous code, but I'm probably misunderstanding something. Can you spot the bug?

I agree that it looks cleaner and am happy to push a change to this PR once I can fix this bug.

@nventuro @shrugs thoughts on this change?

[frangio]

The encode call gives you back a memory pointer to a dynamic byte array. It's already in memory so you don't need to manually mstore it. However, the dynamic byte array has a 32 byte length before the contents, so you should use add(encodedParams, 4) where freeMemoryPtr is being used now. I'm not a fan of storing the output over the input, but I suppose it works.

On second thought, I have a few more doubts about it. Are we sure that the result of abi.encodeWithSelector is 32 bytes long?

[ldub]

Tried what you suggested and it didn't work either.

Spent some time trying to use velma debugger to inspect what exactly abi.encodeWithSelector is giving (as the documentation is kind of sparse on this), but can't seem to get it to work with solidity 0.4.24. Will try with truffle debug and/or hevm tomorrow.

[frangio]

@ldub I was able to get abi.encodeWithSelector working on 0.4.24 as follows:

bytes memory encodedParams = abi.encodeWithSelector(InterfaceId_ERC165, _interfaceId);

// solium-disable-next-line security/no-inline-assembly
assembly {
    let encodedParams_data := add(0x20, encodedParams)
    let encodedParams_size := mload(encodedParams)
    
    let output := mload(0x40)               // Find empty storage location using "free memory pointer"

    success := staticcall(
              30000,     // 30k gas
              _address,  // To addr
              encodedParams_data,
              encodedParams_size,
              output,
              0x20)      // Outputs are 32 bytes long

    result := mload(output)   // Load the result
}

[ldub]

I believe I've made all changes requested by everybody and am waiting for further review/merge. Also, @nventuro @frangio is it ok that coverage percent is decreasing? I can't seem to do anything about it as I definitely have coverage for the lines its complaining about.

[shrugs]

@ldub agreed, coverage is confusing; I also think the tests cover those cases, just reading them. I'm ok with a manual merge here, but will test this locally to see if I can convince coverage to include those lines.

[shrugs]

I added two commits, one merging the latest master and another that uses reverts to test the return variables and re-implements SupportsInterfaceWithLookup to avoid the coverage issues we were facing earlier. Waiting on coverage to report back, but should be 100% on these files.

I realize now we probably don't have to use the requires to test the booleans since the free memory pointer isn't actually being messed up, so I could reasonably change the tests back to the original format where the mock was returning the booleans. Not sure if it matters too much. opinions, @frangio @ldub ?

[cgewecke]

@shrugs Do you know what was happening before if it's not the memory pointer? Is it just that it needs to run through a mock?

[shrugs]

@cgewecke the events that instrumented the SupportsInterfaceWithLookup contract that the target mocks were using was conflicting with the staticcall being issued by the checker contract (staticcalls cannot change state). h/t to @frangio for debugging that

[shrugs]

(I'll be changing the requires back to boolean asserts, btw, commit incoming)

[cgewecke]

@shrugs Ah interesting, thanks.

[ldub]

@shrugs thanks! Looks great. Also, I would love to know more about how you (or @frangio) figured out the issue.

[ldub]

I had changed this to private as per @frangio's request: #1086 (comment)

Since we now have the supportsInterfaces API to help save gas when checking many interfaces, we should make supportsERC165Interface private.

Let me know if you agree and I can make this change.

[frangio]

@shrugs was trying to see if solidity-coverage could be messing with the pointers, but eventually concluded that the instrumentation only adds events so it couldn't be that, and mentioned this in chat. The staticcall had caught my attention before, so I checked the EIP and saw that all the log* operations are not allowed in static mode.

Any attempts to make state-changing operations inside an execution instance with STATIC set to true will instead throw an exception. These operations include [...], LOG0, LOG1, LOG2, [..].

This makes sense because they mutate the blockchain, and in fact Solidity pure functions cannot emit events either.

[shrugs]

huh, @nventuro any thoughts on why the coverage build is failing so hard right now? Looks like a bunch of calls are returning tx objects instead of their results, destroying a bunch of the tests. Normal run looks fine and the solc-nightly is failing for unrelated reasons.

[shrugs]

cc @cgewecke any ideas on why coverage is failing right now? Sorry to keep pulling you in like this 😅

[frangio]

We figured it is being caused by name clashing with this contract that is duplicated:

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/f6d80635e13b9490c376c3a550d94ef2a01bf744/contracts/mocks/ERC165/ERC165InterfacesSupported.sol#L14

[frangio]

Great work to everyone involved!

[nventuro]

Aren't these tests repeats of context('ERC165 not supported')?

[shrugs]

but specifically for EOA accounts, not a contract deployed to the chain

[nventuro]

right, I guess those are different enough to make this a meaningful test, thanks!

[shrugs]

🎉

[nventuro]

Great work @ldub and everyone else! This is now the second most commented PR in OZ!

[ldub]

Yay! Huge thanks to everybody for the reviews and working with me on this :)