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

Disputable: Support payable actions #593

Merged
merged 3 commits into from
Jul 15, 2020

Conversation

facuspagnuolo
Copy link
Contributor

No description provided.

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.

Couple of notes, but looking good!

test/contracts/apps/disputable/disputable_app.js Outdated Show resolved Hide resolved
@@ -129,7 +129,7 @@ contract DisputableAragonApp is IDisputable, AragonApp {
*/
function _newAgreementAction(uint256 _disputableActionId, bytes _context, address _submitter) internal returns (uint256) {
IAgreement agreement = _ensureAgreement();
return agreement.newAction(_disputableActionId, _context, _submitter);
return agreement.newAction.value(msg.value)(_disputableActionId, _context, _submitter);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we shouldn't expose this as a parameter instead; maybe the app takes a cut of ETH itself or etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the app manipulates it, the agreement app won't be able to pay fees, unless the user is sending more than the required amount. However, how would a parameter help here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I was imagining the user sending more fees to the app (because of app-specific logic).

As an example, in the case of a registry, maybe you need to stake some additional ETH into the app as well as pay the fee.

The parameter would help here because the app then has control over how much value is sent.

@coveralls
Copy link

coveralls commented Jul 15, 2020

Coverage Status

Coverage remained the same at 98.87% when pulling baacf10 on make_agreement_new_action_payable into 3c87906 on next.

Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>
@facuspagnuolo facuspagnuolo merged commit 348593f into next Jul 15, 2020
@facuspagnuolo facuspagnuolo deleted the make_agreement_new_action_payable branch July 15, 2020 13:07
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.

4 participants