court: fix terms initialization

aragon · Aug 25, 2019 · da99eeb · da99eeb
1 parent 6b36ce4
commit da99eeb
Show file tree

Hide file tree

Showing 9 changed files with 254 additions and 222 deletions.
diff --git a/contracts/Court.sol b/contracts/Court.sol
@@ -233,8 +233,7 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim
         uint256[5] _subscriptionParams // _periodDuration, _feeAmount, _prePaymentPeriods, _latePaymentPenaltyPct, _governorSharePct
     ) public {
         require(_firstTermStartTime >= _termDuration, ERROR_BAD_FIRST_TERM_START_TIME);
-        // TODO: we should add this validation, cannot enable it now due to how tests are mocking timestamps
-        // require(_firstTermStartTime - _termDuration <= getTimestamp64(), ERROR_BAD_FIRST_TERM_START_TIME);
+        require(_firstTermStartTime >= getTimestamp64(), ERROR_BAD_FIRST_TERM_START_TIME);
 
         termDuration = _termDuration;
         jurorsRegistry = _jurorsRegistry;
@@ -668,8 +667,9 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim
     }
 
     /**
-     * @notice Send a heartbeat to the Court to transition up to `_termTransitions`
-     */
+    * @notice Send a heartbeat to the Court to transition up to `_maxRequestedTransitions` terms
+    * @param _maxRequestedTransitions Max number of term transitions allowed by the sender
+    */
     function heartbeat(uint64 _maxRequestedTransitions) public {
         uint64 neededTransitions = neededTermTransitions();
         uint256 transitions = uint256(_maxRequestedTransitions < neededTransitions ? _maxRequestedTransitions : neededTransitions);
@@ -679,21 +679,21 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim
         uint256 totalFee;
         for (uint256 transition = 1; transition <= transitions; transition++) {
             Term storage previousTerm = terms[termId++];
-            Term storage nextTerm = terms[termId];
+            Term storage currentTerm = terms[termId];
 
             // TODO: allow config to be changed for a future term id
-            nextTerm.courtConfigId = previousTerm.courtConfigId;
+            currentTerm.courtConfigId = previousTerm.courtConfigId;
             // Set the start time of the new term. Note that we are using a constant term duration value to guarantee
             // equally long terms, regardless of heartbeats.
-            nextTerm.startTime = previousTerm.startTime + termDuration;
+            currentTerm.startTime = previousTerm.startTime + termDuration;
             // In order to draft a random number of jurors in a term, we use a randomness factor for each term based on a
             // block number that is set once the term has started. Note that this information could not be known beforehand.
-            nextTerm.randomnessBN = getBlockNumber64() + 1;
+            currentTerm.randomnessBN = getBlockNumber64() + 1;
             emit NewTerm(termId, msg.sender);
 
             // Add amount of fees to be paid for the transitioned term
-            CourtConfig storage config = courtConfigs[nextTerm.courtConfigId];
-            totalFee = totalFee.add(config.heartbeatFee.mul(uint256(nextTerm.dependingDrafts)));
+            CourtConfig storage config = courtConfigs[currentTerm.courtConfigId];
+            totalFee = totalFee.add(config.heartbeatFee.mul(uint256(currentTerm.dependingDrafts)));
         }
 
         // Pay heartbeat fees to the caller of this function
@@ -702,6 +702,22 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim
         }
     }
 
+    /**
+    * @dev Tells the number of terms the Court should transition to be up-to-date
+    * @return Number of terms the Court should transition to be up-to-date
+    */
+    function neededTermTransitions() public view returns (uint64) {
+        // Note that the Court is always initialized at least for the current initialization time or more likely a
+        // in the future. If that the case, no term transitions are needed.
+        uint64 currentTermStartTime = terms[termId].startTime;
+        if (getTimestamp64() < currentTermStartTime) {
+            return uint64(0);
+        }
+
+        // We already know that the start time of the current term is in the past, we are safe to avoid SafeMath here
+        return (getTimestamp64() - currentTermStartTime) / termDuration;
+    }
+
     /**
      * @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.
@@ -711,10 +727,6 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim
         return round.filledSeats == round.jurorNumber;
     }
 
-    function neededTermTransitions() public view returns (uint64) {
-        return (getTimestamp64() - terms[termId].startTime) / termDuration;
-    }
-
     function getNextAppealDetails(uint256 _disputeId, uint256 _roundId) public view
         returns (
             uint64 appealDraftTermId,
@@ -749,6 +761,7 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim
     function _createRound(uint256 _disputeId, DisputeState _disputeState, uint64 _draftTermId, uint64 _jurorNumber, uint256 _jurorFees) internal
         returns (uint256 roundId)
     {
+        // TODO: ensure we cannot create rounds for term zero
         Dispute storage dispute = disputes[_disputeId];
         dispute.state = _disputeState;
 

diff --git a/contracts/test/lib/TimeHelpersMock.sol b/contracts/test/lib/TimeHelpersMock.sol
@@ -9,7 +9,6 @@ contract TimeHelpersMock is TimeHelpers {
     using SafeMath for uint256;
     using SafeMath64 for uint64;
 
-    // TODO: Current mocks need to start from timestamp 0 and blocknumber 1 due to how tests are built, fix tests to be able to start with current values
     uint256 mockedTimestamp;
     uint256 mockedBlockNumber;
 

diff --git a/test/court-batches.js b/test/court-batches.js
@@ -33,9 +33,10 @@ const assertLogs = async (receiptPromise, ...logNames) => {
   }
 }
 
+const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'
+
 contract('Court: Batches', ([ rich, arbitrable, juror1, juror2, juror3, juror4, juror5, juror6, juror7 ]) => {
   const NO_DATA = ''
-  const ZERO_ADDRESS = '0x' + '00'.repeat(20)
   let MAX_JURORS_PER_DRAFT_BATCH
 
   const termDuration = ONE_DAY
@@ -243,6 +244,7 @@ contract('Court: Batches', ([ rich, arbitrable, juror1, juror2, juror3, juror4,
         // make sure we miss randomness
         await this.courtHelper.advanceBlocks(257)
         await assertRevert(this.court.draft(disputeId), ERROR_TERM_RANDOMNESS_UNAVAIL)
+
         // move forward to next term
         await passTerms(1)
         // advance two blocks to ensure we can compute term randomness

diff --git a/test/court-disputes.js b/test/court-disputes.js
@@ -1,7 +1,7 @@
 const { ONE_DAY } = require('./helpers/time')
 const { buildHelper } = require('./helpers/court')(web3, artifacts)
-const { SALT, encryptVote } = require('./helpers/crvoting')
 const { assertRevert } = require('@aragon/os/test/helpers/assertThrow')
+const { SALT, encryptVote } = require('./helpers/crvoting')
 const { decodeEventsOfType } = require('./helpers/decodeEvent')
 
 const TokenFactory = artifacts.require('TokenFactory')

diff --git a/test/court-final-appeal.js b/test/court-final-appeal.js
@@ -12,8 +12,6 @@ const CRVoting = artifacts.require('CRVoting')
 const Subscriptions = artifacts.require('SubscriptionsMock')
 
 const MINIME = 'MiniMeToken'
-const NO_DATA = ''
-const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'
 
 const getLog = (receipt, logName, argName) => {
   const log = receipt.logs.find(({ event }) => event == logName)
@@ -40,6 +38,9 @@ const getVoteId = (disputeId, roundId) => {
   return new web3.BigNumber(2).pow(128).mul(disputeId).add(roundId)
 }
 
+const NO_DATA = ''
+const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'
+
 contract('Court: final appeal', ([ poor, rich, juror1, juror2, juror3, juror4, juror5, juror6, juror7 ]) => {
   const jurors = [ juror1, juror2, juror3, juror4, juror5, juror6, juror7 ]
   const SETTLE_BATCH_SIZE = 40

diff --git a/test/court-init.js b/test/court-init.js
diff --git a/test/court-lifecycle.js b/test/court-lifecycle.js
@@ -1,7 +1,8 @@
+const { buildHelper } = require('./helpers/court')(web3, artifacts)
 const { assertRevert } = require('@aragon/os/test/helpers/assertThrow')
+const { ONE_DAY, TOMORROW } = require('./helpers/time')
 
 const TokenFactory = artifacts.require('TokenFactory')
-const CourtMock = artifacts.require('CourtMock')
 const CourtAccounting = artifacts.require('CourtAccounting')
 const JurorsRegistry = artifacts.require('JurorsRegistryMock')
 const CRVoting = artifacts.require('CRVoting')
@@ -21,34 +22,19 @@ const deployedContract = async (receiptPromise, name) =>
 const assertEqualBN = async (actualPromise, expected, message) =>
   assert.equal((await actualPromise).toNumber(), expected, message)
 
-const assertLogs = async (receiptPromise, ...logNames) => {
-  const receipt = await receiptPromise
-  for (const logName of logNames) {
-    assert.isNotNull(getLog(receipt, logName), `Expected ${logName} in receipt`)
-  }
-}
-
-contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
-  const NO_DATA = ''
-  const ZERO_ADDRESS = '0x' + '00'.repeat(20)
+const NO_DATA = ''
+const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'
 
-  const termDuration = 10
-  const firstTermStart = 15
-  const jurorMinStake = 100
+contract('Court: Lifecycle', ([ poor, rich, juror1, juror2 ]) => {
   const cooldown = 10
-  const commitTerms = 1
-  const revealTerms = 1
-  const appealTerms = 1
-  const appealConfirmTerms = 1
-  const penaltyPct = 100 // 100‱ = 1%
-  const finalRoundReduction = 3300 // 100‱ = 1%
-
+
+  const termDuration = ONE_DAY
+  const firstTermStartTime = TOMORROW
   const initialBalance = 1e6
   const richStake = 1000
   const juror1Stake = 700
   const juror2Stake = 300
 
-  const NEW_TERM_EVENT = 'NewTerm'
   const ERROR_JUROR_TOKENS_AT_STAKE = 'STK_JUROR_TOKENS_AT_STAKE'
 
   before(async () => {
@@ -67,27 +53,17 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
     this.subscriptions = await Subscriptions.new()
     await this.subscriptions.setUpToDate(true)
 
-    this.court = await CourtMock.new(
+    this.courtHelper = buildHelper()
+    this.court = await this.courtHelper.deploy({
       termDuration,
-      [ this.anj.address, ZERO_ADDRESS ], // no fees
-      this.jurorsRegistry.address,
-      this.accounting.address,
-      this.voting.address,
-      this.subscriptions.address,
-      [ 0, 0, 0, 0 ],
-      governor,
-      firstTermStart,
-      jurorMinStake,
-      [ commitTerms, revealTerms, appealTerms, appealConfirmTerms ],
-      [ penaltyPct, finalRoundReduction ],
-      3,
-      4,
-      [ 0, 0, 0, 0, 0 ]
-    )
-
-    // TODO: use more realistic term duration and first term start time values
-    await this.court.mockSetTimestamp(1)
-    await this.jurorsRegistry.mockSetTimestamp(1)
+      firstTermStartTime,
+      feeToken: ZERO_ADDRESS,
+      jurorToken: this.anj,
+      voting: this.voting,
+      accounting: this.accounting,
+      subscriptions: this.subscriptions,
+      jurorsRegistry: this.jurorsRegistry,
+    })
 
     assert.equal(await this.jurorsRegistry.token(), this.anj.address, 'court token')
     await assertEqualBN(this.jurorsRegistry.totalActiveBalance(), 0, 'empty sum tree')
@@ -122,15 +98,15 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
     })
 
     it('can activate during period before heartbeat', async () => {
-      await this.jurorsRegistry.mockSetTimestamp(firstTermStart - 1)
-      await this.court.mockSetTimestamp(firstTermStart - 1)
+      await this.courtHelper.setTimestamp(firstTermStartTime - 1)
+
       await this.jurorsRegistry.activate(0, { from: juror1 }) // will activate all his funds
       await assertEqualBN(this.jurorsRegistry.totalActiveBalance(), juror1Stake, 'total tree sum')
     })
 
     it('gets the correct balance after activation', async () => {
-      await this.jurorsRegistry.mockSetTimestamp(firstTermStart - 1)
-      await this.court.mockSetTimestamp(firstTermStart - 1)
+      await this.courtHelper.setTimestamp(firstTermStartTime - 1)
+
       await this.jurorsRegistry.activate(0, { from: rich })
 
       const id = await this.jurorsRegistry.getJurorId(rich)
@@ -144,8 +120,8 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
     })
 
     it('reverts if activating balance is below dust', async () => {
-      await this.jurorsRegistry.mockSetTimestamp(firstTermStart - 1)
-      await this.court.mockSetTimestamp(firstTermStart - 1)
+      await this.courtHelper.setTimestamp(firstTermStartTime - 1)
+
       await assertRevert(this.jurorsRegistry.activate(0, { from: poor }), 'JR_INVALID_ZERO_AMOUNT')
       await assertRevert(this.jurorsRegistry.activate(10, { from: poor }), 'JR_INVALID_ACTIVATION_AMOUNT')
     })
@@ -155,18 +131,14 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => {
     const term = 3
 
     const passTerms = async terms => {
-      await this.court.mockIncreaseTime(terms * termDuration)
+      await this.courtHelper.increaseTime(terms * termDuration)
       await this.court.heartbeat(terms)
 
       assert.isTrue((await this.court.neededTermTransitions()).eq(0), 'all terms transitioned')
     }
 
-    beforeEach(async () => {
-      await this.jurorsRegistry.mockSetTimestamp(firstTermStart)
-      await this.court.mockSetTimestamp(firstTermStart)
-      await this.court.heartbeat(1)
-
-      await passTerms(2)
+    beforeEach('pass 3 terms', async () => {
+      await passTerms(3)
 
       await assertEqualBN(this.court.getLastEnsuredTermId(), term, 'term #3')
     })

diff --git a/test/court/court-initialize.js b/test/court/court-initialize.js
@@ -0,0 +1,29 @@
+const { buildHelper } = require('../helpers/court')(web3, artifacts)
+const { NOW, ONE_DAY } = require('../helpers/time')
+const { assertRevert } = require('@aragon/os/test/helpers/assertThrow')
+
+contract('Court', () => {
+  let courtHelper
+
+  beforeEach('create court helper', async () => {
+    courtHelper = buildHelper()
+  })
+
+  describe('initialization', () => {
+    it('cannot use a term duration greater than the first term start time', async () => {
+      await assertRevert(courtHelper.deploy({ mockedTimestamp: NOW, firstTermStartTime: ONE_DAY, termDuration: ONE_DAY + 1 }), 'CT_BAD_FIRST_TERM_START_TIME')
+    })
+
+    it('cannot use a first term start time in the past', async () => {
+      await assertRevert(courtHelper.deploy({ mockedTimestamp: NOW, firstTermStartTime: NOW - 1, termDuration: ONE_DAY }), 'CT_BAD_FIRST_TERM_START_TIME')
+    })
+
+    it('cannot use a penalty pct lower than 1% (1/10,000) ', async () => {
+      await assertRevert(courtHelper.deploy({ penaltyPct: 99 }), 'CTBAD_PENALTY')
+
+      const court = await courtHelper.deploy({ penaltyPct: 100 })
+      const penaltyPct = (await court.courtConfigs(1))[9] // config ID 0 is used for undefined
+      assert.equal(penaltyPct.toString(), 100, 'penalty pct does not match')
+    })
+  })
+})