Court: split staking: remove activation and withdraw entry points...

...from main contract, and put them in CourtStaking.
aragon · Jul 26, 2019 · 2ff74de · 2ff74de
1 parent 3f2d84a
commit 2ff74de
Show file tree

Hide file tree

Showing 10 changed files with 68 additions and 80 deletions.
diff --git a/contracts/Court.sol b/contracts/Court.sol
@@ -164,13 +164,7 @@ contract Court is IStakingOwner, ICRVotingOwner, ISubscriptionsOwner {
     }
 
     modifier ensureTerm {
-        uint64 requiredTransitions = neededTermTransitions();
-        require(requiredTransitions <= MODIFIER_ALLOWED_TERM_TRANSITIONS, ERROR_TOO_MANY_TRANSITIONS);
-
-        if (requiredTransitions > 0) {
-            heartbeat(requiredTransitions);
-        }
-
+        _ensureTerm();
         _;
     }
 
@@ -281,27 +275,6 @@ contract Court is IStakingOwner, ICRVotingOwner, ISubscriptionsOwner {
         }
     }
 
-    /**
-     * @notice Become an active juror on next term
-     */
-    function activate() external ensureTerm {
-        staking.activate(msg.sender, termId);
-    }
-
-    /**
-     * @notice Stop being an active juror on next term
-     */
-    function deactivate() external ensureTerm {
-        staking.deactivate(msg.sender, termId);
-    }
-
-    /**
-     * @notice Withdraw `@tokenAmount(_token, _amount)` from the Court (Staking)
-     */
-    function withdraw(ERC20 _token, uint256 _amount) external ensureTerm {
-        staking.withdraw(msg.sender, _token, _amount, termId);
-    }
-
     /**
      * @notice Create a dispute over `_subject` with `_possibleRulings` possible rulings, drafting `_jurorNumber` jurors in term `_draftTermId`
      */
@@ -604,11 +577,20 @@ contract Court is IStakingOwner, ICRVotingOwner, ISubscriptionsOwner {
         return (_time() - terms[termId].startTime) / termDuration;
     }
 
-    function getEnsuredTermId() external view returns (uint64) {
-        require(neededTermTransitions() == 0, ERROR_WRONG_TERM);
+    function ensureAndGetTerm() external returns (uint64) {
+        _ensureTerm();
         return termId;
     }
 
+    function _ensureTerm() internal {
+        uint64 requiredTransitions = neededTermTransitions();
+        require(requiredTransitions <= MODIFIER_ALLOWED_TERM_TRANSITIONS, ERROR_TOO_MANY_TRANSITIONS);
+
+        if (requiredTransitions > 0) {
+            heartbeat(requiredTransitions);
+        }
+    }
+
     /**
      * @dev This function only works for regular rounds. For final round `filledSeats` is always zero,
      *      so the result will always be false. There is no drafting in final round.

diff --git a/contracts/CourtStaking.sol b/contracts/CourtStaking.sol
@@ -28,6 +28,7 @@ contract CourtStaking is IsContract, ERC900, ApproveAndCallFallBack, IStaking {
     string internal constant ERROR_BALANCE_TOO_LOW = "STK_BALANCE_TOO_LOW";
     string internal constant ERROR_TOKENS_BELOW_MIN_STAKE = "STK_TOKENS_BELOW_MIN_STAKE";
     string internal constant ERROR_JUROR_TOKENS_AT_STAKE = "STK_JUROR_TOKENS_AT_STAKE";
+    string internal constant ERROR_WRONG_TOKEN = "STK_WRONG_TOKEN";
     string internal constant ERROR_TOKEN_TRANSFER_FAILED = "STK_TOKEN_TRANSFER_FAILED";
     string internal constant ERROR_SORTITION_LENGTHS_MISMATCH = "STK_SORTITION_LENGTHS_MISMATCH";
 
@@ -84,49 +85,53 @@ contract CourtStaking is IsContract, ERC900, ApproveAndCallFallBack, IStaking {
     /**
      * @notice Become an active juror on next term
      */
-    function activate(address _juror, uint64 _termId) external only(owner) {
-        Account storage account = accounts[_juror];
+    function activate() external {
+        uint64 termId = owner.ensureAndGetTerm();
+
+        Account storage account = accounts[msg.sender];
         uint256 balance = account.balances[jurorToken];
 
-        require(account.deactivationTermId <= _termId, ERROR_INVALID_ACCOUNT_STATE);
+        require(account.deactivationTermId <= termId, ERROR_INVALID_ACCOUNT_STATE);
         require(balance >= jurorMinStake, ERROR_TOKENS_BELOW_MIN_STAKE);
 
         uint256 sumTreeId = account.sumTreeId;
         if (sumTreeId == 0) {
-            sumTreeId = sumTree.insert(_termId, 0); // Always > 0 (as constructor inserts the first item)
+            sumTreeId = sumTree.insert(termId, 0); // Always > 0 (as constructor inserts the first item)
             account.sumTreeId = sumTreeId;
-            jurorsByTreeId[sumTreeId] = _juror;
+            jurorsByTreeId[sumTreeId] = msg.sender;
         }
 
-        uint64 fromTermId = _termId + 1;
+        uint64 fromTermId = termId + 1;
         sumTree.update(sumTreeId, fromTermId, balance, true);
 
         account.deactivationTermId = MAX_UINT64;
         account.balances[jurorToken] = 0; // tokens are in the tree (present or future)
 
-        emit JurorActivated(_juror, fromTermId);
+        emit JurorActivated(msg.sender, fromTermId);
     }
 
     // TODO: Activate more tokens as a juror
 
     /**
      * @notice Stop being an active juror on next term
      */
-    function deactivate(address _juror, uint64 _termId) external only(owner) {
-        Account storage account = accounts[_juror];
+    function deactivate() external {
+        uint64 termId = owner.ensureAndGetTerm();
+
+        Account storage account = accounts[msg.sender];
 
         require(account.deactivationTermId == MAX_UINT64, ERROR_INVALID_ACCOUNT_STATE);
 
         // Always account.sumTreeId > 0, as juror has activated before
         uint256 treeBalance = sumTree.getItem(account.sumTreeId);
         account.balances[jurorToken] += treeBalance;
 
-        uint64 lastTermId = _termId + 1;
+        uint64 lastTermId = termId + 1;
         account.deactivationTermId = lastTermId;
 
         sumTree.set(account.sumTreeId, lastTermId, 0);
 
-        emit JurorDeactivated(_juror, lastTermId);
+        emit JurorDeactivated(msg.sender, lastTermId);
     }
 
     /**
@@ -249,15 +254,24 @@ contract CourtStaking is IsContract, ERC900, ApproveAndCallFallBack, IStaking {
      * @dev This is done this way to conform to ERC900 interface
      */
     function unstake(uint256 _amount, bytes) external {
-        uint64 termId = owner.getEnsuredTermId();
-        return _withdraw(msg.sender, jurorToken, _amount, termId); // withdraw() ensures the correct term
+        uint64 termId = owner.ensureAndGetTerm();
+
+        // Make sure deactivation has finished before withdrawing
+        require(accounts[msg.sender].deactivationTermId <= termId, ERROR_INVALID_ACCOUNT_STATE);
+        require(_amount <= unlockedBalanceOf(msg.sender), ERROR_JUROR_TOKENS_AT_STAKE);
+
+        _withdraw(msg.sender, jurorToken, _amount);
+
+        emit Unstaked(msg.sender, _amount, totalStakedFor(msg.sender), "");
     }
 
     /**
      * @notice Withdraw `@tokenAmount(_token, _amount)` from the Court
+     * @dev Not for `jurorToken` (use unstake for that one instead)
      */
-    function withdraw(address _from, ERC20 _token, uint256 _amount, uint64 _termId) external only(owner) {
-        _withdraw(_from, _token, _amount, _termId);
+    function withdraw(ERC20 _token, uint256 _amount) external {
+        require(_token != jurorToken, ERROR_WRONG_TOKEN);
+        _withdraw(msg.sender, _token, _amount);
     }
 
     function assignTokens(ERC20 _token, address _to, uint256 _amount) external only(owner) {
@@ -376,21 +390,13 @@ contract CourtStaking is IsContract, ERC900, ApproveAndCallFallBack, IStaking {
         emit Staked(_to, _amount, totalStakedFor(_to), "");
     }
 
-    function _withdraw(address _from, ERC20 _token, uint256 _amount, uint64 _termId) internal {
+    function _withdraw(address _from, ERC20 _token, uint256 _amount) internal {
         require(_amount > 0, ERROR_ZERO_TRANSFER);
 
         Account storage account = accounts[_from];
         uint256 balance = account.balances[_token];
         require(balance >= _amount, ERROR_BALANCE_TOO_LOW);
 
-        if (_token == jurorToken) {
-            // Make sure deactivation has finished before withdrawing
-            require(account.deactivationTermId <= _termId, ERROR_INVALID_ACCOUNT_STATE);
-            require(_amount <= unlockedBalanceOf(_from), ERROR_JUROR_TOKENS_AT_STAKE);
-
-            emit Unstaked(_from, _amount, totalStakedFor(_from), "");
-        }
-
         _removeTokens(_token, _from, _amount);
         require(_token.safeTransfer(_from, _amount), ERROR_TOKEN_TRANSFER_FAILED);
 

diff --git a/contracts/standards/erc900/IStaking.sol b/contracts/standards/erc900/IStaking.sol
@@ -9,11 +9,11 @@ import "./IStakingOwner.sol";
 interface IStaking {
     function init(IStakingOwner _owner, ISumTree _sumTree, ERC20 _jurorToken, uint256 _jurorMinStake) external;
     function collectTokens(uint64 _termId, address _juror, uint256 _amount) external returns (bool);
-    function activate(address _juror, uint64 _termId) external;
-    function deactivate(address _juror, uint64 _termId) external;
+    function activate() external;
+    function deactivate() external;
     function draft(uint256[7] _draftParams) external returns (address[] jurors, uint64[] weights, uint256 jurorsLength, uint64 filledSeats);
     function slash(uint64 _termId, address[] _jurors, uint256[] _penalties, uint8[] _castVotes, uint8 _winningRuling) external returns (uint256 collectedTokens);
-    function withdraw(address _from, ERC20 _token, uint256 _amount, uint64 _termId) external;
+    function withdraw(ERC20 _token, uint256 _amount) external;
     function assignTokens(ERC20 _token, address _to, uint256 _amount) external;
     function assignJurorTokens(address _to, uint256 _amount) external;
     function removeTokens(ERC20 _token, address _from, uint256 _amount) external;

diff --git a/contracts/standards/erc900/IStakingOwner.sol b/contracts/standards/erc900/IStakingOwner.sol
@@ -2,5 +2,5 @@ pragma solidity ^0.4.24;
 
 
 interface IStakingOwner {
-    function getEnsuredTermId() external view returns (uint64);
+    function ensureAndGetTerm() external returns (uint64);
 }
diff --git a/test/court-batches.js b/test/court-batches.js
@@ -134,7 +134,7 @@ contract('Court: Batches', ([ rich, governor, arbitrable, juror1, juror2, juror3
 
     const createDispute = async () => {
       for (const juror of [ juror1, juror2, juror3, juror4, juror5, juror6, juror7 ]) {
-        await this.court.activate({ from: juror })
+        await this.staking.activate({ from: juror })
       }
       await passTerms(1) // term = 1
 
@@ -289,7 +289,7 @@ contract('Court: Batches', ([ rich, governor, arbitrable, juror1, juror2, juror3
 
     const createDispute = async () => {
       for (const juror of [juror1, juror2, juror3, juror4, juror5, juror6, juror7]) {
-        await this.court.activate({ from: juror })
+        await this.staking.activate({ from: juror })
       }
       await passTerms(1) // term = 1
 

diff --git a/test/court-disputes.js b/test/court-disputes.js
@@ -165,7 +165,7 @@ contract('Court: Disputes', ([ poor, rich, governor, juror1, juror2, juror3, oth
 
     beforeEach(async () => {
       for (const juror of [juror1, juror2, juror3]) {
-        await this.court.activate({ from: juror })
+        await this.staking.activate({ from: juror })
       }
       await passTerms(1) // term = 1
     })

diff --git a/test/court-final-appeal-non-exact.js b/test/court-final-appeal-non-exact.js
@@ -162,7 +162,7 @@ contract('Court: final appeal (non-exact)', ([ poor, rich, governor, juror1, jur
 
     beforeEach(async () => {
       for (const juror of jurors) {
-        await this.court.activate({ from: juror })
+        await this.staking.activate({ from: juror })
       }
       await passTerms(1) // term = 1
 

diff --git a/test/court-final-appeal.js b/test/court-final-appeal.js
@@ -158,7 +158,7 @@ contract('Court: final appeal', ([ poor, rich, governor, juror1, juror2, juror3,
 
     beforeEach(async () => {
       for (const juror of jurors) {
-        await this.court.activate({ from: juror })
+        await this.staking.activate({ from: juror })
       }
       await passTerms(1) // term = 1
 

diff --git a/test/court-lifecycle.js b/test/court-lifecycle.js
@@ -145,15 +145,15 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
     it('can activate during period before heartbeat', async () => {
       await this.staking.mock_setTime(firstTermStart - 1)
       await this.court.mock_setTime(firstTermStart - 1)
-      await this.court.activate({ from: rich })
+      await this.staking.activate({ from: rich })
 
       await assertEqualBN(this.staking.mock_treeTotalSum(), richStake, 'total tree sum')
     })
 
     it('reverts if activating balance is below dust', async () => {
       await this.staking.mock_setTime(firstTermStart - 1)
       await this.court.mock_setTime(firstTermStart - 1)
-      await assertRevert(this.court.activate({ from: poor }), 'STK_TOKENS_BELOW_MIN_STAKE')
+      await assertRevert(this.staking.activate({ from: poor }), 'STK_TOKENS_BELOW_MIN_STAKE')
     })
 
     it("doesn't perform more transitions than requested", async () => {
@@ -199,8 +199,8 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
     })
 
     it('jurors can activate', async () => {
-      await this.court.activate({ from: juror1 })
-      await this.court.activate({ from: juror2 })
+      await this.staking.activate({ from: juror1 })
+      await this.staking.activate({ from: juror2 })
 
       await passTerms(1)
 
@@ -216,14 +216,14 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
     })
 
     const activateDeactivate = async () => {
-      await this.court.activate({ from: juror1 })
-      await this.court.activate({ from: juror2 })
+      await this.staking.activate({ from: juror1 })
+      await this.staking.activate({ from: juror2 })
       await passTerms(1)
       await assertEqualBN(this.staking.mock_treeTotalSum(), juror1Stake + juror2Stake, 'both jurors in the tree')
-      await this.court.deactivate({ from: juror1 })
+      await this.staking.deactivate({ from: juror1 })
       await passTerms(1)
       await assertEqualBN(this.staking.mock_treeTotalSum(), juror2Stake, 'only juror2 in tree')
-      await this.court.deactivate({ from: juror2 })
+      await this.staking.deactivate({ from: juror2 })
       await passTerms(1)
       await assertEqualBN(this.staking.mock_treeTotalSum(), 0, 'no jurors in tree')
     }
@@ -240,21 +240,21 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
     })
 
     it('fails trying to activate twice', async () => {
-      await this.court.activate({ from: juror1 })
+      await this.staking.activate({ from: juror1 })
       await passTerms(1)
       await assertEqualBN(this.staking.mock_treeTotalSum(), juror1Stake, 'juror is in the tree')
       await passTerms(1)
-      await assertRevert(this.court.activate({ from: juror1 }), 'STK_INVALID_ACCOUNT_STATE')
+      await assertRevert(this.staking.activate({ from: juror1 }), 'STK_INVALID_ACCOUNT_STATE')
     })
 
     it('fails trying to deactivate twice', async () => {
       await activateDeactivate()
-      await assertRevert(this.court.deactivate({ from: juror1 }), 'STK_INVALID_ACCOUNT_STATE')
+      await assertRevert(this.staking.deactivate({ from: juror1 }), 'STK_INVALID_ACCOUNT_STATE')
     })
 
     // TODO: refactor to use at stake tokens
     it.skip('juror can withdraw after cooldown', async () => {
-      await this.court.activate({ from: juror1 })
+      await this.staking.activate({ from: juror1 })
       await passTerms(1)
       await assertEqualBN(this.staking.mock_treeTotalSum(), juror1Stake, 'juror added to tree')
       await passTerms(1)

diff --git a/test/court-staking.js b/test/court-staking.js
@@ -21,6 +21,7 @@ const assertEqualBN = async (actualPromise, expected, message) =>
   assert.equal((await actualPromise).toNumber(), expected, message)
 
 const ERROR_INVALID_ACCOUNT_STATE = 'STK_INVALID_ACCOUNT_STATE'
+const ERROR_WRONG_TOKEN = 'STK_WRONG_TOKEN'
 
 contract('Court: Staking', ([ pleb, rich, governor ]) => {
   const INITIAL_BALANCE = 1e6
@@ -121,12 +122,11 @@ contract('Court: Staking', ([ pleb, rich, governor ]) => {
       await assertStaked(rich, -unstaking, INITIAL_BALANCE - amount, { initialStaked: amount })
     })
 
-    it('unstakes using \'withdraw\'', async () => {
+    it('fails unstaking using \'withdraw\'', async () => {
       const unstaking = amount / 4
 
-      await this.court.withdraw(this.anj.address, unstaking, { from: rich })
+      await assertRevert(this.staking.withdraw(this.anj.address, unstaking, { from: rich }), ERROR_WRONG_TOKEN)
 
-      await assertStaked(rich, -unstaking, INITIAL_BALANCE - amount, { initialStaked: amount })
     })
 
     context('Being activated', () => {
@@ -138,7 +138,7 @@ contract('Court: Staking', ([ pleb, rich, governor ]) => {
       }
 
       beforeEach(async () => {
-        await this.court.activate({ from: rich })
+        await this.staking.activate({ from: rich })
         await passTerms(1)
       })
 
@@ -149,7 +149,7 @@ contract('Court: Staking', ([ pleb, rich, governor ]) => {
         const unstaking = amount / 3
         await assertRevert(this.staking.unstake(unstaking, NO_DATA, { from: rich }), ERROR_INVALID_ACCOUNT_STATE)
         // deactivate
-        await this.court.deactivate({ from: rich })
+        await this.staking.deactivate({ from: rich })
         // still unable to withdraw, must pass to next term
         await assertRevert(this.staking.unstake(unstaking, NO_DATA, { from: rich }), ERROR_INVALID_ACCOUNT_STATE)
         await passTerms(1)