Test governor upgrade for #5 #12

davidlaprade · 2023-01-12T21:33:26Z

This PR does a few things:

adds a fuzz seed to the .env template to reduce RPC calls and speed up local test runs
it renames some test variables to make it clearer which governor instance we're interacting with
it adds some tests:
- that proposals can be defeated by the new governor
- that miscellaneous token sends can be executed by the new governor
- that settings on the new governor can be updated
- that mixed votes (some for, some against) resolve correctly on the new governor
- that votes on the abandoned alpha governor don't affect votes on the active bravo governor
it adds RPC request caching persistence to CI

Create test harness for testing that the new Governor can execute proposals after the upgrade has completed. Write one test demonstrating the Governor's ability to send GTC already held by the timelock.

https://twitter.com/ultrasecreth/status/1603430600442101761/photo/1

mds1

still got a few tests left to review

.env.template

.github/workflows/ci.yml

mds1 · 2023-01-16T17:04:41Z

.github/workflows/ci.yml

+      - name: Cache fork requests
+        uses: actions/cache@v3
+        with:
+          path: ~/.foundry/cache
+          key: ${{ runner.os }}-foundry-network-fork-${{ github.sha }}
+          restore-keys: |
+            ${{ runner.os }}-foundry-network-fork-


This whole step using actions/cache@v3 shouldn't be needed, foundry-rs/foundry-toolchain@v1 now caches automatically

Hm, I removed this in d7b4570 and caching stopped working

For example, you can see that in this build that tests ran to completion, taking 9 minutes. I figured this was expected since we were using a new caching approach -- we'd need to re-cache everything from scratch. So after the build completed I immediately re-ran the workflow to see if things would be faster now. They weren't. I killed the next build after 3 minutes.

For comparison, prior to removing this explicit cache step, fork tests with cached RPC calls were finishing up in ~40s

Hmm yea that's odd. I just ran the same workflow twice:

https://github.com/gitcoinco/2022-Governor-upgrade/actions/runs/3934573618/jobs/6729444859

https://github.com/gitcoinco/2022-Governor-upgrade/actions/runs/3934573618/jobs/6729501579

Both took about 4 minutes. I wonder if this line missing logic to create the cache dir if it doesn't exist, so it's never getting saved the first time? Or perhaps we're doing something wrong. Gonna pull in @PaulRBerg to make sure we're not missing anything

Another test:

Pushed adffed2 which creates the cache directory manually in CI

CI ran here: https://github.com/gitcoinco/2022-Governor-upgrade/actions/runs/3940835577

Manually ran another after here: https://github.com/gitcoinco/2022-Governor-upgrade/actions/runs/3940835577

Both took about the same amount of time 🤔

Thanks for the tag, @mds1. Taking a look now.

There might be a bug in the cache implementation - sorry for that. I'm basing this guess on a number of other reports on Twitter.

However, I don't thin't think that this is about the if statement you linked to, i.e. this:

if (fs.existsSync(CACHE_PATHS[0]))

I added that in foundry-rs/foundry-toolchain#21 to prevent another bug that occurred when the user didn't have any fork tests.

~~I reverted back to the old approach. I'll leave this thread open for now in case Paul gets back to us, and will plan to merge without resolving.~~

EDIT: sorry -- didn't see your response above, Paul! I must have not refreshed the page. Appreciate the help!

No worries, David.

After a few hours of debugging (and hundreds of console logs), I have managed to fix this. Waiting for @mds1 or someone else from the team to review my PR and apply the fix:

foundry-rs/foundry-toolchain#24

I am quite confident that the caching should work as expected now!

test/GitcoinGovernor.t.sol

apbendi

A couple small comments, but overall really nice job @davidlaprade. Really put the new Governor through its paces. This is looking great!

apbendi · 2023-01-16T22:36:26Z

test/GitcoinGovernor.t.sol

+  }
+
+  function delegatesVoteOnBravoGovernor(uint256 _proposalId, uint8 _support) public {
+    require(_support < 3, "Invalid value for support");


Is a require the right thing to use here? Genuinely asking because I've never thought to use one in a test before, and I've generally opted for asserts instead, but off the top of my head I don't see a reason why require wouldn't work too.

See #12 (comment) — basically the test assertion helpers don't stop execution of a test when an assertion fails, but in this case (if support >= 3) something went wrong and we don't care about the rest of the test, so it'd just clutter output. In other words, we're using require because this is input verification, and not a test assertion

test/GitcoinGovernor.t.sol

apbendi · 2023-01-16T22:37:51Z

test/GitcoinGovernor.t.sol

+    assertEq(_state, IGovernor.ProposalState.Pending);
+  }
+
+  function _randomERC20Token(uint256 _seed) internal view returns (IERC20 _token) {


nit: Should we move this helper up above the the first test to keep helpers and tests separate?

Done in b69b4eb

test/GitcoinGovernor.t.sol

This reverts commit adffed2.

This reverts commit d7b4570.

mds1 · 2023-01-17T18:48:12Z

test/GitcoinGovernor.t.sol

+    // else. We also don't want the zero address as a receiver -- if our
+    // governor is sending tokens to the zero address it could just be due to a
+    // bug.


Nit, it is plausible a governor might want to burn tokens for some reason

Suggested change

// else. We also don't want the zero address as a receiver -- if our

// governor is sending tokens to the zero address it could just be due to a

// bug.

// else. We also don't want the zero address as a receiver -- although

// this is valid, it's typically not what a governor wants to do

Updated comment in 4216a16

apbendi and others added 10 commits January 12, 2023 11:26

Set up and run one end-to-end test with the new Governor

eac5a61

Create test harness for testing that the new Governor can execute proposals after the upgrade has completed. Write one test demonstrating the Governor's ability to send GTC already held by the timelock.

Add fuzz seed to .env template to reduce RPC calls

dd4b4d6

Generalize governorBravo token send test

98fb242

Test that proposals can be defeated for the new governor

d9069d1

passQueueAndExecuteUpgradeProposal --> upgradeToBravoGovernor

5dfd792

Test that settings can be updated by new governor

470d880

Test mixed voting results

ddd1c6a

Test that alpha votes don't affect bravo votes

8e60173

s/governor/governorBravo/g

659f149

scopelint

815c79b

davidlaprade marked this pull request as ready for review January 13, 2023 20:24

davidlaprade requested review from apbendi and mds1 January 13, 2023 20:24

davidlaprade mentioned this pull request Jan 13, 2023

Testing of the upgrade Governor #11

Closed

davidlaprade added 2 commits January 13, 2023 16:03

Cache fork requests

64f2a43

https://twitter.com/ultrasecreth/status/1603430600442101761/photo/1

Add a seed to fuzz deterministically in CI

f10554f

mds1 reviewed Jan 16, 2023

View reviewed changes

test/GitcoinGovernor.t.sol Outdated Show resolved Hide resolved

test/GitcoinGovernor.t.sol Show resolved Hide resolved

test/GitcoinGovernor.t.sol Outdated Show resolved Hide resolved

test/GitcoinGovernor.t.sol Show resolved Hide resolved

davidlaprade added 7 commits January 16, 2023 13:24

scopelint

b41f7ad

Troubleshoot CI

cce8185

Cycle CI fuzz seeds each week

bc88087

Bump fuzz tests in CI

4d58ea0

Remove fuzz seed from .env.template

629f1ca

Modify based on PR feedback

8d1ac76

Foundry toolchain caches fork requests for us

d7b4570

apbendi approved these changes Jan 16, 2023

View reviewed changes

mds1 reviewed Jan 16, 2023

View reviewed changes

test/GitcoinGovernor.t.sol Outdated Show resolved Hide resolved

mds1 and others added 3 commits January 17, 2023 07:38

ci: test if manually creating cache dir solves the issue

adffed2

Fix coverage workflow

3f56e3d

Fix coverage workflow

114ccca

davidlaprade added 4 commits January 17, 2023 11:38

Revert "ci: test if manually creating cache dir solves the issue"

751dc77

This reverts commit adffed2.

Revert "Foundry toolchain caches fork requests for us"

9a5c0f7

This reverts commit d7b4570.

Use cached fork requests on coverage

8f10d57

Modify based on PR feedback

b69b4eb

mds1 reviewed Jan 17, 2023

View reviewed changes

mds1 approved these changes Jan 17, 2023

View reviewed changes

Update comment

4216a16

davidlaprade merged commit 350ec8d into main Jan 17, 2023

PaulRBerg mentioned this pull request Jan 17, 2023

Critical fix: load home path with os.homedir foundry-rs/foundry-toolchain#24

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test governor upgrade for #5 #12

Test governor upgrade for #5 #12

davidlaprade commented Jan 12, 2023 •

edited

Loading

mds1 left a comment

mds1 Jan 16, 2023 •

edited

Loading

davidlaprade Jan 16, 2023

mds1 Jan 16, 2023

mds1 Jan 17, 2023

PaulRBerg Jan 17, 2023

davidlaprade Jan 17, 2023 •

edited

Loading

PaulRBerg Jan 17, 2023

apbendi left a comment

apbendi Jan 16, 2023

mds1 Jan 16, 2023

apbendi Jan 16, 2023

davidlaprade Jan 17, 2023

mds1 Jan 17, 2023 •

edited

Loading

davidlaprade Jan 17, 2023

Test governor upgrade for #5 #12

Test governor upgrade for #5 #12

Conversation

davidlaprade commented Jan 12, 2023 • edited Loading

mds1 left a comment

Choose a reason for hiding this comment

mds1 Jan 16, 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

davidlaprade Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apbendi 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

mds1 Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidlaprade commented Jan 12, 2023 •

edited

Loading

mds1 Jan 16, 2023 •

edited

Loading

davidlaprade Jan 17, 2023 •

edited

Loading

mds1 Jan 17, 2023 •

edited

Loading