Added post-proposal Alpha tests and Bravo tests #8

jferas · 2023-11-13T15:29:49Z

Added tests for Alpha governor after upgrade proposal and proposal actions on Bravo governor.

jferas · 2023-11-13T15:30:04Z

Current dependencies on/for this PR:

main
- PR forge install: openzeppelin-contracts #1
  - PR Initial code for Radworks upgraded Governor #2
    - PR Initial tests and architecture #5
      - PR Added post-proposal Alpha tests and Bravo tests #8 👈
        
        PR Updated README.md file #12
        
        PR Implement efficient settable quorum without library #15
        
        PR Pulled alpha governor tests out into a separate suite #14
        
        PR Added transfer of RAD tokens to ScopeLift upon successful upgrade to governor Bravo #19
        
        PR Pauser and renounceAdmin Drips tests #18
        
        PR Added tests for changing admin/upgrading protocol via governance #21
        
        PR Cleanup test names / scopelint fix #22
        
        PR Added GovernorVotesCompQuorumFraction for settable quorum #13

This stack of pull requests is managed by Graphite.

alexkeating

Looking good a few small comments

alexkeating · 2023-11-17T12:07:26Z

test/RadworksGovernor.t.sol

@@ -1,8 +1,11 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 pragma solidity ^0.8.18;

-import {ERC20VotesComp} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20VotesComp.sol";
+import {console2} from "forge-std/console2.sol";


Leftover console import

Done b06582b

alexkeating · 2023-11-17T12:09:07Z

test/RadworksGovernor.t.sol

+    assertEq(TIMELOCK.balance, _timelockETHBalance - _amount);
+  }
+
+  // TODO: This test commented out as the timelock has no ETH as of 2023-11-08


It would be good to create an issue and link it here.

New issue created.. #16

related a bit to this TODO: the question had come up for me earlier, why not deal / vm.deal some ETH / tokens to addresses in relevant tests? not saying we should do it, just curious if there's a reason not to

I don't think there is a reason not to

As it turns out we already had a test like this with "vm.deal".. this was a test with "nodeal" in the name, removed it.. 1832b53

alexkeating · 2023-11-21T18:45:08Z

foundry.toml

@@ -10,7 +10,7 @@
  verbosity = 3

 [profile.ci]
-  fuzz = { runs = 5000 }
+  fuzz = { runs = 50 }


One thing we did in the PoolTogether upgrade to help deal with all of these tests was to also use a fuzz seed.

It would involve adding to the foundry.toml and updating the c

fuzz = { seed = 0x1 }

And also update the CI. There is an example in the PoolTogether upgrade

Done aa49609

alexkeating · 2023-11-21T21:01:34Z

test/RadworksGovernor.t.sol

+
+    // Counter-intuitively, the Governor (not the Timelock) must hold the ETH.
+    // See the deployed GovernorAlpha, line 281:
+    //   https://etherscan.io/address/0x31c8EAcBFFdD875c74b94b077895Bd78CF1E64A3#code


I think this address might be the Radicle token not the Radicle governor

Will resolve in the PR where alpha tests are separated out to avoid rebasing multiple files

Was done in this commit in PR 14 .. 8f5e79e

wildmolasses

some comments / questions / nits, thanks @jferas !

wildmolasses · 2023-11-24T02:15:13Z

test/RadworksGovernor.t.sol

+    assertEq(_token.balanceOf(_receiver), _receiverTokenBalance, "Receiver balance is incorrect");
+  }
+
+  ////


nit: these section headings are formatted differently from one another. not sure if that's intentional.

Not intentional, done: 1eb42ac

test/RadworksGovernor.t.sol

wildmolasses · 2023-11-24T02:36:39Z

test/RadworksGovernor.t.sol

+    assertEq(TIMELOCK.balance, _timelockETHBalance - _amount);
+  }
+
+  // TODO: This test commented out as the timelock has no ETH as of 2023-11-08


related a bit to this TODO: the question had come up for me earlier, why not deal / vm.deal some ETH / tokens to addresses in relevant tests? not saying we should do it, just curious if there's a reason not to

test/RadworksGovernor.t.sol

wildmolasses · 2023-11-24T03:47:35Z

test/RadworksGovernor.t.sol

+    _jumpToActiveProposal(_newProposalId);
+
+    // Delegates vote with a mix of For/Against/Abstain with For winning.
+    // TODO: Need more delegates for this.. pool together had more!


just curious, can you elaborate on why we need more delegates?

We should be able to pull all of the "need more delegates" in a future PR, as we've been able to test with the delegates we have.

i'm confused why we are adding these comments if we don't need them! i won't block merge on it though.

wildmolasses

lgtm besides the comments thing, but that might just be my own misunderstanding. mergeable!

github-actions · 2023-12-07T15:39:13Z

Coverage after merging test-group-1 into initial-tests will be

76.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
RadworksGovernor.sol	73.91%	75%	69.23%	76%	101, 126–127, 135, 138, 152, 203, 84

This was referenced Nov 13, 2023

forge install: openzeppelin-contracts #1

Merged

Initial code for Radworks upgraded Governor #2

Merged

This was referenced Nov 13, 2023

Initial tests and architecture #5

Merged

Updated README.md file #12

Merged

Added GovernorVotesCompQuorumFraction for settable quorum #13

Closed

jferas marked this pull request as ready for review November 16, 2023 19:13

jferas force-pushed the test-group-1 branch from fa4b609 to f913957 Compare November 17, 2023 16:01

This was referenced Nov 20, 2023

Pulled alpha governor tests out into a separate suite #14

Merged

Implement efficient settable quorum without library #15

Closed

alexkeating reviewed Nov 21, 2023

View reviewed changes

wildmolasses reviewed Nov 24, 2023

View reviewed changes

jferas force-pushed the test-group-1 branch from aa49609 to 95a8d93 Compare November 27, 2023 16:09

jferas requested review from wildmolasses and alexkeating November 28, 2023 15:40

This was referenced Nov 28, 2023

Pauser and renounceAdmin Drips tests #18

Merged

Added transfer of RAD tokens to ScopeLift upon successful upgrade to governor Bravo #19

Merged

alexkeating approved these changes Dec 6, 2023

View reviewed changes

wildmolasses approved these changes Dec 7, 2023

View reviewed changes

jferas added 9 commits December 7, 2023 10:26

Added post-proposal Alpha tests and Bravo tests

4bdf42e

Added needed env var setup for CI testing

02a1d36

Updated CI fuzz runs to make fewer RPC requests

51335fc

Removed leftover console import

cf95cd7

Updated foundry.toml and ci.yml for fuzz seeding approach

b2a58a7

test cleanup

0ced31a

Added vm.deal of ETH test of timelock sending ETH can be done

0c5372f

Fix scopelint issue remove no-deal test

a682729

Added assumes to ensure gov settings being changed to new values

be445fa

jferas changed the base branch from initial-tests to main December 7, 2023 15:32

jferas force-pushed the test-group-1 branch from 3a921d8 to be445fa Compare December 7, 2023 15:32

jferas merged commit 07a8dee into main Dec 7, 2023
10 of 11 checks passed

This was referenced Dec 8, 2023

Added tests for changing admin/upgrading protocol via governance #21

Merged

Cleanup test names / scopelint fix #22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added post-proposal Alpha tests and Bravo tests #8

Added post-proposal Alpha tests and Bravo tests #8

jferas commented Nov 13, 2023

jferas commented Nov 13, 2023 •

edited

Loading

alexkeating left a comment

alexkeating Nov 17, 2023

jferas Nov 22, 2023

alexkeating Nov 17, 2023

jferas Nov 22, 2023

wildmolasses Nov 24, 2023

alexkeating Nov 27, 2023

jferas Nov 27, 2023 •

edited

Loading

alexkeating Nov 21, 2023

alexkeating Nov 21, 2023

alexkeating Nov 21, 2023

jferas Nov 22, 2023

alexkeating Nov 21, 2023

jferas Nov 22, 2023

jferas Nov 22, 2023

wildmolasses left a comment

wildmolasses Nov 24, 2023

jferas Nov 27, 2023

wildmolasses Nov 24, 2023

wildmolasses Nov 24, 2023

jferas Nov 28, 2023

wildmolasses Dec 7, 2023

wildmolasses left a comment

github-actions bot commented Dec 7, 2023

Added post-proposal Alpha tests and Bravo tests #8

Added post-proposal Alpha tests and Bravo tests #8

Conversation

jferas commented Nov 13, 2023

jferas commented Nov 13, 2023 • edited Loading

alexkeating left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jferas Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildmolasses left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildmolasses left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2023

jferas commented Nov 13, 2023 •

edited

Loading

jferas Nov 27, 2023 •

edited

Loading