Enable using ERC165 check for one supported interface directly

[ashhanai]

This PR changes function visibility of ERC165Checker._supportsERC165Interface from private to internal. The function will be callable from contract using this library directly.

Rationale

Existing function doesn't return an information whether token doesn't implement ERC165 or given interface. If contract need this information, it need to call supportsERC165 first and then supportsInterface or getSupportedInterfaces. Unfortunately that would call ERC165 check twice.

Solution

Use existing function _supportsERC165Interface and enable it to be called by contract, not just library. Keep the underscore to indicate that this function is not meant to be called directly by default, developer has to know what he is doing and to check ERC165 support prior to calling this function.

PR Checklist

• Tests
• Documentation
• Changelog entry

[Amxx]

Hello @ashhanai

ERC165 dictates that we should check type(ERC165).interfaceId is supported before checking any other interfaceId. If you want to check a contract supports ERC165, you can use supportsERC165. If you want to check a specific interface, you can use supportsInterface (which indeed checks that the contract is ERC165 compliant before checking the specific interface).

As I understand your usecase would be to start by checking ERC165 compliance, then check specific interfaces, wich different error depending in which of the two fail. Can you tell us more about the context where you want to do that?

[ashhanai]

Hi @Amxx, I think you get my point. What I am trying to do is to be able to check list of supported interfaces, but end as soon as I find first match.

The flow could look something like this:

check ERC165 interface -> check invalid interface -> check first interface (if fails)-> check second interface (if fails) -> check third interface (if fails)-> ...

With current ERC165Checker implementation I can use supportsInterface for every interface I am looking for, but I will make two additional external calls, checking ERC165 support, for every interface check.

Or I can try to use getSupportedInterfaces, but this function will not stop when first match is found.

I believe this would be very useful for contracts working with mutually exclusive ERC tokens (ERC20, ERC721, ERC1155, ...).

[Amxx]

What about à function that given an array of interfaces returns a yes/no array ?

[ashhanai]

That's the getSupportedInterfaces function I mentioned earlier.

[Amxx]

Right, the getSupportedInterfaces already exist and returns this array. Why is that not good enough ? The only thing I can think of is that if the returned array returns false for all interfaceIds, you don't know if they are all not supported, or if ERC165 is not supported. Any reason you would like to know if ERC165 is advertised if everything is false anyway ?

[ashhanai]

If we talk about mutually exclusive interfaces, getSupportedInterfaces is not sufficient, because it will not fail on first found match and thus making few more external calls, which are unnecessary.

If we talk about inclusive interfaces (like ERC20, ERC777, ERC2612, ...), as these interfaces doesn't force implementing ERC165 in their EIPs, it's handy to know if it's missing ERC165 or the token really don't implement any of these ERCs. If we just take that they don't implement those ERCs just because getSupportedInterfaces returned false, it could be false-negative result.

What about creating a new function, that would be very similar to getSupportedInterfaces (or extending it, but it won't be backward compatible), which would return bool list, where first item would be if token implements ERC165? Would it be better fit? In that case, there is no need to change visibility of _supportsERC165Interface.

Also it could take flag that would determine if function should stop on a first match or continue (for mutually exclusive interfaces).

[Amxx]

I think having a

function getSupportedInterfaces_v2(address account, bytes4[] memory interfaceIds) /// find a better name
        internal
        view
        returns (bool, bool[] memory)
    {
        // an array of booleans corresponding to interfaceIds and whether they're supported or not
        bool erc165Supported = supportsERC165(account);
        bool[] memory interfaceIdsSupported = new bool[](interfaceIds.length);
        // query support of ERC165 itself
        if (erc165Supported) {
            // query support of each interface in interfaceIds
            for (uint256 i = 0; i < interfaceIds.length; i++) {
                interfaceIdsSupported[i] = _supportsERC165Interface(account, interfaceIds[i]);
            }
        }
        return erc165Supported, interfaceIdsSupported;
    }

could make sense.

[frangio]

@Amxx That is not what @ashhanai is asking for though, because it still checks the entire array.

The use case is to find the first supported interface from a list. I think this feels a little niche to support out of the box, so I'm considering actually making the private function internal. We should give it a scary name so that people don't use it unless they absolutely want to.

[ashhanai]

I agree with @frangio + I also think returning the additional bool representing ERC165 support from getSupportedInterfaces makes a lot of sense.

[YamenMerhi]

Hey @Amxx @frangio 👋
I was about to open an issue about this topic but I found @ashhanai already did a PR about it.
I support @ashhanai with making the function internal. I know that the standard enforces checking beforehand the ERC165 InterfaceID and the 0xfffffffff but from a smart contract perspective, it is cheaper to check one interfaceID than 3.
I agree that this shouldn't be the case all time but it's good to have it as a choice for people who absolutely want to.
As for the LSPs Standards 📜 , many logic is built on top of InterfaceIDs, and we quite need that.
Happy to discuss further, hope we can merge and have it in next release 🚀

[CJ42]

@frangio @Amxx
I think changing the visibility from private to internal make sense, as the low level staticcall would return false if the contract being called is not ERC165 compliant and does not implement IERC165.

Looking at the function body:

function _supportsERC165Interface(address account, bytes4 interfaceId) private view returns (bool) {
        bytes memory encodedParams = abi.encodeWithSelector(IERC165.supportsInterface.selector, interfaceId);
        
        // if the contract being called does not implement `IERC165` this will fail.
        (bool success, bytes memory result) = account.staticcall{gas: 30000}(encodedParams);
        
        if (result.length < 32) return false;
        return success && abi.decode(result, (bool));
    }

The function _supportsERC165Interface(...) would also return false if the payload would end up in the fallback function of the contract being called (if such contract has a fallback function).

I have written an example below and tested in Remix.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "https://raw.githubusercontent.com/OpenZeppelin/openzeppelin-contracts/master/contracts/utils/introspection/ERC165Checker.sol";

import "https://raw.githubusercontent.com/OpenZeppelin/openzeppelin-contracts/master/contracts/utils/introspection/ERC165.sol";

contract Playground {

    function _supportsERC165Interface(address account, bytes4 interfaceId) private view returns (bool) {
        bytes memory encodedParams = abi.encodeWithSelector(IERC165.supportsInterface.selector, interfaceId);
        (bool success, bytes memory result) = account.staticcall{gas: 30000}(encodedParams);
        if (result.length < 32) return false;
        return success && abi.decode(result, (bool));
    }
    
    function testSupportsInterface(address _target) public view returns (bool) {
        return _supportsERC165Interface(_target, 0xaabbccdd);
    }
}

// _supportsERC165Interface(0xaabbccdd) --> returns true
contract TargetWithERC165 is ERC165 {

    function supportsInterface(bytes4 interfaceId) public pure override returns (bool) {
        return interfaceId == 0xaabbccdd;
    }
}

// _supportsERC165Interface(0xaabbccdd) --> returns false
contract TargetWithoutERC165 {

}

// _supportsERC165Interface(0xaabbccdd) --> returns false
contract TargetWithoutERC165ButFallback {
    
    fallback() external {

    }
}

[Amxx]

Should we take this occasion to refactor it to

function _supportsERC165Interface(address account, bytes4 interfaceId) private view returns (bool) {
    try IERC165(account).supportsInterface{ gas: 30000 }(interfaceId) returns (bool supported) {
        return supported;
    } catch {
        false;
    }
}

?

Edit: that would revert for EOA, when we need it to return false :/

[Amxx]

if we make _supportsERC165Interface internal, we need to rename it so it doesn't start with _.

We could rename it to supportsERC165Interface, but that would be confusing considering the "normal" function, that most people should use, are supportsERC165 and supportsInterface.

Any suggestions?

[YamenMerhi]

Nothing in my mind, maybe supportsSingleInterface | supportsInterfaceForce ? @Amxx

[ashhanai]

@Amxx can we keep the _ to show that it's not intended to call directly unless you know what you are doing?

[Amxx]

If it's an internal function in a library, the convention is no. we would have to make an exception ... I don't like that.

[frangio]

What do you think of "raw" for the name? rawSupportsERC165Interface or supportsERC165InterfaceRaw.

[frangio]

What do you think of "raw" for the name? rawSupportsERC165Interface or supportsERC165InterfaceRaw.

[YamenMerhi]

Good Suggestion 👍

[Amxx]

Lets go for supportsERC165InterfaceRaw

[Amxx]

We need a changelog entry

[CJ42]

A bit late to the naming party. I was about to suggest supportsERC165InterfaceUnchecked or something with the "unchecked" word in the name. To follow on @frangio thoughts on "giving it a scary name".

Was thinking that could also look similar to the concept of unchecked in Solidity for arithmetic, but applied to ERC165 interfaces check.

I guess "raw" is smaller and might read better :)

[ashhanai]

I like the unchecked version a lot. What about giving it even more scary name like unchecked_supportsERC165Interface? Even though it again brakes some conventions.

[Amxx]

Lets go with supportsERC165InterfaceUnchecked.

@ashhanai can you do that change and add a Changelog entry please?
We will also need a small tests

[frangio]

Thank you!