diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 820a1874640..27ddcb5c633 100644 --- a/packages/protocol/contracts/governance/Governance.sol +++ b/packages/protocol/contracts/governance/Governance.sol @@ -220,7 +220,7 @@ contract Governance is * @return Patch version of the contract. */ function getVersionNumber() external pure returns (uint256, uint256, uint256, uint256) { - return (1, 3, 0, 0); + return (1, 3, 1, 0); } /** @@ -527,8 +527,12 @@ contract Governance is nonReentrant returns (bool) { - dequeueProposalsIfReady(); - // If acting on an expired proposal, expire the proposal and take no action. + require(queue.contains(proposalId), "cannot upvote a proposal not in the queue"); + + if (dequeueProposalIfReady(proposalId)) { + return false; + } + if (removeIfQueuedAndExpired(proposalId)) { return false; } @@ -536,11 +540,8 @@ contract Governance is address account = getAccounts().voteSignerToAccount(msg.sender); Voter storage voter = voters[account]; removeIfQueuedAndExpired(voter.upvote.proposalId); - - // We can upvote a proposal in the queue if we're not already upvoting a proposal in the queue. uint256 weight = getLockedGold().getAccountTotalLockedGold(account); require(weight > 0, "cannot upvote without locking gold"); - require(queue.contains(proposalId), "cannot upvote a proposal not in the queue"); require( voter.upvote.proposalId == 0 || !queue.contains(voter.upvote.proposalId), "cannot upvote more than one queued proposal" @@ -549,6 +550,9 @@ contract Governance is queue.update(proposalId, upvotes, lesser, greater); voter.upvote = UpvoteRecord(proposalId, weight); emit ProposalUpvoted(proposalId, account, weight); + + // dequeue other proposals if ready. + dequeueProposalsIfReady(); return true; } @@ -601,7 +605,7 @@ contract Governance is } /** - * @notice Approves a proposal in the approval stage. + * @notice Approves a proposal in the approval stage or in the referendum stage. * @param proposalId The ID of the proposal to approve. * @param index The index of the proposal ID in `dequeued`. * @return Whether or not the approval was made successfully. @@ -756,7 +760,6 @@ contract Governance is noVotes, abstainVotes ); - } else { proposal.updateVote( previousVoteRecord.yesVotes, @@ -1252,6 +1255,43 @@ contract Governance is } } + /** + * @notice Removes the proposal from the queue if `lastDequeue` time has passed. + * @param proposalId The ID of the proposal. + * @dev If any of the top proposals have expired, they are deleted. + */ + function dequeueProposalIfReady(uint256 proposalId) public returns (bool isProposalDequeued) { + isProposalDequeued = false; + // solhint-disable-next-line not-rely-on-time + if (now >= lastDequeue.add(dequeueFrequency)) { + Proposals.Proposal storage proposal = proposals[proposalId]; + // Updating refunds back to proposer + refundedDeposits[proposal.proposer] = refundedDeposits[proposal.proposer].add( + proposal.deposit + ); + queue.remove(proposalId); + // solhint-disable-next-line not-rely-on-time + proposal.timestamp = now; + if (emptyIndices.length > 0) { + uint256 indexOfLastEmptyIndex = emptyIndices.length.sub(1); + dequeued[emptyIndices[indexOfLastEmptyIndex]] = proposalId; + delete emptyIndices[indexOfLastEmptyIndex]; + emptyIndices.length = indexOfLastEmptyIndex; + } else { + dequeued.push(proposalId); + } + + // solhint-disable-next-line not-rely-on-time + emit ProposalDequeued(proposalId, now); + isProposalDequeued = true; + + // solhint-disable-next-line not-rely-on-time + lastDequeue = now; + } + + return isProposalDequeued; + } + /** * @notice Returns whether or not a proposal is in the queue. * @dev NOTE: proposal may be expired diff --git a/packages/protocol/test/governance/network/governance.ts b/packages/protocol/test/governance/network/governance.ts index 435a77c81c6..e47bace973b 100644 --- a/packages/protocol/test/governance/network/governance.ts +++ b/packages/protocol/test/governance/network/governance.ts @@ -82,7 +82,6 @@ interface Transaction { data: Buffer } -// TODO(asa): Test dequeueProposalsIfReady // TODO(asa): Dequeue explicitly to make the gas cost of operations more clear contract('Governance', (accounts: string[]) => { let governance: GovernanceTestInstance @@ -1110,9 +1109,7 @@ contract('Governance', (accounts: string[]) => { describe('when it has been more than dequeueFrequency since the last dequeue', () => { const upvotedProposalId = 2 - let originalLastDequeue: BigNumber beforeEach(async () => { - originalLastDequeue = await governance.lastDequeue() await governance.propose( [transactionSuccess1.value], [transactionSuccess1.destination], @@ -1123,23 +1120,21 @@ contract('Governance', (accounts: string[]) => { // @ts-ignore: TODO(mcortesi) fix typings for TransactionDetails { value: minDeposit } ) - await timeTravel(dequeueFrequency, web3) }) - it('should dequeue queued proposal(s)', async () => { - const queueLength = await governance.getQueueLength() - await governance.upvote(upvotedProposalId, 0, 0) - assert.isFalse(await governance.isQueued(proposalId)) - assert.equal( - (await governance.getQueueLength()).toNumber(), - queueLength.minus(concurrentProposals).toNumber() - ) - assertEqualBN(await governance.dequeued(0), proposalId) - assert.isBelow(originalLastDequeue.toNumber(), (await governance.lastDequeue()).toNumber()) + it('should only dequeue proposal(s) which is past lastDequeue time', async () => { + await governance.upvote(proposalId, upvotedProposalId, 0) + + assert.equal((await governance.getQueueLength()).toNumber(), 2) + await timeTravel(dequeueFrequency, web3) + await governance.upvote(upvotedProposalId, 0, proposalId) + assert.equal((await governance.getQueueLength()).toNumber(), 1) }) - it('should revert when upvoting a proposal that will be dequeued', async () => { - await assertRevert(governance.upvote(proposalId, 0, 0)) + it('should return false when upvoting a proposal that will be dequeued', async () => { + await timeTravel(dequeueFrequency, web3) + const isUpvoted = await governance.upvote.call(proposalId, 0, 0) + assert.isFalse(isUpvoted) }) }) @@ -1148,7 +1143,7 @@ contract('Governance', (accounts: string[]) => { // Expire the upvoted proposal without dequeueing it. const queueExpiry1 = 60 beforeEach(async () => { - await governance.setQueueExpiry(60) + await governance.setQueueExpiry(queueExpiry1) await governance.upvote(proposalId, 0, 0) await timeTravel(queueExpiry1, web3) await governance.propose( @@ -3581,6 +3576,46 @@ contract('Governance', (accounts: string[]) => { }) }) + describe('#dequeueProposalIfReady()', () => { + it('should not update lastDequeue proposal does not exist in the queue', async () => { + const nonExistentProposalId = 7 + const originalLastDequeue = await governance.lastDequeue() + await timeTravel(dequeueFrequency, web3) + await assertRevert(governance.dequeueProposalIfReady(nonExistentProposalId)) + + assert.equal((await governance.getQueueLength()).toNumber(), 0) + assert.equal((await governance.lastDequeue()).toNumber(), originalLastDequeue.toNumber()) + }) + describe('when a proposal exists', () => { + beforeEach(async () => { + await governance.propose( + [transactionSuccess1.value], + [transactionSuccess1.destination], + // @ts-ignore bytes type + transactionSuccess1.data, + [transactionSuccess1.data.length], + descriptionUrl, + { value: minDeposit } + ) + }) + + it('should update lastDequeue', async () => { + const originalLastDequeue = await governance.lastDequeue() + + await timeTravel(dequeueFrequency, web3) + await governance.dequeueProposalIfReady(1) + + assert.equal((await governance.getQueueLength()).toNumber(), 0) + assert.isTrue((await governance.lastDequeue()).toNumber() > originalLastDequeue.toNumber()) + }) + + it('should still be valid if not dequeued or expired', async () => { + await governance.dequeueProposalIfReady(1) + const isQueuedProposalExpired = await governance.isQueuedProposalExpired(1) + assert.isFalse(isQueuedProposalExpired) + }) + }) + }) describe('#getProposalStage()', () => { const expectStage = async (expected: Stage, _proposalId: number) => { const stage = await governance.getProposalStage(_proposalId)