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.

Sign up for GitHub

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

Jump to bottom

Disputable apps: add missing pieces for transaction fees module #586

Merged
merged 6 commits into from
Jun 30, 2020
Merged

Disputable apps: add missing pieces for transaction fees module #586

merged 6 commits into from
Jun 30, 2020

Conversation

bingen
Copy link
Contributor

@bingen bingen commented Jun 23, 2020

No description provided.

@bingen bingen self-assigned this Jun 23, 2020
@auto-assign auto-assign bot requested a review from izqui June 23, 2020 18:09
@bingen bingen mentioned this pull request Jun 23, 2020
Merged
@coveralls
Copy link

coveralls commented Jun 23, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 99.148% when pulling 91fd176 on add_disputable_base_app_transaction_fees_integration into 4b283dc on next.

facuspagnuolo
facuspagnuolo approved these changes Jun 25, 2020
View reviewed changes
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! Let's discuss about the external version for appId

contracts/lib/arbitration/ITransactionFeesOracle.sol Show resolved Hide resolved
contracts/lib/arbitration/IArbitrator.sol Outdated Show resolved Hide resolved
contracts/apps/disputable/IDisputable.sol
@@ -39,4 +39,6 @@ contract IDisputable is ERC165 {
function supportsInterface(bytes4 _interfaceId) external pure returns (bool) {
return _interfaceId == DISPUTABLE_INTERFACE_ID || _interfaceId == ERC165_INTERFACE_ID;
}

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.

Maybe we can leverage the new breaking version we are working on to 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.

What’s the benefit?

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid having the compiler warning:

Warning: Functions in interfaces should be declared 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.

Where do you see that warning? I can’t find it. Despite the name, IDisputable is not actually an interface, so the warning is not showing up for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure to be honest, however this is a minor detail, not a blocker at all

@bingen bingen mentioned this pull request Jun 26, 2020
Closed
Base automatically changed from add_disputable_base_app to next June 26, 2020 14:56
@bingen bingen force-pushed the add_disputable_base_app_transaction_fees_integration branch from 060a440 to addfa2a Compare June 26, 2020 17:29
@facuspagnuolo
Copy link
Contributor

@bingen I have merged the base disputable app PR against next. Feel free to rebase or pull to improve the diff for the reviews please 🙏

@bingen bingen changed the base branch from next to master June 29, 2020 15:32
@bingen bingen changed the base branch from master to next June 29, 2020 15:32
@bingen bingen removed the request for review from izqui June 29, 2020 15:37
facuspagnuolo
facuspagnuolo reviewed Jun 30, 2020
View reviewed changes
contracts/apps/disputable/IAgreement.sol Outdated
@@ -12,6 +12,7 @@ import "../../lib/arbitration/IArbitrable.sol";
contract IAgreement is IArbitrable, IACLOracle {

event Signed(address indexed signer, uint256 settingId);
event TransactionFeesOracleSet(address transactionFeesOracle);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this event and leverage this as a new part of the agreement settings

@bingen bingen force-pushed the add_disputable_base_app_transaction_fees_integration branch from 0514e12 to 3c202d8 Compare June 30, 2020 11:20
sohkai
sohkai reviewed Jun 30, 2020
View reviewed changes
contracts/apps/disputable/IDisputable.sol
@@ -39,4 +39,6 @@ contract IDisputable is ERC165 {
function supportsInterface(bytes4 _interfaceId) external pure returns (bool) {
return _interfaceId == DISPUTABLE_INTERFACE_ID || _interfaceId == ERC165_INTERFACE_ID;
}

function appId() public view returns (bytes32);
Copy link
Contributor

@sohkai sohkai Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Somewhat related discussion, but maybe we should create a new interface, e.g. IAragonApp to enforce the kernel() and appId() functions?

That way IDisputable can declare that it's always an IAragonApp (and we can adjust the EIP-165 values to be separated between IDisputable and IAragonApp).

We could also move out supportsInterface() out of the base app—it's kind of weird that it implements it as an app may choose to support any number of interfaces (e.g. the token receiver ones from 721 or 1155).

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of IAragonApp, I'll create an issue for that :)

sohkai
sohkai approved these changes Jun 30, 2020
View reviewed changes
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.

LGTM, although I'm not actually sure if the TransactionFeesOracle contract should live here (are users meant to import it?).

@facuspagnuolo
Copy link
Contributor

@sohkai the thing is that IAgreement relies on it, would you declare it in the agreement app directly?

My rationale between these interfaces that are being shared between Aragon Court and Aragon apps, is that they should live in aragonOS. Currently, we have an incompatibility problem since Aragon Court is using solc 0.5.8. Thus, these interfaces are being repeated in Aragon Court's repo. What do you think?

@facuspagnuolo facuspagnuolo mentioned this pull request Jun 30, 2020
Closed
@bingen bingen merged commit e948841 into next Jun 30, 2020
@facuspagnuolo facuspagnuolo deleted the add_disputable_base_app_transaction_fees_integration branch June 30, 2020 13:33
@sohkai
Copy link
Contributor

sohkai commented Jun 30, 2020 •
edited
Loading

Didn't see that we were using it in IAgreement. Sounds good to me then!

My rationale between these interfaces that are being shared between Aragon Court and Aragon apps, is that they should live in aragonOS

Maybe we should have an @aragon/contract-interfaces package or etc. to hold these, but not really sure I like that.

@bingen
Copy link
Contributor Author

bingen commented Jun 30, 2020

Didn't see that we were using it in IAgreement

There was a commit not yet pushed at the time of your comment ;-)

@bingen bingen mentioned this pull request Jul 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facuspagnuolo facuspagnuolo facuspagnuolo approved these changes

@sohkai sohkai sohkai approved these changes

Assignees

@bingen bingen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bingen @coveralls @facuspagnuolo @sohkai