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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ERC165Query library #1086

Merged
merged 20 commits into from
Aug 22, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions contracts/introspection/ERC165Checker.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
pragma solidity ^0.4.24;


/**
* @title ERC165Checker
* @dev Use `using ERC165Checker for address`; to include this library
* https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

*/
library ERC165Checker {
// As per the EIP-165 spec, no interface should ever match 0xffffffff
bytes4 private constant InterfaceId_Invalid = 0xffffffff;

// As per the EIP-165 spec, ERC165_ID == bytes4(keccak256('supportsInterface(bytes4)')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)'))
   */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@nventuro nventuro Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bytes4 private constant InterfaceId_ERC165 = 0x01ffc9a7;

/**
* @notice Query if a contract implements an interface, does not check ERC165 support
* @param _address The address of the contract to query for support of an interface
* @param _interfaceId The interface identifier, as specified in ERC-165
* @return true if the contract at _address indicates support of the interface with
* identifier _interfaceId, false otherwise
* @dev Assumes that _address contains a contract that supports ERC165, otherwise
* the behavior of this method is undefined. This precondition can be checked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation

* with the `supportsERC165` method in this library.
* Interface identification is specified in ERC-165.
*/
function supportsERC165Interface(address _address, bytes4 _interfaceId)
internal
Copy link
Contributor

@frangio frangio Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be private IMO, because it should never be used before also checking supportsERC165.

Edit: I just saw that this was previously discussed here, and decided against making it private because without it checking for many interfaces would be more costly. Perhaps we could provide a supportsInterfaces(address _address, bytes4[] _interfaceIds) to cater this use case? cc @shrugs @nventuro

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like your supportInterfaces API more than what we have now. Happy to implement this, lets wait for feedback from others.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too! I think that makes much more sense, I wasn't too happy with the naming scheme we'd come up with.

view
returns (bool)
{
// success determines whether the staticcall succeeded and result determines
// whether the contract at _address indicates support of _interfaceId
(bool success, bool result) = noThrowCall(_address, _interfaceId);

return (success && result);
}

/**
* @notice Query if a contract supports ERC165
* @param _address The address of the contract to query for support of ERC165
* @return true if the contract at _address implements ERC165
*/
function supportsERC165(address _address)
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(_address, InterfaceId_ERC165) &&
!supportsERC165Interface(_address, InterfaceId_Invalid);
}

/**
* @notice Query if a contract implements an interface, also checks support of ERC165
* @param _address The address of the contract to query for support of an interface
* @param _interfaceId The interface identifier, as specified in ERC-165
* @return true if the contract at _address indicates support of the interface with
* identifier _interfaceId, false otherwise
* @dev Interface identification is specified in ERC-165.
*/
function supportsInterface(address _address, bytes4 _interfaceId)
internal
view
returns (bool)
{
// query support of both ERC165 as per the spec and support of _interfaceId
return supportsERC165(_address) && supportsERC165Interface(_address, _interfaceId);
}

/**
* @notice Calls the function with selector 0x01ffc9a7 (ERC165) and suppresses throw
* @param _address 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 _address
* indicates support of the interface with identifier _interfaceId, false otherwise
*/
function noThrowCall (
Copy link
Contributor

@frangio frangio Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try to find a name for noThrowCall that is less generic, and make it clear that it's specific for ERC165 supportsInterface. It should also be private.

I also found a problem with this function. If _address is not a contract, it will return success = true. I don't think we can assume that the free memory pointer points at zeroed memory, so it may also return result = true. I think we will be ok if we store a zero in result output before the staticcall.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to your implementation. thanks. also, assuming you mean 'store a zero in output' rather than 'store a zero in result', which is what I've done and also added a test for this.

address _address,
bytes4 _interfaceId
)
internal
view
returns (bool success, bool result)
{
bytes4 erc165ID = InterfaceId_ERC165;

// solium-disable-next-line security/no-inline-assembly
assembly {
let x := mload(0x40) // Find empty storage location using "free memory pointer"
mstore(x, erc165ID) // Place signature at begining of empty storage
mstore(add(x, 0x04), _interfaceId) // Place first argument directly next to signature
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. πŸ˜…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
}


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

result := mload(x) // Load the result
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: trailing newline should be there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont know if my git is doing something weird but i definitely have a new line here locally.. investigating

8 changes: 8 additions & 0 deletions contracts/mocks/ERC165/ERC165GenerallySupported.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
pragma solidity ^0.4.24;

import "../../introspection/SupportsInterfaceWithLookup.sol";


contract ERC165GenerallySupported is SupportsInterfaceWithLookup {

}
12 changes: 12 additions & 0 deletions contracts/mocks/ERC165/ERC165InterfaceSupported.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
pragma solidity ^0.4.24;

import "../../introspection/SupportsInterfaceWithLookup.sol";


contract ERC165InterfaceSupported is SupportsInterfaceWithLookup {
constructor (bytes4 _interfaceId)
public
{
_registerInterface(_interfaceId);
}
}
6 changes: 6 additions & 0 deletions contracts/mocks/ERC165/ERC165NotSupported.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pragma solidity ^0.4.24;


contract ERC165NotSupported {

}
32 changes: 32 additions & 0 deletions contracts/mocks/ERC165CheckerMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
pragma solidity ^0.4.24;

import "../introspection/ERC165Checker.sol";


contract ERC165CheckerMock {
using ERC165Checker for address;

function supportsERC165(address _address)
public
view
returns (bool)
{
return _address.supportsERC165();
}

function supportsERC165Interface(address _address, bytes4 _interfaceId)
public
view
returns (bool)
{
return _address.supportsERC165Interface(_interfaceId);
}

function supportsInterface(address _address, bytes4 _interfaceId)
public
view
returns (bool)
{
return _address.supportsInterface(_interfaceId);
}
}
73 changes: 73 additions & 0 deletions test/introspection/ERC165Checker.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
const ERC165Checker = artifacts.require('ERC165CheckerMock');
const ERC165NotSupported = artifacts.require('ERC165NotSupported');
const ERC165GenerallySupported = artifacts.require('ERC165GenerallySupported');
const ERC165InterfaceSupported = artifacts.require('ERC165InterfaceSupported');

const DUMMY_ID = '0xdeadbeef';

require('chai')
.should();

contract('ERC165Checker', function (accounts) {
before(async function () {
this.mock = await ERC165Checker.new();
});

context('not supported', () => {
beforeEach(async function () {
this.target = await ERC165NotSupported.new();
});

it('does not support ERC165', async function () {
const supported = await this.mock.supportsERC165(this.target.address);
supported.should.eq(false);
});

it(`does not support ${DUMMY_ID} via supportsInterface`, async function () {
const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID);
supported.should.eq(false);
});
});

context('generally supported', () => {
beforeEach(async function () {
this.target = await ERC165GenerallySupported.new();
});

it('supports ERC165', async function () {
const supported = await this.mock.supportsERC165(this.target.address);
supported.should.eq(true);
});

it(`does not support ${DUMMY_ID} via supportsERC165Interface`, async function () {
const supported = await this.mock.supportsERC165Interface(this.target.address, DUMMY_ID);
supported.should.eq(false);
});

it(`does not support ${DUMMY_ID} via supportsInterface`, async function () {
const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID);
supported.should.eq(false);
});
});

context('interface supported', () => {
beforeEach(async function () {
this.target = await ERC165InterfaceSupported.new(DUMMY_ID);
});

it('supports ERC165', async function () {
const supported = await this.mock.supportsERC165(this.target.address);
supported.should.eq(true);
});

it(`supports ${DUMMY_ID} via supportsERC165Interface`, async function () {
const supported = await this.mock.supportsERC165Interface(this.target.address, DUMMY_ID);
supported.should.eq(true);
});

it(`supports ${DUMMY_ID} via supportsInterface`, async function () {
const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID);
supported.should.eq(true);
});
});
});