Skip to content

Commit

Permalink
Revert "Update upvode function (#10268)"
Browse files Browse the repository at this point in the history 
This reverts commit ce75f49.
  • Loading branch information
@montera82
montera82 authored May 19, 2023
1 parent ce75f49 commit 377aa4a
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 100 deletions.
56 changes: 8 additions & 48 deletions packages/protocol/contracts/governance/Governance.sol
View file
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, 1, 0);
return (1, 3, 0, 0);
}

/**
Expand Down Expand Up @@ -527,21 +527,20 @@ contract Governance is
nonReentrant
returns (bool)
{
require(queue.contains(proposalId), "cannot upvote a proposal not in the queue");

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

dequeueProposalsIfReady();
// If acting on an expired proposal, expire the proposal and take no action.
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 @@ -550,9 +549,6 @@ 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 @@ -605,7 +601,7 @@ contract Governance is
}

/**
* @notice Approves a proposal in the approval stage or in the referendum stage.
* @notice Approves a proposal in the approval 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 @@ -760,6 +756,7 @@ contract Governance is
noVotes,
abstainVotes
);

} else {
proposal.updateVote(
previousVoteRecord.yesVotes,
Expand Down Expand Up @@ -1255,43 +1252,6 @@ 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
Expand Down
69 changes: 17 additions & 52 deletions packages/protocol/test/governance/network/governance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ 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 @@ -1109,7 +1110,9 @@ 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 @@ -1120,21 +1123,23 @@ contract('Governance', (accounts: string[]) => {
// @ts-ignore: TODO(mcortesi) fix typings for TransactionDetails
{ value: minDeposit }
)
await timeTravel(dequeueFrequency, web3)
})

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 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 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)
it('should revert when upvoting a proposal that will be dequeued', async () => {
await assertRevert(governance.upvote(proposalId, 0, 0))
})
})

Expand All @@ -1143,7 +1148,7 @@ contract('Governance', (accounts: string[]) => {
// Expire the upvoted proposal without dequeueing it.
const queueExpiry1 = 60
beforeEach(async () => {
await governance.setQueueExpiry(queueExpiry1)
await governance.setQueueExpiry(60)
await governance.upvote(proposalId, 0, 0)
await timeTravel(queueExpiry1, web3)
await governance.propose(
Expand Down Expand Up @@ -3576,46 +3581,6 @@ 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

0 comments on commit 377aa4a

Please sign in to comment.