Skip to content
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 IAragonApp interface #597

Merged
merged 11 commits into from Jul 28, 2020
Merged

Add IAragonApp interface #597

merged 11 commits into from Jul 28, 2020

Conversation

bingen
Copy link
Contributor

@bingen bingen commented Jul 20, 2020

Fixes #587
See: #586 (comment)

@coveralls
Copy link

coveralls commented Jul 20, 2020

Coverage Status

Coverage increased (+0.003%) to 98.505% when pulling 122850f on aragon_app_interface into bf226a2 on next.


contract IAragonApp {
function kernel() public view returns (IKernel);
function appId() public view returns (bytes32);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we leverage this change and make these external?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Those functions are being called by other contracts and in Solidity 4 we can’t override from external to public.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but what’s the advantage of making them external?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

contracts/apps/AppStorage.sol Show resolved Hide resolved
Refactor ERC-165 related inheritance.
@bingen bingen marked this pull request as ready for review July 21, 2020 21:41
Copy link
Contributor

@facuspagnuolo facuspagnuolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


contract IAragonApp {
function kernel() public view returns (IKernel);
function appId() public view returns (bytes32);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a couple more comments... maybe it would be worth discussing the abstractions offline :).

It feels natural that the supportsInterface method 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.

contracts/apps/disputable/IDisputable.sol Outdated Show resolved Hide resolved
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/disputable/IDisputable.sol Outdated Show resolved Hide resolved
@bingen bingen merged commit 5d02845 into next Jul 28, 2020
@bingen bingen deleted the aragon_app_interface branch July 28, 2020 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apps: Create IAragonApp interface
4 participants