Remove the dead gas-snapshot apparatus - #121
Conversation
No snapshot is taken anywhere: neither this repo's workflows nor the rainix reusables they call run `forge snapshot`. The tracked `.gas-snapshot` was 0 bytes, and `REUSE.toml` and `.soldeerignore` each carried an entry to service it. `foundry.toml` kept a commented-out optimizer block whose rationale describes settings that are not in force, under a comment claiming the settings that are in force exist for snapshots. The settings stay -- they determine the bytecode hash pinned in `src/generated/` -- and the comment now says that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The claim it made is false for this repo: `src/generated/CodeGennable.sol` is generated from an empty contract, and regenerating under `optimizer_runs = 200` and again under `optimizer = false` leaves the pinned `BYTECODE_HASH` unchanged. The settings stand on their own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Closes #84
The call: remove, not start snapshotting
#84 offers two routes. Both ledgers, measured on this clone with the flake toolchain:
Route A — remove (taken). Build cost: one deleted file, two config lines, six
comment lines; net −10 lines, no additions. Carrying cost after: zero, nothing left
to keep in sync. Cost to reinstate:
forge snapshotregenerates the whole file inone command — measured at 13054 bytes / 134 lines, and byte-identical across two
runs — and the two config entries are one line each. Nothing authored is lost: the
blob has been 0 bytes since
88f5ac6(2025-10-21), and the last commit that left itnon-empty (
4b3725f, 2024-10-19, 206 bytes) recordedLibBloomTestruns — a testthat does not exist in this repo — so the file was already describing a different
tree the last time it held anything.
Route B — take a snapshot in CI (rejected). Three costs, in order of weight:
.github/workflows/areone-line calls into rainix reusables, and shared CI belongs in rainix rather
than in a consumer. A snapshot gate is therefore a
rainlanguage/rainixchangelanding on every consumer repo, plus a committed snapshot in each — not a change
this issue can make.
LibCodeGen,LibFsandLibHexStringall importforge-std/Vm.soland take aVmas a parameter: this repo is build-timetooling, and none of it is deployed. The 134 numbers a snapshot would record are
the gas of cheatcode calls (
vm.toString,vm.writeFile,vm.readFileBinary)inside a test harness, which no consumer pays on chain. The runtime-gas artifact
this repo exists to produce is the generated constant cache in a consumer's
repo, and its gas is measurable there.
that touches any library — a conflict surface across concurrent PRs — and a
doubled test run in CI.
One thing that is not a reason to reject Route B, contrary to what fuzz-heavy
[fuzz] runs = 2048suggests: the gate would be stable.forge snapshotfixes thefuzz seed, so two runs produced byte-identical files and
forge snapshot --checkagainst one of them exited 0, while plain
forge testre-seeds every run (sametest, three runs: μ 312478 / 306992 / 304937). Route B is rejected for cost and
zero signal, not for flakiness.
What changed
.gas-snapshot.REUSE.toml: dropped the.gas-snapshotannotation..soldeerignore: dropped the.gas-snapshotentry.foundry.toml: dropped the commented-outvia_ir/optimizer/optimizer_runs/optimizer_stepsblock and the "These settings should be used for snapshots"line.
optimizer = true/optimizer_runs = 1000000stay — they are the in-forcecompile settings and were never part of the snapshot apparatus except in the
comment's claim.
Where the issue was wrong
The issue states that
REUSE.toml:5"lists.gas-snapshotamong the annotatedpaths so
reuse lintpasses on a file with no header". Measured, that is not trueat the state the repo is in:
reuseskips zero-byte files, so with the annotationdeleted and the empty file still present,
reuse lintpasses 47/47 (B3 below). Theannotation only becomes load-bearing once the file has content (B3b/B3c). The
.soldeerignoreentry, by contrast, is live today: deleting it alone ships the0-byte file to every consumer (B2). Same fix either way; different account of which
of the two entries was doing anything.
The issue names four locations. There is a fifth reference,
.audit/scope.json:5,which lists
.gas-snapshotin the audited file set for commit7aa85a4. Leftuntouched: it records what that audit run covered, and editing it would falsify the
record rather than clean anything up.
Why no new
forge testThis change removes configuration, not code, so
forge testhas no visibility intoany of it — the removed entries are consumed by
reuse lint(rainix-sol-legal) andby
forge soldeer push(rainix-autopublish) respectively, and both of those gatesalready run in CI. The check ran below with a positive control on each.
A Solidity test asserting
.gas-snapshotis absent would need{ access = "read", path = ".gas-snapshot" }added tofs_permissions, or a readgrant on the repo root. The first is a new config entry servicing a removed file —
literally the defect being fixed. The second widens the test profile's filesystem
reach in the one repo whose libraries exist to confine generated paths. Neither is
worth it for a hygiene removal already covered by two live CI gates.
QA
forge test— the diff is config-only andforge testcannot see any of it (reasoning above). The discriminating checks are the two live CI gates over the removed entries, each run red-then-green:reuse lint(red at B3b —exit 1,.gas-snapshotnamed missing; green at A1 — 47/47) andforge soldeer push --dry-run(red at B2 — the 0-byte file shipped, 21 → 22 entries; green at A2 — 21 entries, file absent). Plusforge script script/Build.sol+git diff --exit-code src/generated, red at A6c and green at A5..soldeerignore:2deleted alone (file kept) -> killed byforge soldeer push --dry-run, package 21 → 22 files.REUSE.toml:6deleted alone with a non-empty.gas-snapshot-> killed byreuse lint, exit 1, 47/48.REUSE.toml:6deleted alone with the 0-byte file -> survived (reuseskips empty files) — reported above as a correction to the issue, not a gap.foundry.tomloptimizer_runs = 1000000->200, andoptimizer = true->false-> both survived the regenerate-and-compare gate, which falsified the replacement comment this branch first wrote; the comment was deleted indb22ba7rather than the check weakened.test/concrete/CodeGennable.solcontract CodeGennable {}-> addfunction f()-> killed by the drift gate,BYTECODE_HASH0x8016e5…a84d→0x7b482d…c4a7, which is the positive control proving that gate is not dead.reuse lint's own file accounting (47 vs 48 files carrying license info), and the actual zipforge soldeer push --dry-runhands to consumers — both external to this repo's code. For Route B's ledger,git log/git cat-file -son.gas-snapshot's three commits, andgit grepoverrainlanguage/rainix@7f223b4's workflows, where everysnapshothit is a per-tag deploy-pin snapshot undersrc/generated/<tag>/and none isforge snapshot.foundry.toml:14-23,.gas-snapshot,REUSE.toml:5,.soldeerignore:2under a stated decision between removal and snapshotting; all four are covered, the decision is ledgered above, and a fifth reference the issue did not name (.audit/scope.json:5) is identified and deliberately left as an audit record.REUSE.toml:17is left to REUSE.toml redundantly annotates src/lib/LibCodeGen.sol, which already carries its own SPDX header #85.Everything below was run in this clone via
nix develop -c, foundry from the flake.Before the fix — each removed entry probed for whether it was load-bearing:
main's treeforge soldeer push --dry-run.gas-snapshotabsent.soldeerignoreentry only, keep the fileforge soldeer push --dry-run0 01-01-1980 00:00 .gas-snapshotnow shippedREUSE.tomlentry only, keep the empty filereuse lintreuseskips zero-byte filesREUSE.tomlentry, file made non-emptyreuse lint# MISSING COPYRIGHT AND LICENSING INFORMATION/* .gas-snapshot, 47/48REUSE.tomlentry, file non-emptyreuse lintB2's red is the failure this PR has to avoid: removing the
.soldeerignoreline onits own would have published a 0-byte
.gas-snapshotto every consumer ofrain-sol-codegen. The file and both entries have to go together, which is what thediff does.
After the fix, at
db22ba7:The package went 73093 → 73074 bytes at the same 21 files: the two deleted config
lines, and nothing else consumers receive.
Mutation matrix — every check above broken on purpose to prove it can fail:
.soldeerignoreentry removed, file kept.gas-snapshotshipped ✅REUSE.tomlentry removed, file non-emptyreuse lintred.gas-snapshotnamed as missing ✅optimizer_runs1000000 → 200, regenerateoptimizer = false, regeneratefunction f() external pure returns (uint256)totest/concrete/CodeGennable.sol, regenerateBYTECODE_HASH0x8016e5…a84d→0x7b482d…c4a7✅A6/A6b are a survived mutant, and they changed the diff. The first version of this
branch replaced the deleted comment with one asserting that the optimizer settings
determine the
BYTECODE_HASHpinned insrc/generated/. Regenerating underoptimizer_runs = 200, and again underoptimizer = false, leftsrc/generated/CodeGennable.solbyte-identical — because it is generated fromcontract CodeGennable {}, an empty contract whose runtime code is the same underany optimizer setting. The claim was false, so the replacement comment was removed
in
db22ba7and the settings are left to stand on their own. A6c is the positivecontrol that keeps A5's "no drift" from being a dead harness: the regenerate-and-
compare gate does go red when the input actually changes, and the tree restored
clean afterwards.
Scope left to #85:
REUSE.toml:18, the redundantsrc/lib/LibCodeGen.solannotation, is untouched here. This PR only removes line 6 of the same list.