From 6b36ce4c70ff5242b0627808116b0fb4871b1ad2 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 14 Aug 2019 02:04:52 -0300 Subject: [PATCH] court: improve and test heartbeat function --- contracts/Court.sol | 93 +++++++----- contracts/test/lib/TimeHelpersMock.sol | 1 + test/court-batches.js | 2 +- test/court-disputes.js | 2 +- test/court-final-appeal-non-exact.js | 2 +- test/court-final-appeal.js | 2 +- test/court-lifecycle.js | 42 +----- test/court/court-terms.js | 193 +++++++++++++++++++++++++ 8 files changed, 255 insertions(+), 82 deletions(-) create mode 100644 test/court/court-terms.js diff --git a/contracts/Court.sol b/contracts/Court.sol index 607fb1c1..5e8f32e2 100644 --- a/contracts/Court.sol +++ b/contracts/Court.sol @@ -124,19 +124,20 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim // Court state uint64 internal termId; uint64 public configChangeTermId; - mapping (uint64 => Term) public terms; + mapping (uint64 => Term) internal terms; Dispute[] public disputes; string internal constant ERROR_INVALID_ADDR = "CTBAD_ADDR"; string internal constant ERROR_DEPOSIT_FAILED = "CTDEPOSIT_FAIL"; string internal constant ERROR_TOO_MANY_TRANSITIONS = "CTTOO_MANY_TRANSITIONS"; - string internal constant ERROR_UNFINISHED_TERM = "CTUNFINISHED_TERM"; + string internal constant ERROR_INVALID_TRANSITION_TERMS = "CT_INVALID_TRANSITION_TERMS"; string internal constant ERROR_PAST_TERM_FEE_CHANGE = "CTPAST_TERM_FEE_CHANGE"; string internal constant ERROR_OVERFLOW = "CTOVERFLOW"; string internal constant ERROR_ROUND_ALREADY_DRAFTED = "CTROUND_ALRDY_DRAFTED"; string internal constant ERROR_NOT_DRAFT_TERM = "CTNOT_DRAFT_TERM"; string internal constant ERROR_TERM_RANDOMNESS_NOT_YET = "CTRANDOM_NOT_YET"; string internal constant ERROR_WRONG_TERM = "CTBAD_TERM"; + string internal constant ERROR_BAD_FIRST_TERM_START_TIME = "CT_BAD_FIRST_TERM_START_TIME"; string internal constant ERROR_TERM_RANDOMNESS_UNAVAIL = "CTRANDOM_UNAVAIL"; string internal constant ERROR_INVALID_DISPUTE_STATE = "CTBAD_DISPUTE_STATE"; string internal constant ERROR_INVALID_ADJUDICATION_ROUND = "CTBAD_ADJ_ROUND"; @@ -231,7 +232,9 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim uint32 _maxRegularAppealRounds, uint256[5] _subscriptionParams // _periodDuration, _feeAmount, _prePaymentPeriods, _latePaymentPenaltyPct, _governorSharePct ) public { - require(_firstTermStartTime >= _termDuration, ERROR_WRONG_TERM); + 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); termDuration = _termDuration; jurorsRegistry = _jurorsRegistry; @@ -611,6 +614,21 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim return termId; } + /** + * @dev Tell the information related to a term based on its ID. Note that if the term has not been reached, the + * information returned won't be computed yet. + * @param _termId ID of the term being queried + * @return Term start time + * @return Number of drafts depending on the requested term + * @return ID of the court configuration associated to the requested term + * @return Block number used for randomness in the requested term + * @return Randomness computed for the requested term + */ + function getTerm(uint64 _termId) external view returns (uint64, uint64, uint64, uint64, bytes32) { + Term storage term = terms[_termId]; + return (term.startTime, term.dependingDrafts, term.courtConfigId, term.randomnessBN, term.randomness); + } + function getLastEnsuredTermId() external view returns (uint64) { return termId; } @@ -652,38 +670,35 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim /** * @notice Send a heartbeat to the Court to transition up to `_termTransitions` */ - function heartbeat(uint64 _termTransitions) public { - require(canTransitionTerm(), ERROR_UNFINISHED_TERM); - - Term storage prevTerm = terms[termId]; - termId += 1; - Term storage nextTerm = terms[termId]; - address heartbeatSender = msg.sender; - - // Set fee structure for term - if (nextTerm.courtConfigId == 0) { - nextTerm.courtConfigId = prevTerm.courtConfigId; - } else { - configChangeTermId = ZERO_TERM_ID; // fee structure changed in this term - } - - // TODO: skip period if you can - - // Set the start time of the term (ensures equally long terms, regardless of heartbeats) - nextTerm.startTime = prevTerm.startTime + termDuration; - nextTerm.randomnessBN = getBlockNumber64() + 1; // randomness source set to next block (content unknown when heartbeat happens) - - CourtConfig storage courtConfig = courtConfigs[nextTerm.courtConfigId]; - uint256 totalFee = nextTerm.dependingDrafts * courtConfig.heartbeatFee; - - if (totalFee > 0) { - accounting.assign(courtConfig.feeToken, heartbeatSender, totalFee); + function heartbeat(uint64 _maxRequestedTransitions) public { + uint64 neededTransitions = neededTermTransitions(); + uint256 transitions = uint256(_maxRequestedTransitions < neededTransitions ? _maxRequestedTransitions : neededTransitions); + require(transitions > uint256(0), ERROR_INVALID_TRANSITION_TERMS); + + // Transition the minimum number of terms between the amount requested and the amount actually needed + uint256 totalFee; + for (uint256 transition = 1; transition <= transitions; transition++) { + Term storage previousTerm = terms[termId++]; + Term storage nextTerm = terms[termId]; + + // TODO: allow config to be changed for a future term id + nextTerm.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; + // 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; + 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))); } - emit NewTerm(termId, heartbeatSender); - - if (_termTransitions > 1 && canTransitionTerm()) { - heartbeat(_termTransitions - 1); + // Pay heartbeat fees to the caller of this function + if (totalFee > uint256(0)) { + accounting.assign(config.feeToken, msg.sender, totalFee); } } @@ -696,10 +711,6 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim return round.filledSeats == round.jurorNumber; } - function canTransitionTerm() public view returns (bool) { - return neededTermTransitions() >= 1; - } - function neededTermTransitions() public view returns (uint64) { return (getTimestamp64() - terms[termId].startTime) / termDuration; } @@ -1171,6 +1182,14 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim return courtConfigs[courtConfigId]; } + /** + * @dev Internal function to compute the randomness that will be used to draft jurors for the given term. This + * function assumes the given term exists. To determine the randomness factor for a term we use the hash of a + * block number that is set once the term has started to ensure it cannot be known beforehand. Note that the + * hash function being used only works for the 256 most recent block numbers. + * @param _term Term to compute the randomness of + * @return Randomness computed for the given term + */ function _getTermRandomness(Term storage _term) internal view returns (bytes32) { require(getBlockNumber64() > _term.randomnessBN, ERROR_TERM_RANDOMNESS_NOT_YET); return blockhash(_term.randomnessBN); diff --git a/contracts/test/lib/TimeHelpersMock.sol b/contracts/test/lib/TimeHelpersMock.sol index 9874f89e..73d1ba7c 100644 --- a/contracts/test/lib/TimeHelpersMock.sol +++ b/contracts/test/lib/TimeHelpersMock.sol @@ -9,6 +9,7 @@ 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 index b4b16532..84ca2157 100644 --- a/test/court-batches.js +++ b/test/court-batches.js @@ -110,7 +110,7 @@ contract('Court: Batches', ([ rich, arbitrable, juror1, juror2, juror3, juror4, await this.courtHelper.increaseTime(terms * termDuration) await this.court.heartbeat(terms) - assert.isFalse(await this.court.canTransitionTerm(), 'all terms transitioned') + assert.isTrue((await this.court.neededTermTransitions()).eq(0), 'all terms transitioned') } context('on multiple draft batches', () => { diff --git a/test/court-disputes.js b/test/court-disputes.js index d3bd014e..9f4283ac 100644 --- a/test/court-disputes.js +++ b/test/court-disputes.js @@ -162,7 +162,7 @@ contract('Court: Disputes', ([ rich, juror1, juror2, juror3, other, appealMaker, await this.courtHelper.increaseTime(terms * termDuration) await this.court.heartbeat(terms) - assert.isFalse(await this.court.canTransitionTerm(), 'all terms transitioned') + assert.isTrue((await this.court.neededTermTransitions()).eq(0), 'all terms transitioned') } beforeEach(async () => { diff --git a/test/court-final-appeal-non-exact.js b/test/court-final-appeal-non-exact.js index 1b643154..96214516 100644 --- a/test/court-final-appeal-non-exact.js +++ b/test/court-final-appeal-non-exact.js @@ -124,7 +124,7 @@ contract('Court: final appeal (non-exact)', ([ poor, rich, juror1, juror2, juror await this.courtHelper.increaseTime(terms * termDuration) await this.court.heartbeat(terms) - assert.isFalse(await this.court.canTransitionTerm(), 'all terms transitioned') + assert.isTrue((await this.court.neededTermTransitions()).eq(0), 'all terms transitioned') } context('Final appeal, non-exact stakes', () => { diff --git a/test/court-final-appeal.js b/test/court-final-appeal.js index a8558353..8135c9e3 100644 --- a/test/court-final-appeal.js +++ b/test/court-final-appeal.js @@ -128,7 +128,7 @@ contract('Court: final appeal', ([ poor, rich, juror1, juror2, juror3, juror4, j await this.courtHelper.increaseTime(terms * termDuration) await this.court.heartbeat(terms) - assert.isFalse(await this.court.canTransitionTerm(), 'all terms transitioned') + assert.isTrue((await this.court.neededTermTransitions()).eq(0), 'all terms transitioned') } // TODO: Fix when making the court settings configurable diff --git a/test/court-lifecycle.js b/test/court-lifecycle.js index 7b37e5c5..132d110a 100644 --- a/test/court-lifecycle.js +++ b/test/court-lifecycle.js @@ -121,25 +121,6 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => { await assertEqualBN(this.court.getLastEnsuredTermId(), 0, 'court term #0') }) - it('transitions to term #1 on heartbeat', async () => { - await this.jurorsRegistry.mockSetTimestamp(15) - await this.court.mockSetTimestamp(15) - await assertLogs(this.court.heartbeat(1), NEW_TERM_EVENT) - - await assertEqualBN(this.court.getLastEnsuredTermId(), 1, 'court term #1') - const [ - startTime, - dependingDraws, - courtConfigId, - randomnessBn - ] = await this.court.terms(1) - - await assertEqualBN(startTime, firstTermStart, 'first term start') - await assertEqualBN(dependingDraws, 0, 'depending draws') - await assertEqualBN(courtConfigId, 1, 'court config id') - await assertEqualBN(randomnessBn, (await this.court.getBlockNumberExt()), 'randomeness bn') - }) - it('can activate during period before heartbeat', async () => { await this.jurorsRegistry.mockSetTimestamp(firstTermStart - 1) await this.court.mockSetTimestamp(firstTermStart - 1) @@ -168,13 +149,6 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => { await assertRevert(this.jurorsRegistry.activate(0, { from: poor }), 'JR_INVALID_ZERO_AMOUNT') await assertRevert(this.jurorsRegistry.activate(10, { from: poor }), 'JR_INVALID_ACTIVATION_AMOUNT') }) - - it("doesn't perform more transitions than requested", async () => { - await this.jurorsRegistry.mockSetTimestamp(firstTermStart + termDuration * 100) - await this.court.mockSetTimestamp(firstTermStart + termDuration * 100) - await this.court.heartbeat(3) - await assertEqualBN(this.court.getLastEnsuredTermId(), 3, 'current term') - }) }) context('on regular court terms', () => { @@ -184,7 +158,7 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => { await this.court.mockIncreaseTime(terms * termDuration) await this.court.heartbeat(terms) - assert.isFalse(await this.court.canTransitionTerm(), 'all terms transitioned') + assert.isTrue((await this.court.neededTermTransitions()).eq(0), 'all terms transitioned') } beforeEach(async () => { @@ -197,20 +171,6 @@ contract('Court: Lifecycle', ([ poor, rich, governor, juror1, juror2 ]) => { await assertEqualBN(this.court.getLastEnsuredTermId(), term, 'term #3') }) - it('has correct term state', async () => { - const [ - startTime, - dependingDraws, - courtConfigId, - randomnessBn - ] = await this.court.terms(term) - - await assertEqualBN(startTime, firstTermStart + (term - 1) * termDuration, 'term start') - await assertEqualBN(dependingDraws, 0, 'depending draws') - await assertEqualBN(courtConfigId, 1, 'court config id') - await assertEqualBN(randomnessBn, (await this.court.getBlockNumberExt()), 'randomeness bn') - }) - it('jurors can activate', async () => { await this.jurorsRegistry.activate(0, { from: juror1 }) await this.jurorsRegistry.activate(0, { from: juror2 }) diff --git a/test/court/court-terms.js b/test/court/court-terms.js new file mode 100644 index 00000000..56a999fc --- /dev/null +++ b/test/court/court-terms.js @@ -0,0 +1,193 @@ +const { bigExp } = require('../helpers/numbers')(web3) +const { buildHelper } = require('../helpers/court')(web3, artifacts) +const { assertRevert } = require('@aragon/os/test/helpers/assertThrow') +const { assertAmountOfEvents, assertEvent } = require('@aragon/os/test/helpers/assertEvent')(web3) + +const MiniMeToken = artifacts.require('MiniMeToken') + +const ONE_DAY = 60 * 60 * 24 +const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000' +const EMPTY_RANDOMNESS = '0x0000000000000000000000000000000000000000000000000000000000000000' + +contract('Court', ([_, sender]) => { + let courtHelper, court, feeToken + + const TERM_DURATION = ONE_DAY + const FIRST_TERM_START_TIME = 1565724237 // random timestamp + const HEARTBEAT_FEE = bigExp(50, 18) // 50 fee tokens + + beforeEach('create court', async () => { + courtHelper = buildHelper() + feeToken = await MiniMeToken.new(ZERO_ADDRESS, ZERO_ADDRESS, 0, 'Court Fee Token', 18, 'CFT', true) + court = await courtHelper.deploy({ + firstTermStartTime: FIRST_TERM_START_TIME, + termDuration: ONE_DAY, + feeToken: feeToken, + heartbeatFee: HEARTBEAT_FEE, + }) + }) + + beforeEach('mock timestamp to first term start time', async () => { + await courtHelper.setTimestamp(FIRST_TERM_START_TIME) + }) + + describe('first term', () => { + it('the must have already started', async () => { + const [startTime, dependingDrafts, courtConfigId, randomnessBN, randomness] = await court.getTerm(0) + + assert.equal(startTime.toString(), FIRST_TERM_START_TIME - TERM_DURATION, 'first term start time does not match') + assert.equal(dependingDrafts.toString(), 0, 'first term should not have depending drafts initially') + assert.equal(courtConfigId.toString(), 1, 'first term config should not be set') + assert.equal(randomnessBN.toString(), 0, 'first term randomness block number should not be computed') + assert.equal(randomness, EMPTY_RANDOMNESS, 'first term randomness should not be computed') + }) + + it('requires only one transition', async () => { + assert.equal((await court.neededTermTransitions()).toString(), 1, 'needed term transitions does not match') + }) + + it.skip('cannot be set before current timestamp', async () => { + // TODO: we cannot test this currently since timestamps cannot be mocked during initialization currently + }) + }) + + describe('heartbeat', () => { + const itUpdatesTermsSuccessfully = (maxTransitionTerms, expectedTransitions) => { + it('updates the term id', async () => { + const lastEnsuredTermId = await court.getLastEnsuredTermId() + + const receipt = await court.heartbeat(maxTransitionTerms, { from: sender }) + + assertAmountOfEvents(receipt, 'NewTerm', expectedTransitions) + + for (let transition = 1; transition <= expectedTransitions; transition++) { + assertEvent(receipt, 'NewTerm', { termId: lastEnsuredTermId.plus(transition), heartbeatSender: sender }, transition - 1) + } + }) + + it('initializes a new term', async () => { + const lastEnsuredTermId = await court.getLastEnsuredTermId() + const currentBlockNumber = await court.getBlockNumberExt() + + await court.heartbeat(maxTransitionTerms, { from: sender }) + + for (let transition = 1; transition <= expectedTransitions; transition++) { + const termId = lastEnsuredTermId.plus(transition) + const [startTime, dependingDrafts, courtConfigId, randomnessBN, randomness] = await court.getTerm(termId) + + assert.equal(startTime.toString(), FIRST_TERM_START_TIME + (TERM_DURATION * (transition - 1)), `start time for term ${termId} does not match`) + assert.equal(dependingDrafts.toString(), 0, `term ${termId} should not have depending drafts initially`) + assert.equal(courtConfigId.toString(), 1, `term ${termId} should be using the previous config`) + assert.equal(randomnessBN.toString(), currentBlockNumber.plus(1).toString(), `randomness block number for term ${termId} should be the next block number`) + assert.equal(randomness, EMPTY_RANDOMNESS, `randomness for term ${termId} should not be computed`) + } + }) + } + + context('when no time has passed after creation', () => { + it('requires only one transition', async () => { + assert.equal((await court.neededTermTransitions()).toString(), 1, 'needed term transitions does not match') + }) + + context('when the max transition terms given is zero', () => { + const maxTransitionTerms = 0 + + it('reverts', async () => { + await assertRevert(court.heartbeat(maxTransitionTerms, { from: sender }), 'CT_INVALID_TRANSITION_TERMS') + }) + }) + + context('when the max transition terms given is one', () => { + const maxTransitionTerms = 1 + const expectedTransitions = 1 + + itUpdatesTermsSuccessfully(maxTransitionTerms, expectedTransitions) + }) + + context('when the max transition terms given is two', () => { + const maxTransitionTerms = 2 + const expectedTransitions = 1 + + itUpdatesTermsSuccessfully(maxTransitionTerms, expectedTransitions) + }) + }) + + context('when it has passed one term after creation', () => { + beforeEach('increase timestamp one term', async () => { + await courtHelper.increaseTime(TERM_DURATION) + }) + + it('requires two transition', async () => { + assert.equal((await court.neededTermTransitions()).toString(), 2, 'needed term transitions does not match') + }) + + context('when the max transition terms given is zero', () => { + const maxTransitionTerms = 0 + + it('reverts', async () => { + await assertRevert(court.heartbeat(maxTransitionTerms, { from: sender }), 'CT_INVALID_TRANSITION_TERMS') + }) + }) + + context('when the max transition terms given is one', () => { + const maxTransitionTerms = 1 + const expectedTransitions = 1 + + itUpdatesTermsSuccessfully(maxTransitionTerms, expectedTransitions) + }) + + context('when the max transition terms given is two', () => { + const maxTransitionTerms = 2 + const expectedTransitions = 2 + + itUpdatesTermsSuccessfully(maxTransitionTerms, expectedTransitions) + }) + + context('when the max transition terms given is three', () => { + const maxTransitionTerms = 3 + const expectedTransitions = 2 + + itUpdatesTermsSuccessfully(maxTransitionTerms, expectedTransitions) + }) + }) + + context('when it has passed two terms after creation', () => { + beforeEach('increase timestamp two terms', async () => { + await courtHelper.increaseTime(TERM_DURATION * 2) + }) + + it('requires three transition', async () => { + assert.equal((await court.neededTermTransitions()).toString(), 3, 'needed term transitions does not match') + }) + + context('when the max transition terms given is zero', () => { + const maxTransitionTerms = 0 + + it('reverts', async () => { + await assertRevert(court.heartbeat(maxTransitionTerms, { from: sender }), 'CT_INVALID_TRANSITION_TERMS') + }) + }) + + context('when the max transition terms given is one', () => { + const maxTransitionTerms = 1 + const expectedTransitions = 1 + + itUpdatesTermsSuccessfully(maxTransitionTerms, expectedTransitions) + }) + + context('when the max transition terms given is two', () => { + const maxTransitionTerms = 2 + const expectedTransitions = 2 + + itUpdatesTermsSuccessfully(maxTransitionTerms, expectedTransitions) + }) + + context('when the max transition terms given is three', () => { + const maxTransitionTerms = 3 + const expectedTransitions = 3 + + itUpdatesTermsSuccessfully(maxTransitionTerms, expectedTransitions) + }) + }) + }) +}) \ No newline at end of file