Disputable: Implement disputable base app #581
Conversation
facuspagnuolo
commented
May 31, 2020
•
facuspagnuolo
commented
facuspagnuolo
force-pushed
the
add_disputable_base_app
branch
from
b028282
to
9844c39
Compare
|
@sohkai thanks again for the review! I addressed almost all your comments, let me know your thoughts! |
| @@ -10,14 +10,20 @@ contract('Arbitrable', () => { | |||
| describe('supportsInterface', () => { | |||
| it('supports ERC165', async () => { | |||
| assert.isTrue(await arbitrable.supportsInterface('0x01ffc9a7'), 'does not support ERC165') | |||
|
|
|||
| assert.equal(await arbitrable.ERC165_INTERFACE(), '0x01ffc9a7', 'ERC165 interface ID does not match') | |||
| assert.equal(await arbitrable.erc165interfaceID(), await arbitrable.ERC165_INTERFACE(), 'ERC165 interface ID does not match') | |||
There was a problem hiding this comment.
On the interface, I like having these tests, but was mostly curious as to how we came up with our own magic numbers for IDisputable.
Are we doing the sanity check via https://github.com/aragon/aragonOS/pull/581/files#diff-a4bee50cd93746db86bb3d3cef7232ebR26-R32? Again, just wondering if there's an automated tool to generate these based on an interface's ABI, so we literally can't miss it if we ever change something.
There was a problem hiding this comment.
On the interface, I like having these tests, but was mostly curious as to how we came up with our own magic numbers for
IDisputable.
It wasn't magic haha, I computed them manually before writing the tests
Are we doing the sanity check via https://github.com/aragon/aragonOS/pull/581/files#diff-a4bee50cd93746db86bb3d3cef7232ebR26-R32? Again, just wondering if there's an automated tool to generate these based on an interface's ABI, so we literally can't miss it if we ever change something.
The link is not working for me, although I see your point. I haven't found any tool to compute interface IDs for ERC165, we could probably write a tool to have this but we will need to force a weird interface inheritance in the contracts (or make sure we don't inherit at all) to make sure the IDs are computed correctly. Specially thinking about IArbitrable and IDisputable that inherit from ERC165, which makes me think we will definitely moving the potential error to another place instead of solving it.
There was a problem hiding this comment.
we will need to force a weird interface inheritance in the contracts
Hmm interesting, can the EIP165 interface differ from being the XOR of all external function selectors in a given interface (e.g. IDisputable's)? If so, could we take the ABI to automatically generate the selector in tests, to make sure ours match?
For example, I would've expected our IDisputable interface identifier to change since we now have more exposed methods.
There was a problem hiding this comment.
I would've expected our IDisputable interface identifier to change since we now have more exposed methods.
It does indeed, that's the con, we are not automatically generating it. I need to update it :/
we will need to force a weird interface inheritance in the contracts
What I meant here is that in order to build an automatic tool, it is not possible to distinguish between the methods coming from inherited contracts, and that's the case for IDisputable and IAgreement. Both carry their parent's methods, and it is not possible to identify them from the compilation output, we could use the AST tho but it will take a bit more of time :)
There was a problem hiding this comment.
it is not possible to distinguish between the methods coming from inherited contracts, and that's the case for
IDisputableandIAgreement
Gotcha. This kind of begs another question: does EIP-165 say anything about this case? In theory, if your interface inherits from another interface, you should consider this new interface as the combination of the whole, not just the new parts.
facuspagnuolo
force-pushed
the
add_disputable_base_app
branch
from
ee51e13
to
07635d1
Compare
|
Thanks @sohkai! Let me know your thoughts about the last tweaks! |
|
Thanks once again @sohkai, addressed/answered all your comments, let me know your thoughts! |
There was a problem hiding this comment.
Looking good and as with the others, just pushed a few cosmetic commits.
I have a couple last notes on the interfaces; let me know what your thoughts are!
(Also left a comment above, in case it gets lost: #581 (comment))
|
@sohkai addressed and answered all your comments! Let me know your thoughts |
