From 4850c083990289c4a5c2214fbc197f05dfb4f318 Mon Sep 17 00:00:00 2001 From: Edwin Tops Date: Wed, 19 Apr 2023 16:35:06 +0200 Subject: [PATCH 1/8] Update upvode function --- packages/protocol/contracts/governance/Governance.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 820a1874640..59513d6fd2a 100644 --- a/packages/protocol/contracts/governance/Governance.sol +++ b/packages/protocol/contracts/governance/Governance.sol @@ -527,6 +527,7 @@ contract Governance is nonReentrant returns (bool) { + require(queue.contains(proposalId), "cannot upvote a proposal not in the queue"); dequeueProposalsIfReady(); // If acting on an expired proposal, expire the proposal and take no action. if (removeIfQueuedAndExpired(proposalId)) { From 21112bbe85a99e9877188fbf101cf58b279fbd2c Mon Sep 17 00:00:00 2001 From: Edwin Tops Date: Tue, 2 May 2023 15:14:04 +0200 Subject: [PATCH 2/8] Add new logic --- .../contracts/governance/Governance.sol | 53 ++++++++++++++++--- 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 59513d6fd2a..06627d79281 100644 --- a/packages/protocol/contracts/governance/Governance.sol +++ b/packages/protocol/contracts/governance/Governance.sol @@ -528,8 +528,12 @@ contract Governance is returns (bool) { require(queue.contains(proposalId), "cannot upvote a proposal not in the queue"); - dequeueProposalsIfReady(); - // If acting on an expired proposal, expire the proposal and take no action. + bool isProposalDequeued = dequeueProposalIfReady(proposalId); + + if (isProposalDequeued) { + return false; + } + if (removeIfQueuedAndExpired(proposalId)) { return false; } @@ -537,11 +541,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" @@ -550,6 +551,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; } @@ -602,7 +606,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. @@ -757,7 +761,6 @@ contract Governance is noVotes, abstainVotes ); - } else { proposal.updateVote( previousVoteRecord.yesVotes, @@ -1253,6 +1256,42 @@ contract Governance is } } + /** + * @notice Removes the proposal from the queue if `lastDequeue` time has passed. + * @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 From 41bb641fb0abdb3c42dbda57d82af56bc678f511 Mon Sep 17 00:00:00 2001 From: Edwin Tops Date: Tue, 2 May 2023 15:14:23 +0200 Subject: [PATCH 3/8] Fix tests. --- .../test/governance/network/governance.ts | 68 ++++++++++++++----- 1 file changed, 52 insertions(+), 16 deletions(-) diff --git a/packages/protocol/test/governance/network/governance.ts b/packages/protocol/test/governance/network/governance.ts index 435a77c81c6..4e7cd49dbc2 100644 --- a/packages/protocol/test/governance/network/governance.ts +++ b/packages/protocol/test/governance/network/governance.ts @@ -1110,9 +1110,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 +1121,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 +1144,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 +3577,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) From 9406101a987adf6a2f1ce11480080bdf5bb20de3 Mon Sep 17 00:00:00 2001 From: Edwin Tops Date: Wed, 17 May 2023 15:33:14 +0200 Subject: [PATCH 4/8] Fix natspec. --- packages/protocol/contracts/governance/Governance.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 06627d79281..664927d145c 100644 --- a/packages/protocol/contracts/governance/Governance.sol +++ b/packages/protocol/contracts/governance/Governance.sol @@ -1258,6 +1258,7 @@ 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) { From 14b3ccf5d59d66fab7d6954e13c18d8a20eb69c4 Mon Sep 17 00:00:00 2001 From: Edwin Tops Date: Wed, 17 May 2023 15:36:15 +0200 Subject: [PATCH 5/8] Keep current format. --- packages/protocol/contracts/governance/Governance.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 664927d145c..686391252b7 100644 --- a/packages/protocol/contracts/governance/Governance.sol +++ b/packages/protocol/contracts/governance/Governance.sol @@ -528,9 +528,8 @@ contract Governance is returns (bool) { require(queue.contains(proposalId), "cannot upvote a proposal not in the queue"); - bool isProposalDequeued = dequeueProposalIfReady(proposalId); - if (isProposalDequeued) { + if (dequeueProposalIfReady(proposalId)) { return false; } From 578e97b778164c15b63fad6fc35ff9eb7b1ae1b2 Mon Sep 17 00:00:00 2001 From: Edwin Tops Date: Wed, 17 May 2023 17:24:43 +0200 Subject: [PATCH 6/8] Remove comment. --- packages/protocol/test/governance/network/governance.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/protocol/test/governance/network/governance.ts b/packages/protocol/test/governance/network/governance.ts index 4e7cd49dbc2..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 From 170817c5d526a18898dac4ac3435791e6c5f8a1e Mon Sep 17 00:00:00 2001 From: Edwin Tops Date: Thu, 18 May 2023 21:30:43 +0200 Subject: [PATCH 7/8] change version --- packages/protocol/contracts/governance/Governance.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 686391252b7..8009a1b7513 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, 0, 1); } /** From 37b7e4450523b054a8d4bef3feee3db69eefa395 Mon Sep 17 00:00:00 2001 From: Edwin Tops Date: Fri, 19 May 2023 13:46:30 +0200 Subject: [PATCH 8/8] Fix versioning --- packages/protocol/contracts/governance/Governance.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 8009a1b7513..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, 1); + return (1, 3, 1, 0); } /**