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.

Sign up for GitHub

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

Test governor upgrade for #5 #12

Merged
merged 27 commits into from
Jan 17, 2023
Merged

Test governor upgrade for #5 #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
eac5a61
Set up and run one end-to-end test with the new Governor
apbendi Dec 19, 2022
dd4b4d6
Add fuzz seed to .env template to reduce RPC calls
davidlaprade Jan 12, 2023
98fb242
Generalize governorBravo token send test
davidlaprade Jan 12, 2023
d9069d1
Test that proposals can be defeated for the new governor
davidlaprade Jan 12, 2023
5dfd792
passQueueAndExecuteUpgradeProposal --> upgradeToBravoGovernor
davidlaprade Jan 12, 2023
470d880
Test that settings can be updated by new governor
davidlaprade Jan 12, 2023
ddd1c6a
Test mixed voting results
davidlaprade Jan 12, 2023
8e60173
Test that alpha votes don't affect bravo votes
davidlaprade Jan 13, 2023
659f149
s/governor/governorBravo/g
davidlaprade Jan 13, 2023
815c79b
scopelint
davidlaprade Jan 13, 2023
64f2a43
Cache fork requests
davidlaprade Jan 13, 2023
f10554f
Add a seed to fuzz deterministically in CI
davidlaprade Jan 13, 2023
b41f7ad
scopelint
davidlaprade Jan 16, 2023
cce8185
Troubleshoot CI
davidlaprade Jan 16, 2023
bc88087
Cycle CI fuzz seeds each week
davidlaprade Jan 16, 2023
4d58ea0
Bump fuzz tests in CI
davidlaprade Jan 16, 2023
629f1ca
Remove fuzz seed from .env.template
davidlaprade Jan 16, 2023
8d1ac76
Modify based on PR feedback
davidlaprade Jan 16, 2023
d7b4570
Foundry toolchain caches fork requests for us
davidlaprade Jan 16, 2023
adffed2
ci: test if manually creating cache dir solves the issue
mds1 Jan 17, 2023
3f56e3d
Fix coverage workflow
davidlaprade Jan 17, 2023
114ccca
Fix coverage workflow
davidlaprade Jan 17, 2023
751dc77
Revert "ci: test if manually creating cache dir solves the issue"
davidlaprade Jan 17, 2023
9a5c0f7
Revert "Foundry toolchain caches fork requests for us"
davidlaprade Jan 17, 2023
8f10d57
Use cached fork requests on coverage
davidlaprade Jan 17, 2023
b69b4eb
Modify based on PR feedback
davidlaprade Jan 17, 2023
4216a16
Update comment
davidlaprade Jan 17, 2023
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
34 changes: 34 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,29 @@ jobs:
with:
version: nightly

- 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-
Comment on lines +43 to +49
Copy link
Collaborator

@mds1 mds1 Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. https://github.com/gitcoinco/2022-Governor-upgrade/actions/runs/3934573618/jobs/6729444859
  2. 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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another test:

  1. Pushed adffed2 which creates the cache directory manually in CI
  2. CI ran here: https://github.com/gitcoinco/2022-Governor-upgrade/actions/runs/3940835577
  3. Manually ran another after here: https://github.com/gitcoinco/2022-Governor-upgrade/actions/runs/3940835577

Both took about the same amount of time 🤔

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@davidlaprade davidlaprade Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


# https://twitter.com/PaulRBerg/status/1611116650664796166
- name: Generate fuzz seed with 1 week TTL
run: >
echo "FOUNDRY_FUZZ_SEED=$(
echo $(($EPOCHSECONDS - $EPOCHSECONDS % 604800))
)" >> $GITHUB_ENV

- name: Run tests
run: forge test

coverage:
env:
DEPLOYER_PRIVATE_KEY: ${{ secrets.DEPLOYER_PRIVATE_KEY }}
MAINNET_RPC_URL: ${{ secrets.MAINNET_RPC_URL }}
PROPOSER_PRIVATE_KEY: ${{ secrets.PROPOSER_PRIVATE_KEY }}
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
Expand All @@ -53,6 +72,21 @@ jobs:
with:
version: nightly

- 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-

# https://twitter.com/PaulRBerg/status/1611116650664796166
- name: Recycle the fuzz seed from the test run
run: >
echo "FOUNDRY_FUZZ_SEED=$(
echo $(($EPOCHSECONDS - $EPOCHSECONDS % 604800))
)" >> $GITHUB_ENV

- name: Run coverage
run: forge coverage --report summary --report lcov

Expand Down
6 changes: 3 additions & 3 deletions foundry.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
verbosity = 3

[profile.ci]
fuzz = { runs = 5000 }
invariant = { runs = 1000 }
fuzz = { runs = 500 }
invariant = { runs = 500 }

[profile.lite]
fuzz = { runs = 50 }
fuzz = { runs = 50, seed = 1673481600 }
invariant = { runs = 10 }
# Speed up compilation and tests during development.
optimizer = false
Expand Down
12 changes: 6 additions & 6 deletions src/GitcoinGovernor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ contract GitcoinGovernor is
public
view
virtual
override (Governor, GovernorTimelockCompound)
override(Governor, GovernorTimelockCompound)
returns (bool)
{
return GovernorTimelockCompound.supportsInterface(interfaceId);
Expand All @@ -65,7 +65,7 @@ contract GitcoinGovernor is
public
view
virtual
override (Governor, GovernorSettings)
override(Governor, GovernorSettings)
returns (uint256)
{
return GovernorSettings.proposalThreshold();
Expand All @@ -76,7 +76,7 @@ contract GitcoinGovernor is
public
view
virtual
override (Governor, GovernorTimelockCompound)
override(Governor, GovernorTimelockCompound)
returns (ProposalState)
{
return GovernorTimelockCompound.state(proposalId);
Expand All @@ -96,7 +96,7 @@ contract GitcoinGovernor is
uint256[] memory values,
bytes[] memory calldatas,
bytes32 descriptionHash
) internal virtual override (Governor, GovernorTimelockCompound) {
) internal virtual override(Governor, GovernorTimelockCompound) {
return
GovernorTimelockCompound._execute(proposalId, targets, values, calldatas, descriptionHash);
}
Expand All @@ -107,7 +107,7 @@ contract GitcoinGovernor is
uint256[] memory values,
bytes[] memory calldatas,
bytes32 descriptionHash
) internal virtual override (Governor, GovernorTimelockCompound) returns (uint256) {
) internal virtual override(Governor, GovernorTimelockCompound) returns (uint256) {
return GovernorTimelockCompound._cancel(targets, values, calldatas, descriptionHash);
}

Expand All @@ -116,7 +116,7 @@ contract GitcoinGovernor is
internal
view
virtual
override (Governor, GovernorTimelockCompound)
override(Governor, GovernorTimelockCompound)
returns (address)
{
return GovernorTimelockCompound._executor();
Expand Down
Loading