Skip to content

Commit

Permalink
DisputableVoting: Optimize quiet ending (aragon#1234)
Browse files Browse the repository at this point in the history 
* DisputableVoting: Improve quiet ending

* DisputableVoting: Make `wasFlipped` implementation clearer

* DisputableVoting: Use `_` prefix instead of suffix for vote storage variables

* DisputableVoting: Fix linter

* DisputableVoting: Use `_` prefix only for args
  • Loading branch information
@facuspagnuolo
facuspagnuolo committed Jul 30, 2020
1 parent 0440392 commit 3a08c7f
Show file tree
Hide file tree
Showing 7 changed files with 550 additions and 132 deletions.
219 changes: 123 additions & 96 deletions apps/voting-disputable/contracts/DisputableVoting.sol
View file

Large diffs are not rendered by default.

27 changes: 23 additions & 4 deletions apps/voting-disputable/test/contracts/voting_delegation.js
View file
Expand Up @@ -3,7 +3,7 @@ const { VOTING_ERRORS } = require('../helpers/errors')
const { VOTER_STATE, createVote, getVoteState } = require('../helpers/voting')

const { skipCoverage } = require('@aragon/os/test/helpers/coverage')
const { ONE_DAY, pct16, bigExp } = require('@aragon/contract-helpers-test')
const { ONE_DAY, pct16, bigExp, bn } = require('@aragon/contract-helpers-test')
const { assertBn, assertRevert, assertEvent, assertAmountOfEvents } = require('@aragon/contract-helpers-test/src/asserts')

contract('Voting delegation', ([_, owner, voter, anotherVoter, thirdVoter, representative, anotherRepresentative, anyone]) => {
Expand All @@ -13,6 +13,7 @@ contract('Voting delegation', ([_, owner, voter, anotherVoter, thirdVoter, repre
const MIN_SUPPORT = pct16(30)
const OVERRULE_WINDOW = ONE_DAY
const QUIET_ENDING_PERIOD = ONE_DAY * 4
const QUIET_ENDING_EXTENSION = ONE_DAY * 5
const VOTE_DURATION = ONE_DAY * 5
const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'

Expand All @@ -24,7 +25,7 @@ contract('Voting delegation', ([_, owner, voter, anotherVoter, thirdVoter, repre
})

beforeEach('deploy voting', async () => {
voting = await deployer.deployAndInitialize({ owner, minimumAcceptanceQuorum: MIN_QUORUM, requiredSupport: MIN_SUPPORT, voteDuration: VOTE_DURATION, overruleWindow: OVERRULE_WINDOW, quietEndingPeriod: QUIET_ENDING_PERIOD })
voting = await deployer.deployAndInitialize({ owner, minimumAcceptanceQuorum: MIN_QUORUM, requiredSupport: MIN_SUPPORT, voteDuration: VOTE_DURATION, overruleWindow: OVERRULE_WINDOW, quietEndingPeriod: QUIET_ENDING_PERIOD, quietEndingExtension: QUIET_ENDING_EXTENSION })
})

const getCastVote = async (voter, id = voteId) => voting.getCastVote(id, voter)
Expand Down Expand Up @@ -388,11 +389,29 @@ contract('Voting delegation', ([_, owner, voter, anotherVoter, thirdVoter, repre

if (shouldExtendVote) {
it('extends the vote duration', async () => {
const receipt = await voting.voteOnBehalfOf(voteId, support, [voter], { from })
// vote and move after the vote's end date
await voting.voteOnBehalfOf(voteId, support, [voter], { from })
await voting.mockIncreaseTime(QUIET_ENDING_PERIOD / 2)

assert.isTrue(await voting.canVote(voteId, thirdVoter), 'voter cannot vote')

const receipt = await voting.vote(voteId, true, { from: thirdVoter })
assertAmountOfEvents(receipt, 'VoteQuietEndingExtension')
assertEvent(receipt, 'VoteQuietEndingExtension', { expectedArgs: { voteId, passing: support } })
})

it('stores the vote extension in the following vote', async () => {
// vote and move after the vote's end date
await voting.voteOnBehalfOf(voteId, support, [voter], { from })
await voting.mockIncreaseTime(QUIET_ENDING_PERIOD / 2)

const { quietEndingExtendedSeconds: previousExtendedSeconds } = await getVoteState(voting, voteId)

await voting.vote(voteId, true, { from: thirdVoter })

const { quietEndingExtendedSeconds: currentExtendedSeconds } = await getVoteState(voting, voteId)
assertBn(currentExtendedSeconds, previousExtendedSeconds.add(bn(QUIET_ENDING_EXTENSION)), 'vote extended seconds do not match')
})
} else {
itDoesNotExtendTheVoteDuration(support)
}
Expand All @@ -409,7 +428,7 @@ contract('Voting delegation', ([_, owner, voter, anotherVoter, thirdVoter, repre
const shouldExtendVote = true

beforeEach('move to the middle of the quiet ending period', async () => {
await voting.mockIncreaseTime(VOTE_DURATION - QUIET_ENDING_PERIOD + 1)
await voting.mockIncreaseTime(VOTE_DURATION - QUIET_ENDING_PERIOD / 2)
})

itHandlesVoteDurationProperly(shouldExtendVote)
Expand Down
68 changes: 43 additions & 25 deletions apps/voting-disputable/test/contracts/voting_disputable.js
View file
@@ -1,41 +1,43 @@
const votingDeployer = require('../helpers/deployer')(web3, artifacts)
const agreementDeployer = require('@aragon/apps-agreement/test/helpers/utils/deployer')(web3, artifacts)
const { VOTING_ERRORS } = require('../helpers/errors')
const { VOTE_STATUS, createVote, voteScript, getVoteState } = require('../helpers/voting')
const { VOTE_STATUS, VOTER_STATE, createVote, voteScript, getVoteState } = require('../helpers/voting')

const { toAscii, utf8ToHex } = require('web3-utils')
const { RULINGS } = require('@aragon/apps-agreement/test/helpers/utils/enums')
const { ONE_DAY, pct16, bigExp, bn } = require('@aragon/contract-helpers-test')
const { assertBn, assertRevert, assertAmountOfEvents } = require('@aragon/contract-helpers-test/src/asserts')
const { assertBn, assertRevert, assertEvent, assertAmountOfEvents } = require('@aragon/contract-helpers-test/src/asserts')

contract('Voting disputable', ([_, owner, representative, voter20, voter29, voter51]) => {
contract('Voting disputable', ([_, owner, representative, voter10, voter20, voter29, voter40]) => {
let voting, token, agreement, voteId, actionId, collateralToken, executionTarget, script

const MIN_QUORUM = pct16(20)
const MIN_SUPPORT = pct16(20)
const MIN_QUORUM = pct16(10)
const MIN_SUPPORT = pct16(50)
const VOTING_DURATION = ONE_DAY * 5
const OVERRULE_WINDOW = ONE_DAY
const QUIET_ENDING_PERIOD = ONE_DAY * 2
const QUIET_ENDING_EXTENSION = ONE_DAY
const CONTEXT = utf8ToHex('some context')

before('deploy agreement and base voting', async () => {
agreement = await agreementDeployer.deployAndInitializeAgreementWrapper({ owner })
collateralToken = await agreementDeployer.deployCollateralToken()
votingDeployer.previousDeploy = agreementDeployer.previousDeploy

await agreement.sign(voter51)
await agreement.sign(voter40)
await votingDeployer.deployBase({ owner, agreement: true })
})

before('mint vote tokens', async () => {
token = await votingDeployer.deployToken({})
await token.generateTokens(voter51, bigExp(51, 18))
await token.generateTokens(voter40, bigExp(40, 18))
await token.generateTokens(voter29, bigExp(29, 18))
await token.generateTokens(voter20, bigExp(20, 18))
await token.generateTokens(voter10, bigExp(10, 18))
})

beforeEach('create voting app', async () => {
voting = await votingDeployer.deployAndInitialize({ owner, agreement: true, requiredSupport: MIN_SUPPORT, minimumAcceptanceQuorum: MIN_QUORUM, voteDuration: VOTING_DURATION, quietEndingPeriod: QUIET_ENDING_PERIOD, overruleWindow: OVERRULE_WINDOW })
voting = await votingDeployer.deployAndInitialize({ owner, agreement: true, requiredSupport: MIN_SUPPORT, minimumAcceptanceQuorum: MIN_QUORUM, voteDuration: VOTING_DURATION, quietEndingPeriod: QUIET_ENDING_PERIOD, quietEndingExtension: QUIET_ENDING_EXTENSION, overruleWindow: OVERRULE_WINDOW })
await voting.mockSetTimestamp(await agreement.currentTimestamp())

const SET_AGREEMENT_ROLE = await voting.SET_AGREEMENT_ROLE()
Expand All @@ -48,7 +50,7 @@ contract('Voting disputable', ([_, owner, representative, voter20, voter29, vote
})

beforeEach('create vote', async () => {
({ voteId } = await createVote({ voting, script, voteContext: CONTEXT, from: voter51 }))
({ voteId } = await createVote({ voting, script, voteContext: CONTEXT, from: voter40 }))
actionId = (await voting.getVote(voteId)).actionId
})

Expand All @@ -69,12 +71,12 @@ contract('Voting disputable', ([_, owner, representative, voter20, voter29, vote
assert.equal(disputable, voting.address, 'disputable address does not match')
assertBn(collateralRequirementId, 1, 'collateral ID does not match')
assert.equal(toAscii(context), 'some context', 'context does not match')
assert.equal(submitter, voter51, 'action submitter does not match')
assert.equal(submitter, voter40, 'action submitter does not match')
assert.isFalse(closed, 'action is not closed')
})

it('cannot be paused if ready to be executed', async () => {
await voting.vote(voteId, true, { from: voter51 })
await voting.vote(voteId, true, { from: voter40 })
await voting.mockIncreaseTime(VOTING_DURATION)

await assertRevert(agreement.challenge({ actionId }), 'AGR_CANNOT_CHALLENGE_ACTION')
Expand All @@ -83,7 +85,7 @@ contract('Voting disputable', ([_, owner, representative, voter20, voter29, vote

describe('execute', () => {
beforeEach('vote', async () => {
await voting.vote(voteId, true, { from: voter51 })
await voting.vote(voteId, true, { from: voter40 })
await voting.mockIncreaseTime(VOTING_DURATION)
await voting.executeVote(voteId)
})
Expand All @@ -107,7 +109,7 @@ contract('Voting disputable', ([_, owner, representative, voter20, voter29, vote
assert.equal(disputable, voting.address, 'disputable address does not match')
assertBn(collateralRequirementId, 1, 'collateral ID does not match')
assert.equal(toAscii(context), 'some context', 'context does not match')
assert.equal(submitter, voter51, 'action submitter does not match')
assert.equal(submitter, voter40, 'action submitter does not match')
})

it('cannot be paused', async () => {
Expand All @@ -120,7 +122,7 @@ contract('Voting disputable', ([_, owner, representative, voter20, voter29, vote

beforeEach('challenge vote', async () => {
currentTimestamp = await voting.getTimestampPublic()
await voting.vote(voteId, true, { from: voter51 })
await voting.vote(voteId, true, { from: voter40 })
await agreement.challenge({ actionId })
})

Expand Down Expand Up @@ -224,33 +226,49 @@ contract('Voting disputable', ([_, owner, representative, voter20, voter29, vote

it('does not affect the overrule window', async () => {
// the vote duration is 5 days and the overrule is 1 day, there still must be 4 days without overruling
await voting.setRepresentative(representative, { from: voter51 })
assert.isTrue(await voting.canVoteOnBehalfOf(voteId, [voter51], representative), 'should be able to vote')
await voting.setRepresentative(representative, { from: voter40 })
assert.isTrue(await voting.canVoteOnBehalfOf(voteId, [voter40], representative), 'should be able to vote')

// move fwd just 1 day
await voting.mockIncreaseTime(ONE_DAY)
assert.isTrue(await voting.canVoteOnBehalfOf(voteId, [voter51], representative), 'should be able to vote')
assert.isTrue(await voting.canVoteOnBehalfOf(voteId, [voter40], representative), 'should be able to vote')

// move fwd right before the overrule window starts
await voting.mockIncreaseTime(ONE_DAY * 3 - 1)
assert.isTrue(await voting.canVoteOnBehalfOf(voteId, [voter51], representative), 'should be able to vote')
assert.isTrue(await voting.canVoteOnBehalfOf(voteId, [voter40], representative), 'should be able to vote')

// move fwd right when the overrule window starts
await voting.mockIncreaseTime(1)
assert.isFalse(await voting.canVoteOnBehalfOf(voteId, [voter51], representative), 'should be able to vote')
assert.isFalse(await voting.canVoteOnBehalfOf(voteId, [voter40], representative), 'should be able to vote')
})

it('does not affect the quiet ending period', async () => {
// the vote duration is 5 days and the quiet ending is 2 days, there still must be 3 days without quiet ending
// move fwd right before the quiet ending period starts
await voting.mockIncreaseTime(ONE_DAY * 3 - 1)
const beforeQuietEnding = currentTimestamp.add(bn(VOTING_DURATION)).sub(bn(QUIET_ENDING_PERIOD + 1))
await voting.mockSetTimestamp(beforeQuietEnding)
const firstReceipt = await voting.vote(voteId, false, { from: voter29 })
assertAmountOfEvents(firstReceipt, 'VoteQuietEndingExtension', { expectedAmount: 0 })

// move fwd right when the quiet ending period starts
await voting.mockIncreaseTime(1)
const secondReceipt = await voting.vote(voteId, true, { from: voter51 })
assertAmountOfEvents(secondReceipt, 'VoteQuietEndingExtension', { expectedAmount: 1 })
const firstVoteState = await getVoteState(voting, voteId)
assertBn(firstVoteState.quietEndingSnapshotSupport, VOTER_STATE.ABSENT, 'quiet ending snapshot does not match')

// force flipped vote, move within quiet ending period
await voting.mockIncreaseTime(QUIET_ENDING_PERIOD / 2)
const secondReceipt = await voting.vote(voteId, true, { from: voter40 })
assertAmountOfEvents(secondReceipt, 'VoteQuietEndingExtension', { expectedAmount: 0 })

const secondVoteState = await getVoteState(voting, voteId)
assertBn(secondVoteState.quietEndingSnapshotSupport, VOTER_STATE.NAY, 'quiet ending snapshot does not match')

// move after the end of the vote and vote
await voting.mockIncreaseTime(QUIET_ENDING_PERIOD / 2 + QUIET_ENDING_EXTENSION / 2)
assert.isTrue(await voting.canVote(voteId, voter10), 'voter cannot vote')
const thirdReceipt = await voting.vote(voteId, true, { from: voter10 })
assertAmountOfEvents(thirdReceipt, 'VoteQuietEndingExtension', { expectedAmount: 1 })
assertEvent(thirdReceipt, 'VoteQuietEndingExtension', { voteId, passing: true })

const thirdVoteState = await getVoteState(voting, voteId)
assertBn(thirdVoteState.quietEndingSnapshotSupport, VOTER_STATE.NAY, 'quiet ending snapshot does not match')
})

it('cannot be challenged again', async () => {
Expand Down
4 changes: 2 additions & 2 deletions apps/voting-disputable/test/contracts/voting_gas_costs.js
View file
Expand Up @@ -46,11 +46,11 @@ contract('Voting', ([_, owner, voter]) => {
}

context('newVote', () => {
itCostsAtMost(344e3, async () => receipt)
itCostsAtMost(343e3, async () => receipt)
})

context('vote', () => {
itCostsAtMost(122e3, async () => await voting.vote(voteId, true, { from: voter }))
itCostsAtMost(123e3, async () => await voting.vote(voteId, true, { from: voter }))
})

context('challenge', () => {
Expand Down

0 comments on commit 3a08c7f

Please sign in to comment.