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

Revert of upvote function #10453

Merged
merged 11 commits into from
Aug 4, 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
6 changes: 6 additions & 0 deletions .github/workflows/circleci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ on:
push:
branches:
- master
- 'release/**'
pull_request:
branches:
- master
- 'release/**'

concurrency:
group: circle-ci-${{ github.ref }}
Expand Down Expand Up @@ -217,6 +219,7 @@ jobs:
contains(needs.install-dependencies.outputs.all_modified_files, 'packages/protocol') ||
contains(needs.install-dependencies.outputs.all_modified_files, ',package.json') ||
contains(needs.install-dependencies.outputs.all_modified_files, ',yarn.lock') ||
github.event_name == 'pull_request' && contains(github.base_ref, 'release/') ||
false
outputs:
protocol-test-must-run: ${{ steps.protocol-test-must-run.outputs.PROTOCOL_TEST_MUST_RUN }}
Expand Down Expand Up @@ -258,6 +261,7 @@ jobs:
contains(needs.install-dependencies.outputs.all_modified_files, 'packages/protocol') ||
contains(needs.install-dependencies.outputs.all_modified_files, ',package.json') ||
contains(needs.install-dependencies.outputs.all_modified_files, ',yarn.lock') ||
github.event_name == 'pull_request' && contains(github.base_ref, 'release/') ||
false
steps:
- uses: actions/cache/restore@v3
Expand Down Expand Up @@ -289,6 +293,7 @@ jobs:
contains(needs.install-dependencies.outputs.all_modified_files, 'packages/protocol') ||
contains(needs.install-dependencies.outputs.all_modified_files, ',package.json') ||
contains(needs.install-dependencies.outputs.all_modified_files, ',yarn.lock') ||
github.event_name == 'pull_request' && contains(github.base_ref, 'release/') ||
false
strategy:
fail-fast: false
Expand Down Expand Up @@ -360,6 +365,7 @@ jobs:
contains(needs.install-dependencies.outputs.all_modified_files, 'packages/contractkit') ||
contains(needs.install-dependencies.outputs.all_modified_files, ',package.json') ||
contains(needs.install-dependencies.outputs.all_modified_files, ',yarn.lock') ||
github.event_name == 'pull_request' && contains(github.base_ref, 'release/') ||
false
steps:
- uses: actions/cache/restore@v3
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/commands/releasegold/withdraw.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ testWithGanache('releasegold:withdraw cmd', (web3: Web3) => {
test('can withdraw released gold to beneficiary', async () => {
await testLocally(SetLiquidityProvision, ['--contract', contractAddress, '--yesreally'])
// ReleasePeriod of default contract
await timeTravel(100000000, web3)
await timeTravel(300000000, web3)
const releaseGoldWrapper = new ReleaseGoldWrapper(
kit.connection,
newReleaseGold(web3, contractAddress),
Expand All @@ -47,7 +47,7 @@ testWithGanache('releasegold:withdraw cmd', (web3: Web3) => {
test("can't withdraw the whole balance if there is a cUSD balance", async () => {
await testLocally(SetLiquidityProvision, ['--contract', contractAddress, '--yesreally'])
// ReleasePeriod of default contract
await timeTravel(100000000, web3)
await timeTravel(300000000, web3)
const releaseGoldWrapper = new ReleaseGoldWrapper(
kit.connection,
newReleaseGold(web3, contractAddress),
Expand Down
60 changes: 7 additions & 53 deletions packages/protocol/contracts/governance/Governance.sol
Original file line number Diff line number Diff line change
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,49 +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];

if (_isQueuedProposalExpired(proposal)) {
emit ProposalExpired(proposalId);
return isProposalDequeued;
}

// 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
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ library UniswapV2Library {
keccak256(abi.encodePacked(token0, token1)),
// This variable was hardcoded for Uniswap Eth Mainnet deployment
// hex"96e8ac4277198ff8b6f785478aa9a39f403cb768dd02cbee326c3e7da348845f" // init code hash
// Hash in the CI "bc307131606b79f0c50770dea78b35921ffca853421cbb031aa5a1ce5d6ea269"
hex"bc307131606b79f0c50770dea78b35921ffca853421cbb031aa5a1ce5d6ea269"
// Hash in the CI "dd00d753e268a006adece16f4a80a641f28edef4544174a2ba46ad72bcf58010"
hex"dd00d753e268a006adece16f4a80a641f28edef4544174a2ba46ad72bcf58010"
// hex"f0bd447d72bc4c5cd510462381a98e87f097a4d31106d6dd8b5922227696ef7a"
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"releaseStartTime": "MAINNET",
"releaseCliffTime": 0,
"numReleasePeriods": 1,
"releasePeriod": 100000000,
"releasePeriod": 300000000,
"amountReleasedPerPeriod": 10002,
"revocable": false,
"beneficiary": "0x5409ED021D9299bf6814279A6A1411A7e866A631",
Expand All @@ -20,7 +20,7 @@
"releaseStartTime": "MAINNET",
"releaseCliffTime": 0,
"numReleasePeriods": 1,
"releasePeriod": 100000000,
"releasePeriod": 300000000,
"amountReleasedPerPeriod": 12,
"revocable": true,
"beneficiary": "0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb",
Expand Down
4 changes: 2 additions & 2 deletions packages/protocol/test/common/feehandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -523,8 +523,9 @@ contract('FeeHandler', (accounts: string[]) => {
// console.log('Uniswap INIT CODE PAIR HASH:', await uniswapFactory.INIT_CODE_PAIR_HASH())
beforeEach(async () => {
deadline = (await web3.eth.getBlock('latest')).timestamp + 100

uniswapFactory = await UniswapV2Factory.new('0x0000000000000000000000000000000000000000') // feeSetter
// tslint:disable-next-line
console.log('Uniswap INIT CODE PAIR HASH:', await uniswapFactory.INIT_CODE_PAIR_HASH())

uniswap = await UniswapRouter.new(
uniswapFactory.address,
Expand All @@ -539,7 +540,6 @@ contract('FeeHandler', (accounts: string[]) => {
) // _factory, _WETH

await feeCurrencyWhitelist.addToken(tokenA.address)

await uniswapFeeHandlerSeller.initialize(registry.address, [], [])
await uniswapFeeHandlerSeller.setRouter(tokenA.address, uniswap.address)
await tokenA.mint(feeHandler.address, new BigNumber(10e18))
Expand Down
86 changes: 17 additions & 69 deletions packages/protocol/test/governance/network/governance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,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 @@ -1117,7 +1118,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 @@ -1128,21 +1131,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 @@ -1151,7 +1156,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 @@ -3595,63 +3600,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 governance.dequeueProposalIfReady(nonExistentProposalId)
assert.equal((await governance.getQueueLength()).toNumber(), 0)
assert.equal((await governance.lastDequeue()).toNumber(), originalLastDequeue.toNumber())
})
describe('when a proposal exists', () => {
const proposalId = 1
beforeEach(async () => {
await governance.propose(
[transactionSuccess1.value],
[transactionSuccess1.destination],
// @ts-ignore bytes type
transactionSuccess1.data,
[transactionSuccess1.data.length],
descriptionUrl,
{ value: minDeposit }
)
})

it('should not update `dequeued` when proposal has expired', async () => {
await timeTravel(queueExpiry, web3)
await governance.dequeueProposalIfReady(proposalId)
const dequeued = await governance.getDequeue()
assert.equal(dequeued.length, 0)
})

it('should update `dequeued` when proposal has not expired', async () => {
await timeTravel(dequeueFrequency, web3)
await governance.dequeueProposalIfReady(proposalId)
const dequeued = await governance.getDequeue()
assert.include(
dequeued.map((x) => x.toNumber()),
proposalId
)
})

it('should update lastDequeue', async () => {
const originalLastDequeue = await governance.lastDequeue()

await timeTravel(dequeueFrequency, web3)
await governance.dequeueProposalIfReady(proposalId)

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(proposalId)
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