Conversation
|
|
||
| contract IAragonApp { | ||
| function kernel() public view returns (IKernel); | ||
| function appId() public view returns (bytes32); |
There was a problem hiding this comment.
Should we leverage this change and make these external?
There was a problem hiding this comment.
Why? Those functions are being called by other contracts and in Solidity 4 we can’t override from external to public.
There was a problem hiding this comment.
I don't see a problem in introducing a breaking change if we are going to use semver properly to release a breaking version, if those contracts want to use this new version then they must update their usage
There was a problem hiding this comment.
Ok, but what’s the advantage of making them external?
There was a problem hiding this comment.
Well, it is the expected way to define interfaces in solc... you won't notice a warning here because it is defined as a contract, but otherwise it would complain. However, not a blocker at all, we still need to define what do we want to include in the next breaking version, so we can discuss whether to address this or not
Refactor ERC-165 related inheritance.
|
|
||
| contract IAragonApp { | ||
| function kernel() public view returns (IKernel); | ||
| function appId() public view returns (bytes32); |
There was a problem hiding this comment.
Well, it is the expected way to define interfaces in solc... you won't notice a warning here because it is defined as a contract, but otherwise it would complain. However, not a blocker at all, we still need to define what do we want to include in the next breaking version, so we can discuss whether to address this or not
sohkai
left a comment
There was a problem hiding this comment.
Have a couple more comments... maybe it would be worth discussing the abstractions offline :).
It feels natural that the
supportsInterfacemethod lives where the rest of the interface is defined
I wouldn't necessarily say so; ultimately the implementation contract has to decide which interfaces it'd like to publicly advertise. I also wouldn't couple an interface with ERC165, especially as a lot of ERC interfaces only suggest that a contract support ERC165.
Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>
Fixes #587
See: #586 (comment)