-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
eip165 - Pseudo-Introspection, or standard interface detection #639
Conversation
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.
Agh, it seems I never submitted this review.
EIPS/eip-165.md
Outdated
|
||
##### Procedure to determine if a contract implements EIP165 standard. | ||
|
||
1. The source contact makes a `CALL` to the destination address with input data: 0x01ffc9a701ffc9a7 value: 0 and gas 30000 |
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.
This should properly be padded to the right with zeroes, but I think it would be clearer to annotate this at function level - eg "the source contract calls supportsInterface(0x01ffc9a7)
".
EIPS/eip-165.md
Outdated
|
||
1. The source contact makes a `CALL` to the destination address with input data: 0x01ffc9a701ffc9a7 value: 0 and gas 30000 | ||
2. If the call fails or return false, the destination contract does not implement EIP165 | ||
3. If the call returns true, a second call is made with input data: 0x01ffc9a7ffffffff |
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.
Is this necessary? The chances of a contract randomly returning true to everything seem pretty low.
Also, I suspect that in most cases we know the target contract implements supportsInterface
, but not which interfaces it implements - such as is the case with ENS resolvers, for instance.
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.
A typical example could be to ask if a wallet implements ITokenFallback
. In most cases, this call will throw
or return false. The return true
case, I see it very unprovable now, but after metropolis I see the possibility that some day solidity could be able to return values in the fallback function. (This could be very useful for implementing proxy contracts for example). I also do not control other languages like serpent or direct assembly contracts. So because of this possibility and to be sure there is no edge cases, I implemented the "Double call", But if you don't see it probable, we can remove this double check and remve point 3.
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.
Personally I don't think it's necessary.
|
||
Returns true if `_contract` supports EIP165 | ||
|
||
function interfaceSupported(address _contract, bytes4 _interfaceId) constant returns (bool); |
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.
Checking if the contract implements EIP165 aside, will this be any more efficient than just asking the contract directly?
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.
If the contract throws
(because it is an old one) it will consume all the gas.... So in this case I suspect that it's much more efficient to have a cache
. For a new contract (and if we remove the double call), probably it will not be very different.
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.
Good point on the throw
.
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.
If we assume that we will use a cache, then may be it's not so important to remove the "double call" in terms of performance (It will be done only once by the cache contract). And this way we don't risk for edge cases.
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.
True; how about rewording it such that it's optional? In cases where you already know the target contract implements supportsInterface
, there's no point in calling it more than once.
Please check #672 as alternative to this EIP. |
I'd still like to standardise this. ENS can't use #672 without massive ouroboros confusion. ;) |
|
||
##### Procedure to determine if a contract implements EIP165 standard. | ||
|
||
1. The source contact makes a `CALL` to the destination address with input data: 0x01ffc9a701ffc9a7 value: 0 and gas 30000. `0x01ffc9a701ffc9a7` is the data for `supportsInterface(0x01ffc9a7)`. Asks if self implements itself. |
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.
I still think this needs to be expressed as an ABI, not as raw bytes.
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.
The problem is that in solidity you cannot catch the throw inside a call by just calling in the ABI way. You will have to use the call
by hand and check the result. I added the explanation in the same line, but I think is worthy to just explain how the internal call is make. Please fill free to rewrite this paragraph in a way you fill more confortable.
EIPS/eip-165.md
Outdated
@@ -0,0 +1,229 @@ | |||
## Preamble | |||
|
|||
EIP: <to be assigned> |
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.
This needs setting.
This function should be |
I don't like how the contract needs to know about the cache. This is surely more complicated than need be and the control flow is inverted. Just make things simple! |
The specification need not be any more complicated than this:
Everything else is just delaying review and stalling this from becoming an actual standard. |
Formal PR for Pseudo-Introspection of #165