Revert of upvote function (#10453)

* Revert "Fix dequeueProposalIfReady() for expired proposals. (#10324)" This reverts commit 890d195. * Revert "Update upvode function (#10268)" This reverts commit ce75f49. * ReleaseGold refund test fix (#10440) * ReleaseGold refund test fix * gas fix * Pahor/cli fix2 (#10457) * ReleaseGold refund test fix * gas fix * PR comments * removal of comment * Cli fix 2 * Running GitHub Action workflow from release/* branches * Fix regex * Trigger protocol tests on release branches * add logging * lint fix * uniswap test fix * Trigger Identity tests too --------- Co-authored-by: Javier Cortejoso <[email protected]>
celo-org · Aug 4, 2023 · 0678be0 · 0678be0
1 parent 49f3dc6
commit 0678be0
Show file tree

Hide file tree

Showing 7 changed files with 38 additions and 130 deletions.
diff --git 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 }}
@@ -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 }}
@@ -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
@@ -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
@@ -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

diff --git 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),

diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol
@@ -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"
@@ -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,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

diff --git 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"
           )
         )

diff --git 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",

diff --git a/packages/protocol/test/common/feehandler.ts b/packages/protocol/test/common/feehandler.ts
@@ -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,
@@ -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))

diff --git 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,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)