Merge pull request #6 from kleros/review/hbarcelos

Review/hbarcelos

Author: fnanni-0

README.md

@@ -19,4 +19,4 @@ MetaMask setup for Sokol:
 ## Unstake
 1. Unstake from the Sokol general court. 
 1. You can use `withdrawAndConvertToPNK()` [method](https://blockscout.com/poa/sokol/address/0xe64818D4e179926299596745BA7C20A3E4a87164/write-contract) to directly convert the Sokol Wrapped PNK into Kovan PNK. If you use this method, it is possible to set the receiver. On the Kovan chain, the tokens have to be claimed though. Go to the [token bridge](https://sokol-omnibridge.web.app/bridge), connect MetaMask with your Kovan account and claim your tokens.
-1. If you rather use the token bridge, call `withdraw()` [method](https://blockscout.com/poa/sokol/address/0xe64818D4e179926299596745BA7C20A3E4a87164/write-contract) instead. On the Kovan chain, the tokens have to be claimed though. Go to the [token bridge](https://sokol-omnibridge.web.app/bridge), connect MetaMask with your Kovan account and claim your tokens.
+1. If you rather use the token bridge, call `withdraw()` [method](https://blockscout.com/poa/sokol/address/0xe64818D4e179926299596745BA7C20A3E4a87164/write-contract) instead. Then go to the [token bridge](https://sokol-omnibridge.web.app/bridge) convert your tokens.

contracts/kleros/xKlerosLiquid.sol

@@ -1,7 +1,7 @@
 /**
  *  https://contributing.kleros.io/smart-contract-workflow
  *  @authors: [@fnanni-0]
- *  @reviewers: [@shalzz, @unknownunknown1, @MerlinEgalite]
+ *  @reviewers: [@shalzz*, @unknownunknown1*, @MerlinEgalite*, @hbarcelos]
  *  @auditors: []
  *  @bounties: []
  *  @deployments: []
@@ -13,7 +13,7 @@ pragma solidity ^0.4.24;
 import "openzeppelin-eth/contracts/zos-lib/Initializable.sol";
 import { TokenController } from "minimetoken/contracts/TokenController.sol";
 import { Arbitrator, Arbitrable } from "@kleros/kleros-interaction/contracts/standard/arbitration/Arbitrator.sol";
-import { WrappedPinakion as Pinakion } from "../tokens/WrappedPinakion.sol";
+import { WrappedPinakion } from "../tokens/WrappedPinakion.sol";
 import { IRandomAuRa } from "../interfaces/IRandomAuRa.sol";
 
 import { SortitionSumTreeFactory } from "@kleros/kleros/contracts/data-structures/SortitionSumTreeFactory.sol";
@@ -134,12 +134,13 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
     event Draw(address indexed _address, uint indexed _disputeID, uint _appeal, uint _voteID);
 
     /** @dev Emitted when a juror wins or loses tokens and xDai from a dispute.
+     *  The original name from KlerosLiquid is kept in this event to match its ABI.
      *  @param _address The juror affected.
      *  @param _disputeID The ID of the dispute.
      *  @param _tokenAmount The amount of tokens won or lost.
      *  @param _xDaiAmount The amount of xDai won or lost.
      */
-    event TokenAndXDaiShift(address indexed _address, uint indexed _disputeID, int _tokenAmount, int _xDaiAmount);
+    event TokenAndETHShift(address indexed _address, uint indexed _disputeID, int _tokenAmount, int _xDaiAmount);
 
     /* Storage */
 
@@ -150,7 +151,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
     uint public constant ALPHA_DIVISOR = 1e4; // The number to divide `Court.alpha` by.
     // General Contracts
     address public governor; // The governor of the contract.
-    Pinakion public pinakion; // The Pinakion token contract.
+    WrappedPinakion public pinakion; // The Pinakion token contract.
     IRandomAuRa public RNGenerator; // The random number generator contract.
     // General Dynamic
     Phase public phase; // The current phase.
@@ -214,10 +215,11 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
      *  @param _jurorsForCourtJump The `jurorsForCourtJump` property value of the general court.
      *  @param _timesPerPeriod The `timesPerPeriod` property value of the general court.
      *  @param _sortitionSumTreeK The number of children per node of the general court's sortition sum tree.
+     *  @param _maxTotalStakeAllowed The maximum amount of wrapped PNK that is allowed to be staked in this contract.
      */
     function initialize(
         address _governor,
-        Pinakion _pinakion,
+        WrappedPinakion _pinakion,
         IRandomAuRa _RNGenerator,
         uint _minStakingTime,
         uint _maxDrawingTime,
@@ -227,7 +229,8 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
         uint _feeForJuror,
         uint _jurorsForCourtJump,
         uint[4] _timesPerPeriod,
-        uint _sortitionSumTreeK
+        uint _sortitionSumTreeK,
+        uint _maxTotalStakeAllowed
     ) public initializer {
         // Initialize contract.
         governor = _governor;
@@ -238,7 +241,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
         lastPhaseChange = now;
         lockInsolventTransfers = true;
         nextDelayedSetStake = 1;
-        maxTotalStakeAllowed = 30 * 1e24; // 30 Million PNK.
+        maxTotalStakeAllowed = _maxTotalStakeAllowed;
 
         // Create the general court.
         courts.push(Court({
@@ -275,7 +278,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
     /** @dev Changes the `pinakion` storage variable.
      *  @param _pinakion The new value for the `pinakion` storage variable.
      */
-    function changePinakion(Pinakion _pinakion) external onlyByGovernor {
+    function changePinakion(WrappedPinakion _pinakion) external onlyByGovernor {
         pinakion = _pinakion;
     }
 
@@ -468,6 +471,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
     }
 
     /** @dev Executes the next delayed set stakes.
+     *  `O(n)` where `n` is the number of iterations to run.
      *  @param _iterations The number of delayed set stakes to execute.
      */
     function executeDelayedSetStakes(uint _iterations) external onlyDuringPhase(Phase.staking) {
@@ -580,12 +584,13 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
     }
 
     /** @dev Computes the token and xDai rewards for a specified appeal in a specified dispute.
+     *  The original name from KlerosLiquid is kept in this function to match its ABI.
      *  @param _disputeID The ID of the dispute.
      *  @param _appeal The appeal.
      *  @return tokenReward The token reward.
      *  @return xDaiReward The xDai reward.
      */
-    function computeTokenAndXDaiRewards(uint _disputeID, uint _appeal) private view returns(uint tokenReward, uint xDaiReward) {
+    function computeTokenAndETHRewards(uint _disputeID, uint _appeal) private view returns(uint tokenReward, uint xDaiReward) {
         Dispute storage dispute = disputes[_disputeID];
 
         // Distribute penalties and arbitration fees.
@@ -637,7 +642,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
             if (end > dispute.votes[_appeal].length) end = dispute.votes[_appeal].length;
         } else {
             // We loop over the votes twice, first to collect penalties, and second to distribute them as rewards along with arbitration fees.
-            (tokenReward, xDaiReward) = dispute.repartitionsInEachRound[_appeal] >= dispute.votes[_appeal].length ? computeTokenAndXDaiRewards(_disputeID, _appeal) : (0, 0); // Compute rewards if rewarding.
+            (tokenReward, xDaiReward) = dispute.repartitionsInEachRound[_appeal] >= dispute.votes[_appeal].length ? computeTokenAndETHRewards(_disputeID, _appeal) : (0, 0); // Compute rewards if rewarding.
             if (end > dispute.votes[_appeal].length * 2) end = dispute.votes[_appeal].length * 2;
         }
         for (uint i = dispute.repartitionsInEachRound[_appeal]; i < end; i++) {
@@ -652,7 +657,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
                     pinakion.transfer(vote.account, tokenReward);
                     // Intentional use to avoid blocking.
                     vote.account.send(xDaiReward); // solium-disable-line security/no-send
-                    emit TokenAndXDaiShift(vote.account, _disputeID, int(tokenReward), int(xDaiReward));
+                    emit TokenAndETHShift(vote.account, _disputeID, int(tokenReward), int(xDaiReward));
                     jurors[vote.account].lockedTokens -= dispute.tokensAtStakePerJuror[_appeal];
                 }
             } else { // Juror was inactive, or voted incoherently and it was not a tie.
@@ -661,7 +666,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
                     // Penalize.
                     uint penalty = dispute.tokensAtStakePerJuror[_appeal] > pinakion.balanceOf(vote.account) ? pinakion.balanceOf(vote.account) : dispute.tokensAtStakePerJuror[_appeal];
                     pinakion.transferFrom(vote.account, this, penalty);
-                    emit TokenAndXDaiShift(vote.account, _disputeID, -int(penalty), 0);
+                    emit TokenAndETHShift(vote.account, _disputeID, -int(penalty), 0);
                     penaltiesInRoundCache += penalty;
                     jurors[vote.account].lockedTokens -= dispute.tokensAtStakePerJuror[_appeal];
 
@@ -681,7 +686,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
                 } else if (i + 1 < end) {
                     // Compute rewards because we are going into rewarding.
                     dispute.penaltiesInEachRound[_appeal] = penaltiesInRoundCache;
-                    (tokenReward, xDaiReward) = computeTokenAndXDaiRewards(_disputeID, _appeal);
+                    (tokenReward, xDaiReward) = computeTokenAndETHRewards(_disputeID, _appeal);
                 }
             }
         }
@@ -767,7 +772,7 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
         emit NewPeriod(_disputeID, Period.evidence);
     }
 
-    /** @dev Called when `_owner` sends xDai to the MiniMe Token contract.
+    /** @dev DEPRECATED. Called when `_owner` sends xDai to the Wrapped Token contract.
      *  @param _owner The address that sent the xDai to create tokens.
      *  @return allowed Whether the operation should be allowed or not.
      */
@@ -896,7 +901,8 @@ contract xKlerosLiquid is Initializable, TokenController, Arbitrator {
 
         /** If maxTotalStakeAllowed was reduced through governance, 
          *  totalStake could be greater than maxTotalStakeAllowed. 
-         *  In such case allow unstaking. Always allow unstaking. 
+         *  If this happens, removing stake is still allowed even if
+         *  totalStake is greater than maxTotalStakeAllowed afterwards.
          */ 
         if ((totalStake - juror.stakedTokens + newTotalStake > maxTotalStakeAllowed) && (newTotalStake > juror.stakedTokens))
             return false; // Maximum PNK stake reached.

contracts/tokens/WrappedPinakion.sol

@@ -1,7 +1,7 @@
 /**
  *  https://contributing.kleros.io/smart-contract-workflow
  *  @authors: [@fnanni-0]
- *  @reviewers: [@unknownunknown1, @MerlinEgalite]
+ *  @reviewers: [@unknownunknown1, @MerlinEgalite, @hbarcelos]
  *  @auditors: []
  *  @bounties: []
  *  @deployments: []

migrations/2_deploy_contracts.js

@@ -38,7 +38,8 @@ let xKlerosLiquidParams = {
     feeForJuror: web3.utils.toWei('40000000000000000','wei'),
     jurorsForCourtJump: 511,
     timesPerPeriod: [280800, 583200, 583200, 388800],
-    sortitionSumTreeK: 2
+    sortitionSumTreeK: 2,
+    maxTotalStakeAllowed: web3.utils.toWei('30000000','ether')
   },
   "sokol-fork": {
     governor: null,
@@ -52,7 +53,8 @@ let xKlerosLiquidParams = {
     feeForJuror: web3.utils.toWei('40000000000000000','wei'),
     jurorsForCourtJump: 511,
     timesPerPeriod: [280800, 583200, 583200, 388800],
-    sortitionSumTreeK: 2
+    sortitionSumTreeK: 2,
+    maxTotalStakeAllowed: web3.utils.toWei('30000000','ether')
   },
   "sokol": {
     governor: null,
@@ -66,7 +68,8 @@ let xKlerosLiquidParams = {
     feeForJuror: web3.utils.toWei('40000000000000000','wei'),
     jurorsForCourtJump: 511,
     timesPerPeriod: [280800, 583200, 583200, 388800],
-    sortitionSumTreeK: 2
+    sortitionSumTreeK: 2,
+    maxTotalStakeAllowed: web3.utils.toWei('30000000','ether')
   }
 }
 
@@ -106,6 +109,7 @@ module.exports = async function(deployer, network) {
       xKlerosLiquidParams[network].jurorsForCourtJump,
       xKlerosLiquidParams[network].timesPerPeriod,
       xKlerosLiquidParams[network].sortitionSumTreeK,
+      xKlerosLiquidParams[network].maxTotalStakeAllowed,
     ], 
     { deployer, unsafeAllowLinkedLibraries: true }
   );

test/kleros/kleros-liquid.js

@@ -101,6 +101,7 @@ contract('xKlerosLiquid', (accounts) => {
   let maxDrawingTime
   let subcourtTree
   let subcourtMap
+  let maxTotalStakeAllowed
   let klerosLiquid
   beforeEach(async () => {
     governor = accounts[0]
@@ -126,6 +127,7 @@ contract('xKlerosLiquid', (accounts) => {
     } = generateSubcourts(randomInt(4, 2), 3)
     subcourtTree = _subcourtTree
     subcourtMap = _subcourtMap
+    maxTotalStakeAllowed =  web3.utils.toWei('30000000', 'ether') // 30M PNK
 
     const sortitionSumTreeFactoryLib = await SortitionSumTreeFactory.new()
     await xKlerosLiquid.link(
@@ -147,6 +149,7 @@ contract('xKlerosLiquid', (accounts) => {
         subcourtTree.jurorsForJump,
         subcourtTree.timesPerPeriod,
         subcourtTree.sortitionSumTreeK,
+        maxTotalStakeAllowed,
       ],
       { unsafeAllowLinkedLibraries: true }
     )
@@ -534,7 +537,7 @@ contract('xKlerosLiquid', (accounts) => {
               .mod(coherentVotes)
             expect(PNKBefore.eq(PNKAfter.add(burntPNK))).to.equal(true)
             const tokensShiftEvents = await klerosLiquid.getPastEvents(
-              'TokenAndXDaiShift',
+              'TokenAndETHShift',
               {
                 filter: { _disputeID: dispute.ID },
                 fromBlock: executeBlockNumber,