Skip to content

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

Merged
bingen merged 6 commits intonextfrom
add_disputable_base_app_transaction_fees_integration
Jun 30, 2020
Merged

Disputable apps: add missing pieces for transaction fees module#586
bingen merged 6 commits intonextfrom
add_disputable_base_app_transaction_fees_integration

Conversation

@bingen
Copy link
Contributor

@bingen bingen commented Jun 23, 2020

No description provided.

@bingen bingen requested review from facuspagnuolo and sohkai June 23, 2020 18:07
@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/apps/disputable/IDisputable.sol
return _interfaceId == DISPUTABLE_INTERFACE_ID || _interfaceId == ERC165_INTERFACE_ID;
}

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

@facuspagnuolo facuspagnuolo Jun 25, 2020

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

@bingen bingen Jun 26, 2020

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

@facuspagnuolo facuspagnuolo Jun 26, 2020

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

@bingen bingen Jun 29, 2020

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

@facuspagnuolo facuspagnuolo Jun 29, 2020

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

facuspagnuolo commented Jun 26, 2020

@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 approved these changes Jun 29, 2020
View reviewed changes
facuspagnuolo
facuspagnuolo reviewed Jun 30, 2020
View reviewed changes
contracts/apps/disputable/IAgreement.sol Outdated
contract IAgreement is IArbitrable, IACLOracle {

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

@facuspagnuolo facuspagnuolo Jun 30, 2020

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
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

@facuspagnuolo facuspagnuolo Jun 30, 2020

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

facuspagnuolo commented Jun 30, 2020

@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

2 more reviewers

@facuspagnuolo facuspagnuolo facuspagnuolo approved these changes

@sohkai sohkai sohkai approved these changes

Reviewers whose approvals may not affect merge requirements

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