Agreement: Implement protocol for disputable apps #1133

facuspagnuolo · 2020-04-15T19:50:24Z

Fixes #1132

apps/agreement/contracts/Agreement.sol

izqui · 2020-04-20T19:19:03Z

apps/agreement/contracts/Agreement.sol

+            _acceptChallenge(action, setting);
+            emit ActionRejected(dispute.actionId);
+        } else {
+            _voidChallenge(action, setting);


In this case, is it possible that Aragon Court could have refunded fees (in case no jurors voted for example) and do we want to handle it?

We shouldn't handle that here cause it will occur when penalties are settled in court. But it's an edge case we should consider somehow.

apps/agreement/contracts/Agreement.sol

facuspagnuolo · 2020-04-21T22:54:28Z

@izqui thx for the review and the thorough feedback, I addressed almost all your comments. There are a few pending to be discussed

facuspagnuolo · 2020-04-22T12:20:10Z

For the record, the Agreement app contract is super close to size limit, currently it is 23,928

izqui

Still haven't looked at the tests in depth. The biggest thing we need to decide on is how to manage the challenge permission

apps/agreement/test/agreement_integration.js

apps/agreement/test/agreement_permissions.js

apps/agreement/contracts/Agreement.sol

facuspagnuolo · 2020-04-24T00:11:02Z

@izqui thx for the review again, I changed the contract to allow having an optional token balance permission for challenges similar to how it was implemented for the signing process

sohkai

Looking really, really good! I've pushed a few mostly cosmetic commits to align some of the comments or clarify in places where I could :).

I've left the review comments in order of priority / severity that we may want to look at. For the most part, I'm very comfortable with the current mechanisms and these are just small gaps that I may have noticed.

Finally, I'd like to go through and do a sanity check for the following:

Add @dev notes to our external/public functions to explain why they're implicitly protected against initialization (see Voting as an example—a lot of functions explain how they implicitly don't need an hasInitialized modifier)
Order our storage setters in generally the same order as they're defined in the struct (can lead to slightly better gas) (e.g. I saw an example with CollateralRequirements being in the wrong order)

apps/agreement/contracts/Agreement.sol

apps/agreement/arapp.json

apps/agreement/contracts/example/RegistryApp.sol

apps/agreement/package.json

apps/agreement/contracts/test/mocks/helpers/TimeHelpersMock.sol

apps/agreement/test/helpers/assert/assertBn.js

* Integrate external Staking app * Staking integration: adapt to new names * test: fix wrapper names Also, fix token deposit errors * Upgrade Staking dependency * Fixes after moving moving responsibilities to disputable app - action end date - cannot challenge error * Fixes after rebase on moving responsibilities to disputable app * Update Staking dependency * Fixes after rebase on moving responsibilities to disputable app Address PR #1151 comments. * Clean _unlockAndSlashBalance Fix dependencies (https://github.com/cgewecke/web3-issue-3544) * fixup! Clean _unlockAndSlashBalance Co-authored-by: Facu Spagnuolo <facuspagnuolo@users.noreply.github.com>

facuspagnuolo · 2020-06-13T12:53:19Z

@sohkai thanks once again! I addressed most of your comments and answered others, let me know your thoughts!

Broken due to web3 1.2.8 issue and decode events.

apps/agreement/contracts/Agreement.sol

facuspagnuolo · 2020-06-27T12:12:34Z

@sohkai addressed some last comments and conversations we had off-line, let me know your thoughts!

sohkai

Everything's looking good to me; I have a few last notes but I think we're good to go 🚢🛳⛴

sohkai · 2020-06-29T19:30:39Z

apps/agreement/test/agreement/agreement_gas_cost.js

@@ -31,7 +31,7 @@ contract('Agreement', ([_, user]) => {
    })

    context('newAction', () => {


This is a bit awkward—I was hoping we'd lower the gas costs 😅.

Was this because of the aragonOS beta version or changes to the Agreement app and its ids?

Honestly don't know, I would need to check what change introduce this increase :/
We can create another issue to look into it with fresh eyes :)

apps/agreement/contracts/Agreement.sol

* agreements: implement contracts * agreement: init project * agreement: implement happy path tests * agreement: do not allow changing collateral token * agreement: organize errors * agreement: small contract fixes * agreement: complete unit tests * agreement: submit evidence only if there is context on dispute * agreement: implement gas costs tests * agreement: fix authentication logic * agreement: allow challenger to claim settlements * agreements: add settings tests * agreement: model token-balance based permissions * agreements: simplify state assertion logic * agreements: use safe transfers always * agreements: simplify periods validation logic * agreements: use challenge collateral instead of multiplier * agreement: support forwarding interface * agreement: simplify token balance based permissions * agreement: do not allow changing the arbitrator * agreements: add information to emitted events * agreements: improve blacklist for evm scripts * agreements: improve arbitrator fees approval * agreements: implement intergration tests * agreements: add contract inline documentation * agreement: add authentication check when scheduling actions * agreement: update readme * agreement: allow having token balance challenge permissions * agreement: add arapp json file * agreement: add manifest json file * agreement: migrate tests to Truffle v5 * agreements: tell whether a signer must review the content or not * chore: use buidler to run tests * Agreements: Implement disputable app (aragon#1140) * agreements: implement explicit signing process * agreements: implement agreement executors architecture * agreements: remove old executor * agreements: rename "executor" to "disputable" * agreements: migrate back from buidler to truffle * agreements: add test script * agreements: fix clock mocking * agreements: update delay app gas costs * agreements: optimize collateral requirements storage * agreements: use shared staking pool (aragon#1138) * agreements: improve disputable interface naming * disputable: allow setting agreement after initialization * agreements: improve challenge callback * agreements: test disputable voting app * delay: remove embedded token balance permission * agreements: support ACL oracle interface * agreements: implement registration process * disputable: add sample registry app * chore: remove voting app dependency * disputable: add can proceed helper * disputable: supports ERC165 * agreement: multiple enhancements * disputable: allow working without agreement * agreement: optimize bytecode * agreement: remove unregistering state * agreements: support multiple challenges * agreement: add action lifetime config * agreement: allow changing the arbitrator address * agreement: improve architecture naming * agreement: improve dispute metadata * agreement: move disputable base app to aragonOS * agreement: update arapp json file * agreement: fix circular dependency * agreement: skip flaky test * agreement: poc moving responsibilities to disputable app * agreement: poc moving responsibilities to disputable app 2 * agreement: poc moving responsibilities to disputable app 3 * agreement: fix action getter * Agreement: update README and details * Agreement: typo fix in manifest.json * Agreement: cosmetic import re-ordering * Agreement: cosmetic comment rewording/clarification * Agreement: cosmetic code naming clarifications * Agreement: small state optimization in _canProceed() * Agreement: Integrate external Staking app (aragon#1151) * Integrate external Staking app * Staking integration: adapt to new names * test: fix wrapper names Also, fix token deposit errors * Upgrade Staking dependency * Fixes after moving moving responsibilities to disputable app - action end date - cannot challenge error * Fixes after rebase on moving responsibilities to disputable app * Update Staking dependency * Fixes after rebase on moving responsibilities to disputable app Address PR aragon#1151 comments. * Clean _unlockAndSlashBalance Fix dependencies (https://github.com/cgewecke/web3-issue-3544) * fixup! Clean _unlockAndSlashBalance Co-authored-by: Facu Spagnuolo <facuspagnuolo@users.noreply.github.com> * agreement: multiple enhancements * Fix CI (aragon#1167) Broken due to web3 1.2.8 issue and decode events. * Agreement: update comments and docstrings * agreements: follow same ID pattern for all objects * agreements: use aragon os v5 beta * agreements: fix tests * agreements: small enhancements * agreements: optimize disputable activation * agreements: fix gas cost tests Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com> Co-authored-by: ßingen <bingen@aragon.one>

facuspagnuolo requested review from izqui and sohkai April 15, 2020 19:50

facuspagnuolo self-assigned this Apr 15, 2020

probot-autolabeler bot added the component: contract label Apr 15, 2020

auto-assign bot requested review from bpierre and Evalir April 15, 2020 19:50

facuspagnuolo mentioned this pull request Apr 15, 2020

Agreements: Implement app #1132

Closed

15 tasks

facuspagnuolo removed request for bpierre and Evalir April 15, 2020 19:51

sohkai mentioned this pull request Apr 17, 2020

Organization Agreements: contract POC aragonone/product#305

Open

6 tasks

izqui reviewed Apr 20, 2020

View reviewed changes

facuspagnuolo requested a review from izqui April 21, 2020 22:54

facuspagnuolo force-pushed the implement_agreements_app branch from f216d85 to fea4106 Compare April 22, 2020 23:03

facuspagnuolo changed the title ~~Agreement: Implement smart contract~~ Agreement: Implement app Apr 23, 2020

facuspagnuolo changed the title ~~Agreement: Implement app~~ Agreement: Implement smart contract app Apr 23, 2020

izqui reviewed Apr 23, 2020

View reviewed changes

facuspagnuolo force-pushed the implement_agreements_app branch from 04474b8 to 471cb06 Compare April 27, 2020 20:53

This was referenced Apr 30, 2020

Agreements: Split staking contract #1138

Merged

Agreements: Implement explicit signing #1139

Closed

facuspagnuolo mentioned this pull request May 12, 2020

Agreements: Implement disputable app #1140

Merged

6 tasks

facuspagnuolo changed the base branch from master to next May 22, 2020 12:02

facuspagnuolo force-pushed the implement_agreements_app branch from bb6a6a9 to 9a253bb Compare May 22, 2020 12:06

probot-autolabeler bot added app: agent component: frontend labels May 22, 2020

facuspagnuolo force-pushed the implement_agreements_app branch from 9a253bb to 7e90c4f Compare May 22, 2020 12:13

facuspagnuolo changed the title ~~Agreement: Implement smart contract app~~ Agreement: Implement protocol for disputable apps May 27, 2020

facuspagnuolo force-pushed the implement_agreements_app branch from c930112 to 5c3cb11 Compare June 1, 2020 04:14

facuspagnuolo and others added 9 commits June 7, 2020 20:55

agreement: poc moving responsibilities to disputable app 2

4caf89b

agreement: poc moving responsibilities to disputable app 3

9d99ed4

agreement: fix action getter

80d499c

Agreement: update README and details

8edaa2f

Agreement: typo fix in manifest.json

f03b4f6

Agreement: cosmetic import re-ordering

3ef1bbf

Agreement: cosmetic comment rewording/clarification

ae6dc11

Agreement: cosmetic code naming clarifications

1876d42

Agreement: small state optimization in _canProceed()

f698674

sohkai reviewed Jun 12, 2020

View reviewed changes

ßingen and others added 2 commits June 12, 2020 19:01

agreement: multiple enhancements

4121234

Fix CI (#1167)

016c0cc

Broken due to web3 1.2.8 issue and decode events.

bingen reviewed Jun 22, 2020

View reviewed changes

apps/agreement/contracts/Agreement.sol Show resolved Hide resolved

sohkai mentioned this pull request Jun 25, 2020

Migrate helpers from Agreements aragon/contract-helpers#34

Closed

sohkai reviewed Jun 26, 2020

View reviewed changes

apps/agreement/contracts/Agreement.sol Show resolved Hide resolved

apps/agreement/contracts/Agreement.sol Outdated Show resolved Hide resolved

apps/agreement/contracts/Agreement.sol Outdated Show resolved Hide resolved

sohkai reviewed Jun 26, 2020

View reviewed changes

apps/agreement/contracts/Agreement.sol Outdated Show resolved Hide resolved

apps/agreement/contracts/Agreement.sol Show resolved Hide resolved

sohkai and others added 4 commits June 26, 2020 18:11

Agreement: update comments and docstrings

061896a

agreements: follow same ID pattern for all objects

a9e72dc

agreements: use aragon os v5 beta

7b0e41c

agreements: fix tests

6d94937

sohkai approved these changes Jun 29, 2020

View reviewed changes

facuspagnuolo added 3 commits June 29, 2020 17:48

agreements: small enhancements

f2369db

agreements: optimize disputable activation

396954a

agreements: fix gas cost tests

8ac7e07

facuspagnuolo merged commit 82aecfb into master Jun 30, 2020

facuspagnuolo deleted the implement_agreements_app branch June 30, 2020 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Agreement: Implement protocol for disputable apps #1133

Agreement: Implement protocol for disputable apps #1133

facuspagnuolo commented Apr 15, 2020 •

edited

Loading

izqui Apr 20, 2020

facuspagnuolo Apr 21, 2020

facuspagnuolo commented Apr 21, 2020

facuspagnuolo commented Apr 22, 2020

izqui left a comment

facuspagnuolo commented Apr 24, 2020 •

edited

Loading

sohkai left a comment

facuspagnuolo commented Jun 13, 2020

facuspagnuolo commented Jun 27, 2020

sohkai left a comment

sohkai Jun 29, 2020

facuspagnuolo Jun 29, 2020 •

edited

Loading

		@@ -31,7 +31,7 @@ contract('Agreement', ([_, user]) => {
		})

		context('newAction', () => {

Agreement: Implement protocol for disputable apps #1133

Agreement: Implement protocol for disputable apps #1133

Conversation

facuspagnuolo commented Apr 15, 2020 • edited Loading

Related PRs

Current version - Rinkeby

Old version - Rinkeby

izqui Apr 20, 2020

Choose a reason for hiding this comment

facuspagnuolo Apr 21, 2020

Choose a reason for hiding this comment

facuspagnuolo commented Apr 21, 2020

facuspagnuolo commented Apr 22, 2020

izqui left a comment

Choose a reason for hiding this comment

facuspagnuolo commented Apr 24, 2020 • edited Loading

sohkai left a comment

Choose a reason for hiding this comment

facuspagnuolo commented Jun 13, 2020

facuspagnuolo commented Jun 27, 2020

sohkai left a comment

Choose a reason for hiding this comment

sohkai Jun 29, 2020

Choose a reason for hiding this comment

facuspagnuolo Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

facuspagnuolo commented Apr 15, 2020 •

edited

Loading

facuspagnuolo commented Apr 24, 2020 •

edited

Loading

facuspagnuolo Jun 29, 2020 •

edited

Loading