From 0766cfe94c171c332da39dbb2e94d8308b74f21e Mon Sep 17 00:00:00 2001 From: pahor167 <47992132+pahor167@users.noreply.github.com> Date: Tue, 1 Aug 2023 20:47:38 +0200 Subject: [PATCH 01/11] Revert "Fix dequeueProposalIfReady() for expired proposals. (#10324)" This reverts commit 890d1954fa7ef968f75d857b49a12807ac45fe3c. --- .../contracts/governance/Governance.sol | 6 ----- .../test/governance/network/governance.ts | 25 +++---------------- 2 files changed, 4 insertions(+), 27 deletions(-) diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 8ee3eb6cf13..d888322cb63 100644 --- a/packages/protocol/contracts/governance/Governance.sol +++ b/packages/protocol/contracts/governance/Governance.sol @@ -1265,12 +1265,6 @@ contract Governance is // 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 diff --git a/packages/protocol/test/governance/network/governance.ts b/packages/protocol/test/governance/network/governance.ts index 030fcafc926..f9f6b7b7360 100644 --- a/packages/protocol/test/governance/network/governance.ts +++ b/packages/protocol/test/governance/network/governance.ts @@ -3600,12 +3600,12 @@ contract('Governance', (accounts: string[]) => { const nonExistentProposalId = 7 const originalLastDequeue = await governance.lastDequeue() await timeTravel(dequeueFrequency, web3) - await governance.dequeueProposalIfReady(nonExistentProposalId) + 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', () => { - const proposalId = 1 beforeEach(async () => { await governance.propose( [transactionSuccess1.value], @@ -3618,35 +3618,18 @@ contract('Governance', (accounts: string[]) => { ) }) - 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) + 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(proposalId) + await governance.dequeueProposalIfReady(1) const isQueuedProposalExpired = await governance.isQueuedProposalExpired(1) assert.isFalse(isQueuedProposalExpired) }) From 61677aff2e686195f1a237c33533f0d06bf83d1a Mon Sep 17 00:00:00 2001 From: pahor167 <47992132+pahor167@users.noreply.github.com> Date: Tue, 1 Aug 2023 20:49:04 +0200 Subject: [PATCH 02/11] Revert "Update upvode function (#10268)" This reverts commit ce75f49564ce46f6b783d3d2f1c66d754ae347a0. --- .../contracts/governance/Governance.sol | 54 ++------------- .../test/governance/network/governance.ts | 69 +++++-------------- 2 files changed, 24 insertions(+), 99 deletions(-) diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index d888322cb63..9cecfb357c2 100644 --- a/packages/protocol/contracts/governance/Governance.sol +++ b/packages/protocol/contracts/governance/Governance.sol @@ -527,12 +527,8 @@ 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; } @@ -540,8 +536,11 @@ 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,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; } @@ -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. @@ -760,6 +756,7 @@ contract Governance is noVotes, abstainVotes ); + } else { proposal.updateVote( previousVoteRecord.yesVotes, @@ -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 diff --git a/packages/protocol/test/governance/network/governance.ts b/packages/protocol/test/governance/network/governance.ts index f9f6b7b7360..fd4740fb649 100644 --- a/packages/protocol/test/governance/network/governance.ts +++ b/packages/protocol/test/governance/network/governance.ts @@ -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 @@ -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], @@ -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)) }) }) @@ -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( @@ -3595,46 +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 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 0baeae6d63fa63970b4fd13f863e764ba2810d43 Mon Sep 17 00:00:00 2001 From: pahor167 <47992132+pahor167@users.noreply.github.com> Date: Wed, 2 Aug 2023 15:28:01 +0200 Subject: [PATCH 03/11] ReleaseGold refund test fix (#10440) * ReleaseGold refund test fix * gas fix --- .../protocol/scripts/truffle/releaseGoldExampleConfigs.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/protocol/scripts/truffle/releaseGoldExampleConfigs.json b/packages/protocol/scripts/truffle/releaseGoldExampleConfigs.json index a1ed128782c..bd8b934e594 100644 --- a/packages/protocol/scripts/truffle/releaseGoldExampleConfigs.json +++ b/packages/protocol/scripts/truffle/releaseGoldExampleConfigs.json @@ -4,7 +4,7 @@ "releaseStartTime": "MAINNET", "releaseCliffTime": 0, "numReleasePeriods": 1, - "releasePeriod": 100000000, + "releasePeriod": 300000000, "amountReleasedPerPeriod": 10002, "revocable": false, "beneficiary": "0x5409ED021D9299bf6814279A6A1411A7e866A631", @@ -20,7 +20,7 @@ "releaseStartTime": "MAINNET", "releaseCliffTime": 0, "numReleasePeriods": 1, - "releasePeriod": 100000000, + "releasePeriod": 300000000, "amountReleasedPerPeriod": 12, "revocable": true, "beneficiary": "0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb", From 902637db50c600a3d6457e84bb37df4695140496 Mon Sep 17 00:00:00 2001 From: pahor167 <47992132+pahor167@users.noreply.github.com> Date: Wed, 2 Aug 2023 23:56:49 +0200 Subject: [PATCH 04/11] Pahor/cli fix2 (#10457) * ReleaseGold refund test fix * gas fix * PR comments * removal of comment * Cli fix 2 --- packages/cli/src/commands/releasegold/withdraw.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/releasegold/withdraw.test.ts b/packages/cli/src/commands/releasegold/withdraw.test.ts index 5aaa8be8251..150da5491cc 100644 --- a/packages/cli/src/commands/releasegold/withdraw.test.ts +++ b/packages/cli/src/commands/releasegold/withdraw.test.ts @@ -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), @@ -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), From ddba6133d6d7d657f02b7086cf52dadf2dee36b9 Mon Sep 17 00:00:00 2001 From: Javier Cortejoso Date: Thu, 3 Aug 2023 15:20:20 +0200 Subject: [PATCH 05/11] Running GitHub Action workflow from release/* branches --- .github/workflows/circleci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/circleci.yml b/.github/workflows/circleci.yml index 46b3e3fd7bb..dee970b1042 100644 --- a/.github/workflows/circleci.yml +++ b/.github/workflows/circleci.yml @@ -8,9 +8,11 @@ on: push: branches: - master + - release/* pull_request: branches: - master + - release/* concurrency: group: circle-ci-${{ github.ref }} From 853a611e01243010ba10e8198c23b40aaec6e0fc Mon Sep 17 00:00:00 2001 From: Javier Cortejoso Date: Thu, 3 Aug 2023 15:22:48 +0200 Subject: [PATCH 06/11] Fix regex --- .github/workflows/circleci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/circleci.yml b/.github/workflows/circleci.yml index dee970b1042..bd9b89fb672 100644 --- a/.github/workflows/circleci.yml +++ b/.github/workflows/circleci.yml @@ -8,11 +8,11 @@ on: push: branches: - master - - release/* + - 'release/**' pull_request: branches: - master - - release/* + - 'release/**' concurrency: group: circle-ci-${{ github.ref }} From ed28473f303e085d982325bf866e2830d2728762 Mon Sep 17 00:00:00 2001 From: Javier Cortejoso Date: Thu, 3 Aug 2023 19:18:29 +0200 Subject: [PATCH 07/11] Trigger protocol tests on release branches --- .github/workflows/circleci.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/circleci.yml b/.github/workflows/circleci.yml index bd9b89fb672..ac1deb38e6d 100644 --- a/.github/workflows/circleci.yml +++ b/.github/workflows/circleci.yml @@ -219,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 }} @@ -260,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 @@ -291,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 From 168d11ad56d5f873c56171b12542a80e3291964c Mon Sep 17 00:00:00 2001 From: pahor167 <47992132+pahor167@users.noreply.github.com> Date: Thu, 3 Aug 2023 22:15:32 +0200 Subject: [PATCH 08/11] add logging --- packages/protocol/test/common/feehandler.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/protocol/test/common/feehandler.ts b/packages/protocol/test/common/feehandler.ts index 35b42f6937c..972bff2055d 100644 --- a/packages/protocol/test/common/feehandler.ts +++ b/packages/protocol/test/common/feehandler.ts @@ -523,8 +523,8 @@ 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 + console.log('Uniswap INIT CODE PAIR HASH:', await uniswapFactory.INIT_CODE_PAIR_HASH()) uniswap = await UniswapRouter.new( uniswapFactory.address, @@ -539,7 +539,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)) From 684c702d41869d4a2629b933b7b2a4afb1ccdbfe Mon Sep 17 00:00:00 2001 From: pahor167 <47992132+pahor167@users.noreply.github.com> Date: Thu, 3 Aug 2023 22:40:04 +0200 Subject: [PATCH 09/11] lint fix --- packages/protocol/test/common/feehandler.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/protocol/test/common/feehandler.ts b/packages/protocol/test/common/feehandler.ts index 972bff2055d..07eb9f7bb0a 100644 --- a/packages/protocol/test/common/feehandler.ts +++ b/packages/protocol/test/common/feehandler.ts @@ -524,6 +524,7 @@ contract('FeeHandler', (accounts: string[]) => { 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( From 137241708d5a6f49a991537c94cb7fc4fcade6a5 Mon Sep 17 00:00:00 2001 From: pahor167 <47992132+pahor167@users.noreply.github.com> Date: Thu, 3 Aug 2023 23:39:18 +0200 Subject: [PATCH 10/11] uniswap test fix --- .../contracts/uniswap/test/libraries/UniswapV2Library.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/protocol/contracts/uniswap/test/libraries/UniswapV2Library.sol b/packages/protocol/contracts/uniswap/test/libraries/UniswapV2Library.sol index 101bff91627..0c8c57a5dd9 100644 --- a/packages/protocol/contracts/uniswap/test/libraries/UniswapV2Library.sol +++ b/packages/protocol/contracts/uniswap/test/libraries/UniswapV2Library.sol @@ -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" ) ) From 1bc62a7c74c6e39f45d00183ad37c002bfacadae Mon Sep 17 00:00:00 2001 From: Javier Cortejoso Date: Fri, 4 Aug 2023 09:48:45 +0200 Subject: [PATCH 11/11] Trigger Identity tests too --- .github/workflows/circleci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/circleci.yml b/.github/workflows/circleci.yml index ac1deb38e6d..eb619da5c48 100644 --- a/.github/workflows/circleci.yml +++ b/.github/workflows/circleci.yml @@ -365,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