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.

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

refactor: decouple Foundry project layout and build system #572

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

agostbiro
Copy link
Member

@agostbiro agostbiro commented Jul 31, 2024

Goal

Decouple the Solidity test runner from the project layout and build system and enable support for functionality that depends on compilation artifacts in the JS runner. Decoupling the project layout and build system is not only important to remove the dependence on Foundry’s format, but also to give flexibility to iterate on Hardhat's build system.

Background

The Solidity test runner needs access to Solidity compilation artifacts for the following functionality:

  • Executing tests.
    • The bytecode and JSON ABI for test suite contracts is currently passed through the TypeScript API. For large projects this can become a performance bottleneck.
  • The getCode and getDeployedCode cheatcodes.
    • These were originally intended for deployment scripting which we don’t want to support, but they are also being used for mocking which we do want to support.
    • Foundry’s implementation primarily reads artifacts from memory, but it can read from disk under certain conditions.
    • getCode and getDeployedCode are currently no-op when Solidity tests are executed through from JS.
  • Invariant testing targets
    • Needed to access the ABI for the contracts that will be called over the course of a given invariant test fuzzing campaign.
    • This is currently no-op when Solidity tests are executed through from JS.

Solution

This PR brings the following changes:

  • Pass all the compilation artifacts through the TS API, not just the test suite contracts.
  • In addition, pass the test suite artifact ids so that EDR knows which artifacts are test suites to execute.
  • As discussed, drop support for the following Foundry functionality:
    • auto-linking libraries (out of scope)
    • loading artifacts from disk for getCode and getDeployedCode cheatcodes as a fallback behavior (rarely used)
    • inline configuration in Solidity source files (rarely used)

Alternatives considered

An alternative approach to achieve the same goal would be to provide a callback to EDR from JS that EDR can call to query artifacts. This approach was tested in a spike, but it was discarded, because there are several places in the Foundry codebase where the Rust code will iterate over all the artifacts which will be very slow if that involves fetching all the artifacts from JS.

In the future, once the new Hardhat build system has stabilized, we could move file loading over to Rust to address potential performance problems from passing a large amount of data through the ffi.

Notes

I tried to minimize changes to interfaces to make this PR easy to review. The MultiContractRunner is especially a candidate for future refactoring after this change.

I manually benchmarked performance and found no regression.

Copy link

changeset-bot bot commented Jul 31, 2024

⚠️ No Changeset found

Latest commit: 8ade557

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 agostbiro requested a review from fvictorio July 31, 2024 16:28
@agostbiro agostbiro temporarily deployed to github-action-benchmark July 31, 2024 16:28 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark July 31, 2024 16:28 — with GitHub Actions Inactive
@agostbiro agostbiro requested a review from Wodann July 31, 2024 16:28
@agostbiro agostbiro self-assigned this Jul 31, 2024
@agostbiro agostbiro added the no changeset needed This PR doesn't require a changeset label Jul 31, 2024
Copy link
Member

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just some small suggestions and questions

crates/edr_napi/src/solidity_tests.rs Outdated Show resolved Hide resolved
crates/foundry/common/src/contracts.rs Outdated Show resolved Hide resolved
crates/foundry/forge/src/lib.rs Outdated Show resolved Hide resolved
crates/tools/js/benchmark/solidity-tests.js Show resolved Hide resolved
@agostbiro agostbiro temporarily deployed to github-action-benchmark August 1, 2024 08:46 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark August 1, 2024 08:46 — with GitHub Actions Inactive
Copy link
Member

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

I only checked the js/ts changes. LGTM.

@agostbiro agostbiro merged commit 9732177 into feat/solidity-tests Aug 1, 2024
42 checks passed
@agostbiro agostbiro deleted the refactor/sol-artifacts-from-js branch August 1, 2024 14:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no changeset needed This PR doesn't require a changeset
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Decouple Foundry build and project format from JS Solidity test runner
3 participants