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
Changes from 1 commit
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
83 changes: 83 additions & 0 deletions contracts/introspection/ERC165Query.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
pragma solidity ^0.4.20;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please target ^0.4.24.

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



/**
* @title ERC165Query
* @dev https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md
*/
library ERC165Query {
bytes4 constant INVALID_ID = 0xffffffff;
bytes4 constant ERC165_ID = 0x01ffc9a7;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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


/**
* @notice Query if a contract implements an interface
* @param _contract 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 _contract indicates support of the interface with
* identifier _interfaceId, false otherwise
* @dev Interface identification is specified in ERC-165. This function
* uses less than 30,000 gas.
*/
function doesContractImplementInterface (
address _contract,
bytes4 _interfaceId
)
external
view
returns (bool)
{
uint256 success;
uint256 result;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)


(success, result) = noThrowCall(_contract, ERC165_ID);
if ((success==0)||(result==0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

return false;
}

(success, result) = noThrowCall(_contract, INVALID_ID);
if ((success==0)||(result!=0)) {
return false;
}

(success, result) = noThrowCall(_contract, _interfaceId);
if ((success==1)&&(result==1)) {
return true;
}
return false;
}

/**
* @notice Calls the function with selector 0x01ffc9a7 (ERC165) and suppresses throw
* @param _contract 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 _contract
* indicates support of the interface with identifier _interfaceId, false otherwise
*/
function noThrowCall (
address _contract,
bytes4 _interfaceId
)
internal
view
returns (uint256 success, uint256 result)
{
bytes4 erc165ID = ERC165_ID;

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'm not an inline assembly expert, but should't we also update the free memory pointer at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


success := staticcall(
30000, // 30k gas
_contract, // 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
}
}
}