Integrate external Staking app #1151

bingen · 2020-05-29T16:39:39Z

No description provided.

facuspagnuolo

Looking good! I left a couple of questions.

I think we can implement the receiveLockManager method in the Agreement app to allow users to sign it in a single tx, what do you think?

facuspagnuolo · 2020-06-01T13:58:08Z

apps/agreement/test/helpers/wrappers/disputable.js

+      await this.approveAndCall({ amount: stake, from: submitter })
+    } else {
+      stake = this.actionCollateral
+    }


Isn't the else case being caught right above? Do we need this?

Nope, sometimes stake is false, and I thought this was intended, like if it is false we explicitly don’t want to stake.

Sure but I mean, I don't see we are using the stake variable below

Yes, now we are (it was used before). Fixing it.

apps/agreement/test/helpers/wrappers/disputable.js

facuspagnuolo · 2020-06-01T14:18:17Z

apps/agreement/test/helpers/wrappers/agreement.js

+    const unlocked = await staking.unlockedBalanceOf(user)
+    const tokenBalance = await token.balanceOf(user)
+
+    return unlocked.add(tokenBalance)


I think it is a bit confusing mixing balances from different contracts. Based on the usages of this function, I think we are using it only to the challenger's balance.

I think this is because the challenger now is forced to use the staking pool. I'm not sure about this to be honest, what was the rationale?

It's important we fix this, we are mixing different balances here

facuspagnuolo · 2020-06-01T14:18:44Z

apps/agreement/test/agreement/agreement_staking.js

@@ -294,15 +294,15 @@ contract('Agreement', ([_, someone, user]) => {
        const amount = bigExp(200, 18)

        it('reverts', async () => {
-          await assertRevert(agreement.unstake({ token, user, amount }), STAKING_ERRORS.ERROR_NOT_ENOUGH_AVAILABLE_STAKE)
+          await assertRevert(agreement.unstake({ token, user, amount }), SAFE_MATH_ERRORS.ERROR_SUB_UNDERFLOW)


We can probably handle this scenario in the Staking contract, what do you think?

facuspagnuolo · 2020-06-01T14:19:31Z

apps/agreement/test/agreement/agreement_staking.js

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

      context('when the user has not approved the requested amount', () => {
        it('reverts', async () => {
-          await assertRevert(agreement.stake({ token, user, amount, from, approve }), STAKING_ERRORS.ERROR_TOKEN_DEPOSIT_FAILED)
+          await assertRevert(agreement.stake({ token, user, amount, from, approve }), STAKING_ERRORS.ERROR_TOKEN_TRANSFER_FAILED)


What do you think about distinguishing between token deposits and transfers for the error messages in the staking contract?

facuspagnuolo · 2020-06-01T14:23:31Z

apps/agreement/test/agreement/agreement_rule.js

                      assertBn(currentChallengerBalance, previousChallengerBalance.add(actionCollateral).add(challengeCollateral), 'challenger balance does not match')

                      const currentAgreementBalance = await collateralToken.balanceOf(agreement.address)
                      assertBn(currentAgreementBalance, previousAgreementBalance.sub(challengeCollateral), 'agreement balance does not match')

                      const currentStakingBalance = await collateralToken.balanceOf(stakingAddress)
-                      assertBn(currentStakingBalance, previousStakingBalance.sub(actionCollateral), 'staking balance does not match')
+                      assertBn(currentStakingBalance, previousStakingBalance, 'staking balance does not match')


To understand this a bit, are we changing the way this was working before right? I mean, the challenger must use the staking pool now if I understand correctly.

What do you think about transferring the rewards to the challenger directly?

For reference: aragon/aragon-apps#1151 (comment) aragon/aragon-apps#1151 (comment)

Also, fix token deposit errors

- action end date - cannot challenge error

facuspagnuolo · 2020-06-09T15:23:42Z

apps/agreement/test/helpers/wrappers/disputable.js

+      await this.approveAndCall({ amount: stake, from: submitter })
+    } else {
+      stake = this.actionCollateral
+    }


Sure but I mean, I don't see we are using the stake variable below

facuspagnuolo · 2020-06-09T15:28:31Z

apps/agreement/contracts/Agreement.sol

@@ -924,7 +922,7 @@ contract Agreement is IAgreement, AragonApp {
    */
    function _unlockAndSlashBalance(Staking _staking, address _user, uint256 _unlockAmount, address _challenger, uint256 _slashAmount) internal {
        if (_unlockAmount != 0 && _slashAmount != 0) {
-            _staking.unlockAndSlash(_user, _unlockAmount, _challenger, _slashAmount);
+            _staking.slashAndUnlock(_user, _challenger, _unlockAmount, _slashAmount);


I thought we agreed on using slashAndUnstake here for now.
The UI is not thought to have the challenger interacting with the staking for now, we will probably do it in the future tho.

facuspagnuolo · 2020-06-09T15:33:50Z

apps/agreement/test/helpers/wrappers/agreement.js

+    const unlocked = await staking.unlockedBalanceOf(user)
+    const tokenBalance = await token.balanceOf(user)
+
+    return unlocked.add(tokenBalance)


It's important we fix this, we are mixing different balances here

Address PR #1151 comments.

Fix dependencies (https://github.com/cgewecke/web3-issue-3544)

…king

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

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

bingen self-assigned this May 29, 2020

probot-autolabeler bot added the component: contract label May 29, 2020

auto-assign bot requested review from bpierre and Evalir May 29, 2020 16:39

bpierre removed request for bpierre and Evalir June 1, 2020 01:49

bingen requested a review from facuspagnuolo June 1, 2020 11:29

facuspagnuolo reviewed Jun 1, 2020

View reviewed changes

bingen pushed a commit to aragon/staking that referenced this pull request Jun 3, 2020

Add error messages

8d13f5a

For reference: aragon/aragon-apps#1151 (comment) aragon/aragon-apps#1151 (comment)

facuspagnuolo force-pushed the implement_agreements_app branch from b332423 to df48d5b Compare June 3, 2020 18:50

bingen pushed a commit to aragon/staking that referenced this pull request Jun 3, 2020

Add error messages

cbcc721

For reference: aragon/aragon-apps#1151 (comment) aragon/aragon-apps#1151 (comment)

bingen mentioned this pull request Jun 3, 2020

Add error messages aragon/staking#29

Merged

bingen pushed a commit to aragon/staking that referenced this pull request Jun 3, 2020

Add error messages (#29)

31031db

For reference: aragon/aragon-apps#1151 (comment) aragon/aragon-apps#1151 (comment)

bingen force-pushed the agreements_executors_staking branch 2 times, most recently from 6a5e751 to 1254aed Compare June 8, 2020 11:51

ßingen added 6 commits June 9, 2020 13:47

Integrate external Staking app

e5817ce

Staking integration: adapt to new names

1c6b5bb

test: fix wrapper names

8363d0f

Also, fix token deposit errors

Upgrade Staking dependency

7da434d

Fixes after moving moving responsibilities to disputable app

fb20e27

- action end date - cannot challenge error

Fixes after rebase on moving responsibilities to disputable app

c6c039c

bingen force-pushed the agreements_executors_staking branch from 1254aed to c6c039c Compare June 9, 2020 15:12

Update Staking dependency

f404308

facuspagnuolo reviewed Jun 9, 2020

View reviewed changes

ßingen and others added 4 commits June 9, 2020 18:10

Fixes after rebase on moving responsibilities to disputable app

c9a625d

Address PR #1151 comments.

Clean _unlockAndSlashBalance

c0a9416

Fix dependencies (https://github.com/cgewecke/web3-issue-3544)

fixup! Clean _unlockAndSlashBalance

fec7655

Merge branch 'implement_agreements_app' into agreements_executors_sta…

eab9d96

…king

facuspagnuolo merged commit b378a20 into implement_agreements_app Jun 12, 2020

facuspagnuolo deleted the agreements_executors_staking branch June 12, 2020 22:01

sohkai reviewed Jun 25, 2020

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate external Staking app #1151

Integrate external Staking app #1151

bingen commented May 29, 2020

facuspagnuolo left a comment

facuspagnuolo Jun 1, 2020

bingen Jun 1, 2020

facuspagnuolo Jun 9, 2020

bingen Jun 9, 2020

facuspagnuolo Jun 1, 2020

facuspagnuolo Jun 9, 2020

facuspagnuolo Jun 1, 2020

facuspagnuolo Jun 1, 2020

facuspagnuolo Jun 1, 2020

facuspagnuolo Jun 9, 2020

facuspagnuolo Jun 9, 2020

facuspagnuolo Jun 9, 2020

Integrate external Staking app #1151

Integrate external Staking app #1151

Conversation

bingen commented May 29, 2020

facuspagnuolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment