Conversation
🦋 Changeset detectedLatest commit: 00e080f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end scenario intended to reproduce inline-config parsing failures on very large Solidity build-info outputs (base-contracts), along with harness support needed to run it (pnpm + Foundry in the devcontainer).
Changes:
- Add Foundry installation to the devcontainer setup script.
- Extend end-to-end scenario schema/types/validation to support
pnpm(including tests). - Add a new
end-to-end/base-contractsscenario with a preinstall script that pulls required Foundry deps.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/setup.sh |
Installs Foundry (forge) during devcontainer creation to support Foundry-based scenarios. |
scripts/end-to-end/types.ts |
Adds pnpm as an allowed scenario package manager type. |
scripts/end-to-end/schema/scenario.schema.json |
Updates JSON schema enum to allow pnpm. |
scripts/end-to-end/schema/scenario-schema.ts |
Updates runtime validation to accept pnpm. |
scripts/end-to-end/schema/scenario-schema.test.ts |
Adds a test covering pnpm; updates unsupported-PM test value. |
end-to-end/base-contracts/scenario.json |
Introduces a new external-repo scenario for base-contracts using pnpm. |
end-to-end/base-contracts/preinstall.sh |
Preinstall script that installs Foundry dependencies and checks out specific library versions. |
| const outputJsonString = JSON.stringify(outputJson); | ||
| const buildInfoOutputPath = join(tmpdir(), `${randomUUID()}.json`); | ||
| writeFileSync(buildInfoOutputPath, outputJsonString); | ||
|
|
There was a problem hiding this comment.
makeBuildInfo writes a JSON file into the OS temp directory but nothing deletes it after the test run. Over time (or with repeated CI retries) this can accumulate temp files and waste disk space; consider deleting these files in the tests (e.g., track returned buildInfoOutputPaths and remove them in an after/afterEach hook) or generate them in a per-test temp dir that is cleaned up automatically.
The base/contracts repo uses pnpm as the package manager, to support pulling this in as a scenario we are adding `pnpm` as a package manager choice in scenario files.
eff588f to
3b16f4d
Compare
3b16f4d to
00e080f
Compare
| const buildInfoOutput = await readJsonFileAsStream<SolidityBuildInfoOutput>( | ||
| buildInfoAndOutput.buildInfoOutputPath, | ||
| ); |
There was a problem hiding this comment.
collectRawOverrides re-reads buildInfoOutputPath from disk even though getBuildInfosAndOutputs already loaded output into memory. For large build-info outputs this doubles I/O and can noticeably slow down test startup. Consider parsing from the already-loaded Uint8Array using a streaming JSON parser (e.g., piping a Readable.from(buildInfoAndOutput.output) into the same JSONParser used by readJsonFileAsStream), or refactoring getBuildInfosAndOutputs to avoid eagerly reading output when only inline-config extraction needs it.
| npm install -g bun | ||
|
|
||
| # Install forge | ||
| curl -L https://foundry.paradigm.xyz | bash |
There was a problem hiding this comment.
The Foundry install step uses curl -L ... | bash without failing on HTTP errors. Using curl -fsSL (or at least -f) will make the setup fail fast if the download fails (e.g., transient network issues or a non-200 response) instead of piping an error page into bash.
| curl -L https://foundry.paradigm.xyz | bash | |
| curl -fsSL https://foundry.paradigm.xyz | bash |
|
Some important datapoint: this doesn't happen anymore in HH 3.4.0. |
This PR reproduces the issue of inline config parsing failing on large enough build infos.
To run and end to end test that reproduces the failure based on our fork of base contracts (https://github.com/popescuoctavian/base-contracts/tree/migrate-to-HH3), then:
This will, eventually generate this output:
Approach
First this PR replicates the issue with an end to end scenario by:
./end-to-end/base-contractsscenario pointing at our fork where we found the issue (i.e.popescuoctavian/base-contracts)Second this PR demonstrates one approach to the fix, implemented by Claude:
This does resolve the immediate error from
JSON.parseat the cost of a second read from the build info file.Fixes #8117.