chore: fork Foundry Forge #466

agostbiro · 2024-05-23T17:40:52Z

Adds a fork of Foundry's Forge as a library to the EDR workspace to serve as a basis of the Solidity tests features. As discussed, this is a clean fork, applying patches from upstream is not a prio. The forked commit is: https://github.com/foundry-rs/foundry/commits/0a5b22f07

Forge depends on a lot of crates within Foundry. These are included in this fork. The test suites for all these crates are included as well. Foundry Solidity build tools are necessary to run these tests, so these are also included.

At the linked commit, the Foundry repo has 123275 lines of Rust code, this fork has 56243 lines of Rust code. I've only changed the included Foundry code to pass our lints and disable flaky tests. I suspect that we can further remove a significant part of these 56k lines in the future.

The Foundry repo uses workspace dependencies so I added these to our root Cargo.toml. Unifying these dependencies with EDR crates is a follow up task. The Foundry repo had a lot of customized compilation flags in their root Cargo.toml, I ported these to our root manifest as well to speed up compilation.

changeset-bot · 2024-05-23T17:40:55Z

⚠️ No Changeset found

Latest commit: 75b1443

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

agostbiro · 2024-05-23T17:42:18Z

.cargo-husky/hooks/pre-commit

-# Setting RUSTFLAGS env for clippy makes it not include custom rules
-RUSTFLAGS=-Dwarnings cargo check --workspace --all-targets ${ALL_FEATURES}
-cargo clippy --all --all-targets ${ALL_FEATURES} -- -D warnings


I've removed the separate cargo check as Clippy is running it anyway and it was triggering recompilation for some reason.

agostbiro · 2024-05-23T17:43:27Z

.cargo/config.toml

@@ -28,7 +28,6 @@ rustflags = [
 "-Wclippy::float_cmp_const",
 "-Wclippy::fn_params_excessive_bools",
 "-Wclippy::from_iter_instead_of_collect",
- "-Wclippy::if-not-else",


I removed this lint as Foundry code has a pattern of checking for error conditions first with if !invariant { error } else { ... }. I think this is a reasonable pattern that we shouldn't prevent.

Why not just allow the pattern in the foundry crates?

We're planning to heavily modify the foundry crates, and it's not great if there are different coding standards for different parts of the code base + I don't think the pattern is actually harmful.

agostbiro · 2024-05-23T17:44:18Z

.gitignore

@@ -29,9 +29,6 @@ pids
 # Directory for instrumented libs generated by jscoverage/JSCover
 lib-cov

-# Coverage directory used by tools like istanbul
-coverage


Removed this as it excluded the coverage directory in the Foundry code and we don't use JS code coverage

.github/workflows/edr-ci.yml

Cargo.toml

crates/edr_napi/Cargo.toml

Cargo.toml

Wodann · 2024-05-23T18:52:17Z

It's hard for me to evaluate the side-effects of all of the changes to the workspace's Cargo.toml. Do you have a full understanding of that? Are there any things to be aware of?

agostbiro · 2024-05-24T09:13:13Z

It's hard for me to evaluate the side-effects of all of the changes to the workspace's Cargo.toml. Do you have a full understanding of that? Are there any things to be aware of?

Thanks for your review! The main thing to look out for is not to break edr_napi I think. This I've checked locally and the CI checks for the various supported platforms passed as well, so for me it's ok.

agostbiro · 2024-05-24T10:57:18Z

.gitignore

@@ -14,7 +14,6 @@ workspace.code-workspace
 # ignoring the node_modules part, as it'd ignore every node_modules, and we have some for testing

 # Logs
-logs


Removed this as it excluded the logs directory in Foundry test data. We can re-add it in subdirectories if it becomes necessary

agostbiro · 2024-05-24T11:14:53Z

.github/workflows/edr-ci.yml

+ - name: Cargo hack for EDR crates
+ run: |
+  chmod +x ./github/scripts/cargo-hack-edr.sh
+ ./github/scripts/cargo-hack-edr.sh
+ shell: bash


cargo-hack, the tool that makes sure that all feature combinations compile is failing for the Foundry crates. Since we aren't planning to publish them, this is ok, so I changed the CI to only run cargo-hack for EDR crates.

Update: #466 (comment)

agostbiro · 2024-05-24T17:28:56Z

I added the following changes to fix CI failures:

Only run cargo hack for EDR crates. This tool checks that the all the combinations of feature flags compile which is important if we release the crates, but it's not important otherwise. cargo hack fails for the forked Foundry crates and it's a lot of effort to fix it. Since we're planning to heavily modify these anyway and the failure doesn't cause any issues at this point I opted to disable cargo hack for Foundry crates.
Only run strict cargo doc checks for EDR crates and run the same more relaxed checks that Foundry runs in their CI for their crates. I started fixing the failures in the Foundry crates, but it's a lot of effort, so in the interest of time, I opted for this split approach.
Remove the asm-keccak feature option from the forge crate. This was a thorny one. We had keccak-asm as a dependency in our Cargo.lock before, but we didn't compile it, as it's an optional dependency of alloy/revm primitives. keccak-asm doesn't build on Windows with the (default) MSVC toolchain. This wasn't causing a problem in Foundry for the forge crate, but it broke the CI for us, because we run tests with --all-features.
Disable more flaky tests.

The PR is now ready for final review.

Wodann · 2024-05-24T18:08:56Z

This tool checks that the all the combinations of feature flags compile which is important if we release the crates, but it's not important otherwise.

I think this has effects beyond publishing. In my normal workflow I always test syntax with --all --no-default-features and --all --all-features when doing cargo check. If either of those would now start failing due to foundry having invalid syntax, that would disrupt my workflow as well.

Is that the case here?

Wodann

I left one new question based on updates

agostbiro · 2024-05-27T08:13:52Z

I think this has effects beyond publishing. In my normal workflow I always test syntax with --all --no-default-features and --all --all-features when doing cargo check. If either of those would now start failing due to foundry having invalid syntax, that would disrupt my workflow as well.

Right, --all --all-features is covered by clippy, but --all --no-default-features could've failed undetected.

I addressed this in 75b1443 by splitting up the cargo hack check and now we run cargo check --all --no-default-features for all crates and cargo hack check --feature-powerset --exclude-no-default-features --no-dev-deps for EDR crates only.

Wodann · 2024-05-27T14:12:00Z

Thanks!

Fork Foundry Forge

cbecb72

agostbiro had a problem deploying to github-action-benchmark May 23, 2024 17:40 — with GitHub Actions Error

agostbiro commented May 23, 2024

View reviewed changes

Temporarily disable cargo hack

033dd06

agostbiro marked this pull request as draft May 23, 2024 17:53

agostbiro requested review from Wodann and fvictorio May 23, 2024 17:53

agostbiro added the no changeset needed This PR doesn't require a changeset label May 23, 2024

agostbiro temporarily deployed to github-action-benchmark May 23, 2024 17:53 — with GitHub Actions Inactive

agostbiro commented May 23, 2024

View reviewed changes

.github/workflows/edr-ci.yml Outdated Show resolved Hide resolved

Wodann reviewed May 23, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

crates/edr_napi/Cargo.toml Show resolved Hide resolved

Cargo.toml Show resolved Hide resolved

Wodann assigned agostbiro May 23, 2024

Remove unnecessary release flags

e9a745b

agostbiro temporarily deployed to github-action-benchmark May 24, 2024 09:11 — with GitHub Actions Inactive

Commit logs testdata

6864298

agostbiro had a problem deploying to github-action-benchmark May 24, 2024 10:52 — with GitHub Actions Error

agostbiro commented May 24, 2024

View reviewed changes

agostbiro had a problem deploying to github-action-benchmark May 24, 2024 11:13 — with GitHub Actions Error

agostbiro commented May 24, 2024

View reviewed changes

Execute cargo hack for EDR crates

6545adb

agostbiro force-pushed the fork-forge branch from 560a976 to 6545adb Compare May 24, 2024 11:17

agostbiro temporarily deployed to github-action-benchmark May 24, 2024 11:18 — with GitHub Actions Inactive

Different doc checks for EDR and Foundry crates

52995fa

agostbiro temporarily deployed to github-action-benchmark May 24, 2024 12:31 — with GitHub Actions Inactive

Remove asm-keccak feature

864386f

agostbiro temporarily deployed to github-action-benchmark May 24, 2024 15:53 — with GitHub Actions Inactive

Disable more flaky tests

b6035fd

agostbiro temporarily deployed to github-action-benchmark May 24, 2024 16:28 — with GitHub Actions Inactive

agostbiro requested a review from Wodann May 24, 2024 17:29

agostbiro marked this pull request as ready for review May 24, 2024 17:29

Wodann reviewed May 24, 2024

View reviewed changes

fvictorio mentioned this pull request May 25, 2024

Update crates/README.md #472

Merged

Run no-default-features checks for all crates

75b1443

agostbiro temporarily deployed to github-action-benchmark May 27, 2024 08:10 — with GitHub Actions Inactive

agostbiro requested a review from Wodann May 27, 2024 08:13

Wodann approved these changes May 27, 2024

View reviewed changes

agostbiro merged commit 318a25e into feat/solidity-tests May 27, 2024
31 checks passed

agostbiro deleted the fork-forge branch May 27, 2024 14:28

github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fork Foundry Forge #466

chore: fork Foundry Forge #466

agostbiro commented May 23, 2024 •

edited

Loading

changeset-bot bot commented May 23, 2024 •

edited

Loading

agostbiro May 23, 2024

agostbiro May 23, 2024

Wodann May 23, 2024

agostbiro May 24, 2024

agostbiro May 23, 2024

Wodann commented May 23, 2024

agostbiro commented May 24, 2024

agostbiro May 24, 2024

agostbiro May 24, 2024

agostbiro May 27, 2024

agostbiro commented May 24, 2024 •

edited

Loading

Wodann commented May 24, 2024

Wodann left a comment

agostbiro commented May 27, 2024

Wodann commented May 27, 2024

chore: fork Foundry Forge #466

chore: fork Foundry Forge #466

Conversation

agostbiro commented May 23, 2024 • edited Loading

changeset-bot bot commented May 23, 2024 • edited Loading

⚠️ No Changeset found

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

Wodann commented May 23, 2024

agostbiro commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agostbiro commented May 24, 2024 • edited Loading

Wodann commented May 24, 2024

Wodann left a comment

Choose a reason for hiding this comment

agostbiro commented May 27, 2024

Wodann commented May 27, 2024

agostbiro commented May 23, 2024 •

edited

Loading

changeset-bot bot commented May 23, 2024 •

edited

Loading

agostbiro commented May 24, 2024 •

edited

Loading