Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: ERC165Checker - getSupportedInterfaces #2429

Closed
conspyrosy opened this issue Dec 9, 2020 · 8 comments 路 Fixed by #2469
Closed

Proposal: ERC165Checker - getSupportedInterfaces #2429

conspyrosy opened this issue Dec 9, 2020 · 8 comments 路 Fixed by #2469
Labels
feature New contracts, functions, or helpers.

Comments

@conspyrosy
Copy link
Contributor

conspyrosy commented Dec 9, 2020

馃 Motivation

ERC165 allows you to check which interfaces are implemented on a contract. OZ has a library ERC165Checker.sol with helper methods for checking if some contract implements an interface. First, it checks that ERC165 is implemented and then it checks if the interface is implemented. For batch checking interfaces, there is a separate method supportsAllInterfaces which checks if ERC165 is implemented only once, and then checks each interface to ensure they are implemented. Here is the relevant existing code:

    /**
     * @dev Returns true if `account` supports the interface defined by
     * `interfaceId`. Support for {IERC165} itself is queried automatically.
     *
     * See {IERC165-supportsInterface}.
     */
    function supportsInterface(address account, bytes4 interfaceId) internal view returns (bool) {
        // query support of both ERC165 as per the spec and support of _interfaceId
        return supportsERC165(account) &&
            _supportsERC165Interface(account, interfaceId);
    }

    /**
     * @dev Returns true if `account` supports all the interfaces defined in
     * `interfaceIds`. Support for {IERC165} itself is queried automatically.
     *
     * Batch-querying can lead to gas savings by skipping repeated checks for
     * {IERC165} support.
     *
     * See {IERC165-supportsInterface}.
     */
    function supportsAllInterfaces(address account, bytes4[] memory interfaceIds) internal view returns (bool) {
        // query support of ERC165 itself
        if (!supportsERC165(account)) {
            return false;
        }

        // query support of each interface in _interfaceIds
        for (uint256 i = 0; i < interfaceIds.length; i++) {
            if (!_supportsERC165Interface(account, interfaceIds[i])) {
                return false;
            }
        }

        // all interfaces supported
        return true;
    }

However, this batching mechanism reduces the result to a single boolean value "true" or "false". The proposal is to replicate this behaviour but for each interface return a separate true/false value as sometimes you anticipate some interfaces will be implemented but not all and can tailor your logic around the interfaces supported. The benefit of the helper method vs looping over supportsInterface is that redundant checks to supportsERC165 aren't made.

馃摑 Details

Here is the code I've written which is pretty self-explanatory. If ERC165 isnt implemented on the contract then no checks can be made and an empty array is returned, else a boolean array is returned corresponding to the interface ids passed.

/**
     * @dev Returns a boolean array where each value corresponds to the
     * interfaces passed in and whether they're supported or not. This allows
     * you to batch check interfaces for a contract where your expectation
     * is that some interfaces may not be supported.
     *
     * See {IERC165-supportsInterface}.
     */
    function getSupportedInterfaces(address account, bytes4[] memory interfaceIds) public view returns (bool[] memory) {
        // query support of ERC165 itself
        if (!supportsERC165(account)) {
            //return an empty array to signify no checks could be made...
            return new bool[](0);
        }

        // an array of booleans corresponding to interfaceIds and whether they're supported or not
        bool[] memory interfaceIdsSupported = new bool[](interfaceIds.length);

        // query support of each interface in interfaceIds
        for (uint256 i = 0; i < interfaceIds.length; i++) {
            if (!_supportsERC165Interface(account, interfaceIds[i])) {
                interfaceIdsSupported[i] = false;
            } else {
                interfaceIdsSupported[i] = true;
            }
        }

        return interfaceIdsSupported;
    }

If this proposal is good and OZ is happy to go ahead with it, I can write tests and PR in.

@abcoathup
Copy link
Contributor

Hi @conspyrosy! Thanks for the suggestion, it is really appreciated.

The project owner will review your suggestion as soon as they can.

As per our conversation in the forum, please wait until we have discussed this idea before writing any code or submitting a Pull Request, so we can go through the design beforehand. We don鈥檛 want you to waste your time!

@frangio
Copy link
Contributor

frangio commented Dec 10, 2020

Hi @conspyrosy, I think this idea makes sense. Have you ran into the need for a function like this yourself?

Feel free to open a PR. I'm not sure if returning an empty array is the right choice in case the first check fails. Maybe if you explain the context where this is needed we can reason through it and find out what would be the best return value in that case.

@conspyrosy
Copy link
Contributor Author

@frangio Yes, i've ran into the need for this function which is why i'm proposing this. Let me explain my scenario:

Options protocols come in different shapes and sizes and support different functionality. I'm creating a generic OptionsAdapter that generifies options protocols into a single base interface so they can be interacted with in a uniform way. Additional functionality can be added on top of the base interface too. The adapter pattern is a fairly common pattern in software development. Here's the interfaces I have so far:

IOptionsProtocol - base interface (mandatory)
IDiscreteOptionsProtocol - for non-AMM type protocols where there is a rigid set of options
IResellableOptionsProtocol - for protocols with a secondary market to sell options
(probably more eventually, still a work in progress)

Users of the adapter will be able to pass in an arbitrary list of protocol addresses that conform to at least the base interface. So I want to form some kind of in memory object that describes the protocol features, which then informs me how i can interact with the protocol:

struct ProtocolFeatures {
    bool isDiscreteProtocol;
    bool areOptionsResellable;
    ...
}

That way I can pull the details of the protocol before looping some functionality and can also perform logic conditionally based on the functionality supported without having to make redundant calls per protocol action.

It's expected that ERC165 is always implemented on the addresses checked as the base interface (IOptionsProtocol) will implement ERC165. I think if the ERC165 method was not implemented it wouldnt really make sense to call this method as there's no way to check for interfaces that exist without it being implemented - that would imply the address isn't an IOptionsProtocol (which is actually an abstract contract implementing ERC165). The alternative to returning an empty array is reverting which I guess could be more appropriate since they passed in an invalid address?

@frangio
Copy link
Contributor

frangio commented Dec 11, 2020

The alternative to returning an empty array is reverting

I was actually thinking of returning an array of the same length where all values are false.

So in your case this is what I imagine: you would ask if the contract supports [IOptionsProtocol, IDiscreteOptionsProtocol, IResellableOptionsProtocol]. Then if the contract doesn't even implement ERC165 you would receive an array full of false values. You would check the first array value that corresponds to IOptionsProtocol which is the minimum interface that you need the contract to support, and then you would revert because you read a false value.

In fact the same would happen if the contract implements ERC165 but none of these interfaces, and I think it makes sense to treat those two situations uniformly.

@conspyrosy
Copy link
Contributor Author

conspyrosy commented Dec 11, 2020

@frangio I was initially against this as I thought it was possible that an interface was implemented without implementing ERC165 (in which case the result wouldn't strictly be true). But i've just remembered in the design mentioned ERC165 is on the base level interface which everything extends from so this will work perfectly.

I'll fix the code over the next week, add some tests then PR in.

@Amxx Amxx added the feature New contracts, functions, or helpers. label Jan 7, 2021
@Amxx
Copy link
Collaborator

Amxx commented Jan 7, 2021

@conspyrosy Are you still considering submitting a PR for this ?

@conspyrosy
Copy link
Contributor Author

conspyrosy commented Jan 11, 2021

@Amxx yes, bare with me as I'm very busy but I have the code. Just need to write a couple tests. I will try get it done over the next week.

@conspyrosy
Copy link
Contributor Author

@Amxx @frangio see this PR: #2469

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants