From 629f3fc34286b09a118c531f553a7083ae82cef1 Mon Sep 17 00:00:00 2001 From: Asa Oines Date: Wed, 22 Jan 2020 17:15:13 -0800 Subject: [PATCH 1/4] Try to repro in tests --- packages/protocol/test/governance/election.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/protocol/test/governance/election.ts b/packages/protocol/test/governance/election.ts index 0b686db72c9..a3e45b6518d 100644 --- a/packages/protocol/test/governance/election.ts +++ b/packages/protocol/test/governance/election.ts @@ -713,7 +713,7 @@ contract('Election', (accounts: string[]) => { }) }) - describe('#revokeActive', () => { + describe.only('#revokeActive', () => { const voter = accounts[0] const group = accounts[1] const value = 1000 @@ -729,6 +729,8 @@ contract('Election', (accounts: string[]) => { await election.vote(group, value, NULL_ADDRESS, NULL_ADDRESS) await mineBlocks(EPOCH, web3) await election.activate(group) + // Distribute some rewards to test rounding errors. + await election.distributeEpochRewards(group, 1, NULL_ADDRESS, NULL_ADDRESS) }) describe('when the revoked value is less than the active votes', () => { From 0f812a4a8b761a79d91faaba31b8abc0cfc87743 Mon Sep 17 00:00:00 2001 From: Asa Oines Date: Thu, 23 Jan 2020 20:09:57 -0800 Subject: [PATCH 2/4] Fix bug --- .../protocol/contracts/governance/Election.sol | 14 +++++++++++--- packages/protocol/test/governance/election.ts | 9 +++++---- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/packages/protocol/contracts/governance/Election.sol b/packages/protocol/contracts/governance/Election.sol index 98ad77e22c3..54264fc2b72 100644 --- a/packages/protocol/contracts/governance/Election.sol +++ b/packages/protocol/contracts/governance/Election.sol @@ -679,11 +679,19 @@ contract Election is ActiveVotes storage active = votes.active; active.total = active.total.sub(value); - uint256 unitsDelta = getActiveVotesUnitsDelta(group, value); - + // Rounding may cause getActiveVotesUnitsDelta to return 0 for value != 0, preventing users + // from revoking the last of their votes. The case where value == activeVotes is special cased + // to prevent this. + uint256 unitsDelta = 0; + uint256 activeVotes = getActiveVotesForGroupByAccount(group, account); GroupActiveVotes storage groupActive = active.forGroup[group]; - groupActive.total = groupActive.total.sub(value); + if (activeVotes == value) { + unitsDelta = groupActive.unitsByAccount[account]; + } else { + unitsDelta = getActiveVotesUnitsDelta(group, value); + } + groupActive.total = groupActive.total.sub(value); groupActive.totalUnits = groupActive.totalUnits.sub(unitsDelta); groupActive.unitsByAccount[account] = groupActive.unitsByAccount[account].sub(unitsDelta); } diff --git a/packages/protocol/test/governance/election.ts b/packages/protocol/test/governance/election.ts index a3e45b6518d..ff64f1f3707 100644 --- a/packages/protocol/test/governance/election.ts +++ b/packages/protocol/test/governance/election.ts @@ -717,20 +717,21 @@ contract('Election', (accounts: string[]) => { const voter = accounts[0] const group = accounts[1] const value = 1000 + const rewards = 1 describe('when the voter has active votes', () => { beforeEach(async () => { await mockValidators.setMembers(group, [accounts[9]]) await registry.setAddressFor(CeloContractName.Validators, accounts[0]) await election.markGroupEligible(group, NULL_ADDRESS, NULL_ADDRESS) await registry.setAddressFor(CeloContractName.Validators, mockValidators.address) - await mockLockedGold.setTotalLockedGold(value) + await mockLockedGold.setTotalLockedGold(value - rewards) await mockValidators.setNumRegisteredValidators(1) - await mockLockedGold.incrementNonvotingAccountBalance(voter, value) - await election.vote(group, value, NULL_ADDRESS, NULL_ADDRESS) + await mockLockedGold.incrementNonvotingAccountBalance(voter, value - rewards) + await election.vote(group, value - rewards, NULL_ADDRESS, NULL_ADDRESS) await mineBlocks(EPOCH, web3) await election.activate(group) // Distribute some rewards to test rounding errors. - await election.distributeEpochRewards(group, 1, NULL_ADDRESS, NULL_ADDRESS) + await election.distributeEpochRewards(group, rewards, NULL_ADDRESS, NULL_ADDRESS) }) describe('when the revoked value is less than the active votes', () => { From 5c55396007ffa187ad8016a0ab5191b3433b19f0 Mon Sep 17 00:00:00 2001 From: Asa Oines Date: Fri, 24 Jan 2020 10:44:30 -0800 Subject: [PATCH 3/4] Actually trigger edge case in tests --- packages/protocol/test/governance/election.ts | 88 ++++++++++++++----- 1 file changed, 66 insertions(+), 22 deletions(-) diff --git a/packages/protocol/test/governance/election.ts b/packages/protocol/test/governance/election.ts index ff64f1f3707..2fdff16f189 100644 --- a/packages/protocol/test/governance/election.ts +++ b/packages/protocol/test/governance/election.ts @@ -714,57 +714,69 @@ contract('Election', (accounts: string[]) => { }) describe.only('#revokeActive', () => { - const voter = accounts[0] - const group = accounts[1] - const value = 1000 - const rewards = 1 + const voter0 = accounts[0] + const voter1 = accounts[1] + const group = accounts[2] + const voteValue0 = 1000 + const reward0 = 111 + const voteValue1 = 1000 describe('when the voter has active votes', () => { beforeEach(async () => { await mockValidators.setMembers(group, [accounts[9]]) await registry.setAddressFor(CeloContractName.Validators, accounts[0]) await election.markGroupEligible(group, NULL_ADDRESS, NULL_ADDRESS) await registry.setAddressFor(CeloContractName.Validators, mockValidators.address) - await mockLockedGold.setTotalLockedGold(value - rewards) + await mockLockedGold.setTotalLockedGold(voteValue0 + voteValue1) await mockValidators.setNumRegisteredValidators(1) - await mockLockedGold.incrementNonvotingAccountBalance(voter, value - rewards) - await election.vote(group, value - rewards, NULL_ADDRESS, NULL_ADDRESS) + await mockLockedGold.incrementNonvotingAccountBalance(voter0, voteValue0) + await mockLockedGold.incrementNonvotingAccountBalance(voter1, voteValue1) + // Gives 1000 units to voter 0 + await election.vote(group, voteValue0, NULL_ADDRESS, NULL_ADDRESS) await mineBlocks(EPOCH, web3) await election.activate(group) - // Distribute some rewards to test rounding errors. - await election.distributeEpochRewards(group, rewards, NULL_ADDRESS, NULL_ADDRESS) + + // Makes those 1000 units represent 1111 votes. + await election.distributeEpochRewards(group, reward0, NULL_ADDRESS, NULL_ADDRESS) + + // Gives 900 units to voter 1. + // Revoking the 1111 votes would cause getActiveVotesUnitsDelta to return 999 units rather + // than 1000 units. + await election.vote(group, voteValue1, NULL_ADDRESS, NULL_ADDRESS, { from: voter1 }) + await mineBlocks(EPOCH, web3) + await election.activate(group, { from: voter1 }) }) describe('when the revoked value is less than the active votes', () => { const index = 0 - const revokedValue = value - 1 - const remaining = value - revokedValue + const revokedValue = voteValue0 + reward0 - 1 + const remaining = 1 let resp: any beforeEach(async () => { resp = await election.revokeActive(group, revokedValue, NULL_ADDRESS, NULL_ADDRESS, index) }) it("should decrement the account's active votes for the group", async () => { - assertEqualBN(await election.getActiveVotesForGroupByAccount(group, voter), remaining) + assertEqualBN(await election.getActiveVotesForGroupByAccount(group, voter0), remaining) }) it("should decrement the account's total votes for the group", async () => { - assertEqualBN(await election.getTotalVotesForGroupByAccount(group, voter), remaining) + assertEqualBN(await election.getTotalVotesForGroupByAccount(group, voter0), remaining) }) it("should decrement the account's total votes", async () => { - assertEqualBN(await election.getTotalVotesByAccount(voter), remaining) + assertEqualBN(await election.getTotalVotesByAccount(voter0), remaining) }) it('should decrement the total votes for the group', async () => { - assertEqualBN(await election.getTotalVotesForGroup(group), remaining) + assertEqualBN(await election.getTotalVotesForGroup(group), remaining + voteValue1) }) it('should decrement the total votes', async () => { - assertEqualBN(await election.getTotalVotes(), remaining) + assertEqualBN(await election.getTotalVotes(), remaining + voteValue1) }) it("should increment the account's nonvoting locked gold balance", async () => { - assertEqualBN(await mockLockedGold.nonvotingAccountBalance(voter), revokedValue) + assertEqualBN(await mockLockedGold.nonvotingAccountBalance(voter0), revokedValue) }) it('should emit the ValidatorGroupVoteRevoked event', async () => { @@ -773,7 +785,7 @@ contract('Election', (accounts: string[]) => { assertContainSubset(log, { event: 'ValidatorGroupVoteRevoked', args: { - account: voter, + account: voter0, group, value: new BigNumber(revokedValue), }, @@ -784,12 +796,38 @@ contract('Election', (accounts: string[]) => { describe('when the revoked value is equal to the active votes', () => { describe('when the correct index is provided', () => { const index = 0 + const remaining = 0 + const revokedValue = voteValue0 + reward0 beforeEach(async () => { - await election.revokeActive(group, value, NULL_ADDRESS, NULL_ADDRESS, index) + await election.revokeActive(group, revokedValue, NULL_ADDRESS, NULL_ADDRESS, index) + }) + + it("should decrement the account's active votes for the group", async () => { + assertEqualBN(await election.getActiveVotesForGroupByAccount(group, voter0), remaining) + }) + + it("should decrement the account's total votes for the group", async () => { + assertEqualBN(await election.getTotalVotesForGroupByAccount(group, voter0), remaining) + }) + + it("should decrement the account's total votes", async () => { + assertEqualBN(await election.getTotalVotesByAccount(voter0), remaining) + }) + + it('should decrement the total votes for the group', async () => { + assertEqualBN(await election.getTotalVotesForGroup(group), remaining + voteValue1) + }) + + it('should decrement the total votes', async () => { + assertEqualBN(await election.getTotalVotes(), remaining + voteValue1) + }) + + it("should increment the account's nonvoting locked gold balance", async () => { + assertEqualBN(await mockLockedGold.nonvotingAccountBalance(voter0), revokedValue) }) it('should remove the group to the list of groups the account has voted for', async () => { - assert.deepEqual(await election.getGroupsVotedForByAccount(voter), []) + assert.deepEqual(await election.getGroupsVotedForByAccount(voter0), []) }) }) @@ -797,7 +835,7 @@ contract('Election', (accounts: string[]) => { const index = 1 it('should revert', async () => { await assertRevert( - election.revokeActive(group, value, NULL_ADDRESS, NULL_ADDRESS, index) + election.revokeActive(group, voteValue0 + reward0, NULL_ADDRESS, NULL_ADDRESS, index) ) }) }) @@ -807,7 +845,13 @@ contract('Election', (accounts: string[]) => { const index = 0 it('should revert', async () => { await assertRevert( - election.revokeActive(group, value + 1, NULL_ADDRESS, NULL_ADDRESS, index) + election.revokeActive( + group, + voteValue0 + reward0 + 1, + NULL_ADDRESS, + NULL_ADDRESS, + index + ) ) }) }) From c7495c6c488d9ca7771b58db077ed8f68bff5c27 Mon Sep 17 00:00:00 2001 From: Asa Oines Date: Fri, 24 Jan 2020 14:46:12 -0800 Subject: [PATCH 4/4] Update packages/protocol/test/governance/election.ts Co-Authored-By: Victor "Nate" Graf --- packages/protocol/test/governance/election.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/test/governance/election.ts b/packages/protocol/test/governance/election.ts index 2fdff16f189..bc1cbaa13e7 100644 --- a/packages/protocol/test/governance/election.ts +++ b/packages/protocol/test/governance/election.ts @@ -713,7 +713,7 @@ contract('Election', (accounts: string[]) => { }) }) - describe.only('#revokeActive', () => { + describe('#revokeActive', () => { const voter0 = accounts[0] const voter1 = accounts[1] const group = accounts[2]