-
Notifications
You must be signed in to change notification settings - Fork 12k
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
Add ERC165Query library #1086
Changes from 4 commits
e226781
bf13df9
c9eef38
9babb73
bfee4e0
3176956
1a09d88
1ec887e
53c3c22
d3a42be
2535886
2b48005
92d7869
f0829ad
f62067b
111d588
9de7979
0b08abc
f6d8063
e228f81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
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 | ||
*/ | ||
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)') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
||
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 supportsInterface(_address, InterfaceId_ERC165) && | ||
!supportsInterface(_address, InterfaceId_Invalid); | ||
} | ||
|
||
/** | ||
* @notice Query if a contract implements an interface | ||
* @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 | ||
* with the `supportsERC165` method in this library. | ||
* Interface identification is specified in ERC-165. | ||
*/ | ||
function supportsInterface(address _address, bytes4 _interfaceId) | ||
internal | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had changed this to Since we now have the Let me know if you agree and I can make this change. |
||
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 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 ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would try to find a name for I also found a problem with this function. If There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can be done with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If i understand correctly, you would prefer something like:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The On second thought, I have a few more doubts about it. Are we sure that the result of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ldub I was able to get 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 | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: trailing newline should be there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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 { | ||
|
||
} |
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); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
pragma solidity ^0.4.24; | ||
|
||
|
||
contract ERC165NotSupported { | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
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 supportsInterface(address _address, bytes4 _interfaceId) | ||
public | ||
view | ||
returns (bool) | ||
{ | ||
return _address.supportsInterface(_interfaceId); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
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') | ||
.use(require('chai-as-promised')) | ||
.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); | ||
}); | ||
}); | ||
|
||
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}`, 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}`, async function () { | ||
const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID); | ||
supported.should.eq(true); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed