add 165 to 721 #972
add 165 to 721 #972
Conversation
shrugs
force-pushed
the
feat/erc721-updates
branch
from
38b3b40
to
225b7bb
Compare
shrugs
force-pushed
the
feat/erc721-updates
branch
from
225b7bb
to
02fed58
Compare
There was a problem hiding this comment.
Nice job @shrugs. This seems pretty harmless to me. I have questions, but they shouldn't block this PR and if required, we can work on them later.
I love the tests that verify the hardcoded hashes!
As an aside, I would prefer changes like context to be done on separate PRs. But I know we have been doing a poor job reviewing. Once we catch up with the backlog and get this under control, I would love to have very small prs that we review and land quickly.
contracts/ERC165.sol
Outdated
| @@ -0,0 +1,20 @@ | |||
| pragma solidity ^0.4.23; | |||
There was a problem hiding this comment.
what would you think of moving this to contracts/introspection/ERC165.sol
contracts/ERC165.sol
Outdated
| * @notice Query if a contract implements an interface | ||
| * @param _interfaceId The interface identifier, as specified in ERC-165 | ||
| * @dev Interface identification is specified in ERC-165. This function | ||
| * @dev uses less than 30,000 gas. |
There was a problem hiding this comment.
Question here: what happens if there are too many methods and interfaces and this ends up consuming more than 30000 gas?
There was a problem hiding this comment.
The lookup will always use less than 30k gas; the gas limit would come in if we're doing some special calculations with a bunch of branches in the code, I think.
| /** | ||
| * @dev private method for registering an interface | ||
| */ | ||
| function _registerInterface(bytes4 _interfaceId) |
There was a problem hiding this comment.
should we check here that _interfaceId is not 0xffffffff?
And another question, where does that 0xffffffff comes from?
There was a problem hiding this comment.
The 0xff... is a magic value. If it returns true for that query, the chances of the contract having implemented the function supportsInterface(bytes4) is infinitesimally low (since the return value of a function which has no return value is garbage data, which is, 99.99999999...% of the time != false. so if the function returns true, it wasn't implemented.
There was a problem hiding this comment.
Interesting. I wonder why this is not explained in the EIP.
There was a problem hiding this comment.
yeah I had to come to that conclusion on my own; maybe it's an existing pattern? comments don't mention it either Β―_(γ)_/Β―
| event Transfer( | ||
| address indexed _from, | ||
| address indexed _to, | ||
| uint256 _tokenId | ||
| uint256 indexed _tokenId |
There was a problem hiding this comment.
do we have a policy for which ones to make indexed?
There was a problem hiding this comment.
This is actually looking at the current spec and making sure it's conformant.
| * @notice Query if a contract implements an interface | ||
| * @param _interfaceId The interface identifier, as specified in ERC-165 | ||
| * @dev Interface identification is specified in ERC-165. This function | ||
| * @dev uses less than 30,000 gas. |
There was a problem hiding this comment.
Should we add a test to check gas usage? While the current lookup implementation will always be below the limit, I feel this would be nice to have for completeness, and to safeguard against future changes to said implementation.
| /** | ||
| * @dev private method for registering an interface | ||
| */ | ||
| function _registerInterface(bytes4 _interfaceId) |
There was a problem hiding this comment.
Hate to be a blocker for this but I think this function should require(_interfaceId != 0xffffffff) due to the spec of supportsInterface (link):
@return `true` if the contract implements `interfaceID` and
`interfaceID` is not 0xffffffff, `false` otherwise
(Alternatively we could add the precondition to supportsInterface but I think the registering helper is the right place.)
There was a problem hiding this comment.
I just saw that there was a previous comment about this. I would still add this line just to be super clear that we're correctly implementing the spec.
There was a problem hiding this comment.
@frangio I thought the purpose of the magic value was to test the implementation as well; if the implementation, without the check, returns true for 0xffffffff, it shouldn't be trusted. And the default value for the key 0xffffffff in the supportedInterfaces map is false, so we theoretically shouldn't need a special check.
Like how in the ERC165MappingImplementation from https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md#implementation they just say "don't set 0xffffffff to true".
|
Thank you all! |
Fixes #840
π Description
Adds 165 to 721 using lookup table.
npm run lint:all:fix).