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

Disputable: Support payable actions #593

Merged
merged 3 commits into from
Jul 15, 2020
+18 −7
Merged

Disputable: Support payable actions #593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16f8ab5
Chore: Bump v5.0.0-beta.2
facuspagnuolo Jul 15, 2020
762621b
disputables: support payable actions
facuspagnuolo Jul 15, 2020
baacf10
disputable: enhance test case description
facuspagnuolo Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contracts/apps/disputable/DisputableAragonApp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

/**
Expand Down
2 changes: 1 addition & 1 deletion contracts/apps/disputable/IAgreement.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ contract IAgreement is IArbitrable, IACLOracle {

function deactivate(address _disputable) external;

function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external returns (uint256);
function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external payable returns (uint256);

function closeAction(uint256 _actionId) external;

Expand Down
4 changes: 2 additions & 2 deletions contracts/test/mocks/apps/disputable/AgreementMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ pragma solidity 0.4.24;


contract AgreementMock {
function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external returns (uint256) {
function newAction(uint256 /* _disputableActionId */, bytes /* _context */, address /* _submitter */) external payable returns (uint256) {
// do nothing
return 0;
}

function closeAction(uint256 _actionId) external {
function closeAction(uint256 /* _actionId */) external {
// do nothing
}
}
2 changes: 1 addition & 1 deletion contracts/test/mocks/apps/disputable/DisputableAppMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ contract DisputableAppMock is DisputableAragonApp {
initialized();
}

function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external {
function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external payable {
_newAgreementAction(_disputableActionId, _context, _submitter);
}

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@aragon/os",
"version": "5.0.0-beta.1",
"version": "5.0.0-beta.2",
"description": "Core contracts for Aragon",
"scripts": {
"compile": "truffle compile",
Expand Down
13 changes: 12 additions & 1 deletion test/contracts/apps/disputable/disputable_app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,25 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>
})

context('when the agreement is set', () => {
let agreement

beforeEach('set agreement', async () => {
const agreement = await AgreementMock.new()
agreement = await AgreementMock.new()
await disputable.setAgreement(agreement.address, { from: owner })
})

it('does not revert', async () => {
await disputable.newAction(0, '0x00', owner)
})

it('receives ETH when sent', async () => {
const previousBalance = await web3.eth.getBalance(agreement.address)

await disputable.newAction(0, '0x00', owner, { value: 1e18 })

const currentBalance = await web3.eth.getBalance(agreement.address)
assert.equal(currentBalance.sub(previousBalance).toString(), 1e18, 'agreement balance does not match')
})
})
})

Expand Down