Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update upvode function #10268

Merged
merged 9 commits into from
May 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 48 additions & 8 deletions packages/protocol/contracts/governance/Governance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -527,20 +527,21 @@ 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");
montera82 marked this conversation as resolved.
Show resolved Hide resolved

if (dequeueProposalIfReady(proposalId)) {
return false;
}

if (removeIfQueuedAndExpired(proposalId)) {
return false;
}

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"
Expand All @@ -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;
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -756,7 +760,6 @@ contract Governance is
noVotes,
abstainVotes
);

} else {
proposal.updateVote(
previousVoteRecord.yesVotes,
Expand Down Expand Up @@ -1252,6 +1255,43 @@ contract Governance is
}
}

/**
* @notice Removes the proposal from the queue if `lastDequeue` time has passed.
montera82 marked this conversation as resolved.
Show resolved Hide resolved
* @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];
montera82 marked this conversation as resolved.
Show resolved Hide resolved
// Updating refunds back to proposer
refundedDeposits[proposal.proposer] = refundedDeposits[proposal.proposer].add(
montera82 marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
69 changes: 52 additions & 17 deletions packages/protocol/test/governance/network/governance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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],
Expand All @@ -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)
montera82 marked this conversation as resolved.
Show resolved Hide resolved
await timeTravel(dequeueFrequency, web3)
await governance.upvote(upvotedProposalId, 0, proposalId)
assert.equal((await governance.getQueueLength()).toNumber(), 1)
montera82 marked this conversation as resolved.
Show resolved Hide resolved
})

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)
})
})

Expand All @@ -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(
Expand Down Expand Up @@ -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)
Expand Down