Skip to content

Remove dead LibSnapshot - #33

Merged
thedavidmeister merged 1 commit into
mainfrom
2026-08-13-remove-dead-libsnapshot
Aug 13, 2026
Merged

Remove dead LibSnapshot#33
thedavidmeister merged 1 commit into
mainfrom
2026-08-13-remove-dead-libsnapshot

Conversation

@thedavidmeister

@thedavidmeisterthedavidmeister commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

LibSnapshot is dead code. Removing it, with its test and the one reference to
it from LibFs.t.sol.

It has no callers

Not "no callers we need to worry about" — none at all:

  • an org-wide code search finds LibSnapshot only inside this repo (the library
    plus its own test, plus the one cross-check in test/lib/LibFs.t.sol)
  • every consumer pins rain-sol-codegen at 0.1.0, which predates it. It
    first shipped in sol-v0.1.1.

So there is nothing to sequence and no migration window to protect. This PR can
merge in any order relative to anything else.

Why it is dead rather than merely unused

It is release machinery, not code generation. "Which release am I building",
"where does its record live" and "freeze it immutably" are the deploy lifecycle;
emitting a Solidity constant is codegen. Splitting those across two repos is why
nothing adopted it — the repo that needed it could not get a whole lifecycle
from here.

All of it now lives in rain.deploy as LibRainDeploySnapshot
(rainlanguage/rain.deploy#26), beside the deploy library it belongs with, where
it also gained the guards a release actually needs: strict X.Y.Z, refusing to
re-cut a frozen release, refusing to freeze nothing, and a freeze that takes
the regeneration as an argument so "freeze, then regenerate" has nowhere to be
written.

LibCodeGen, LibFs and LibHexString are untouched and remain this repo's
subject.

The LibFs cross-check

testFrozenPathTracksGeneratedFilename asserted that LibSnapshot's frozen path
tracked LibFs.pathForContract. That coupling no longer exists here — and in
rain.deploy it cannot drift, because pathForSnapshot delegates to
LibFs.pathForContract rather than restating it. testPathForContract stays and
still pins the generated-file location, which is the part consumers depend on.

Publishing

Removing a file changes the published package, so merging bumps this repo's
version via autopublish. That is expected: nothing consumes the removed symbols
on any published version anyone pins.

QA

  • Discriminating tests: forge test — 9 tests across 4 suites, all pass after
    the removal; forge fmt --check clean. The removed test only exercised
    removed code, so no remaining test loses coverage, and testPathForContract
    still pins the src/generated/<name>.sol location consumers commit and
    import. Verified by running the suite on this branch.
  • Mutations applied: n/a — a pure deletion has no surviving line to mutate. The
    equivalent evidence is the caller search: were any caller to exist, the
    removal would fail compilation rather than a test, and compilation passes.
  • Oracle: the callers, not the code. A symbol with no callers on any published
    version anyone pins is dead by definition; the org-wide search plus the pinned
    version of every consumer (0.1.0, which predates the symbol) is the evidence,
    and neither is derived from this repo's own tests.
  • Category check: the ask is "remove the dead code", and the category is every
    declaration of and reference to LibSnapshot — the library, its test, and the
    LibFs.t.sol cross-check — not just the library file. Verified by grep: no
    LibSnapshot string survives anywhere in the repo.

Summary by CodeRabbit

  • Refactor
    • Removed obsolete snapshot management functionality, including release-tag parsing, snapshot path generation, and generated-file freezing.
    • Removed related snapshot test coverage and unused test references.

It has no callers anywhere: an org-wide search finds it only in this repo, and
every consumer pins rain-sol-codegen 0.1.0, which predates it.
It was release machinery in a codegen library — 'which release am I building',
'where does its record live', 'freeze it immutably' are the deploy lifecycle,
not code generation. That split is why nothing adopted it. The whole of it now
lives in rain.deploy as LibRainDeploySnapshot, beside the deploy library it
belongs with.
LibFs.t.sol's cross-check on the frozen path goes with it; that coupling cannot
drift in its new home, where pathForSnapshot delegates to LibFs.pathForContract
rather than restating it.
@thedavidmeisterthedavidmeister self-assigned this Aug 13, 2026
@coderabbitai

coderabbitaiBot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8d84d60a-182d-444c-8da7-307316f553a6

📥 Commits

Reviewing files that changed from the base of the PR and between 0e8a3aa and 49ffe9f.

📒 Files selected for processing (3)
  • src/lib/LibSnapshot.sol
  • test/lib/LibFs.t.sol
  • test/lib/LibSnapshot.t.sol
💤 Files with no reviewable changes (3)
  • src/lib/LibSnapshot.sol
  • test/lib/LibSnapshot.t.sol
  • test/lib/LibFs.t.sol

Walkthrough

The change removes LibSnapshot.sol, its snapshot path and freeze logic, and all associated tests. LibFs.t.sol no longer imports LibSnapshot or checks frozen snapshot paths.

Changes

Snapshot removal

Layer / File(s)Summary
Remove snapshot library and test coverage
src/lib/LibSnapshot.sol, test/lib/LibSnapshot.t.sol, test/lib/LibFs.t.sol
Removes snapshot parsing, path construction, freezing logic, lifecycle tests, and the related frozen-path assertion. LibFs.t.sol also removes the unused import.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score:⚪ Minimal · up to 49ffe

This localized cleanup removes unused code and its related tests without changing supported functionality; no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title clearly and concisely describes the main change: removing the unused LibSnapshot library.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2026-08-13-remove-dead-libsnapshot

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thedavidmeister

Copy link
Copy Markdown
ContributorAuthor

Reviewed 49ffe9f: APPROVE — pure deletion, 191 lines removed across three files with nothing added: src/lib/LibSnapshot.sol (-94), test/lib/LibSnapshot.t.sol (-84), and the cross-check in test/lib/LibFs.t.sol (-13). All five checks green, zero unresolved review threads.

Safe because it has no callers, verified rather than assumed: an org-wide code search for LibSnapshot finds it only inside this repo, and every consumer pins rain-sol-codegen at 0.1.0 — which predates it, since it first shipped in sol-v0.1.1. So no published version anyone consumes contains the removed symbols.

The functionality is not lost. It moved to rainlanguage/rain.deploy as src/lib/LibRainDeploySnapshot.sol on rain.deploy#26, on the grounds that it is release machinery rather than codegen: it reads [package].version, sequences a release, and enforces append-only immutability of a DEPLOYMENT record, while LibCodeGen/LibFs turn artifacts into source. Splitting those across two repos was the homing defect; this completes the move.

Expected on merge: the deletion changes published package content, so autopublish bumps rain-sol-codegen. Nothing consumes the removed symbols, so that bump breaks no consumer.

Rulings-conformance:

  • "don't leave dead code lying around" (human, this work) — OBEYED, and this PR exists solely because of it. I had instructed the agent to REPORT the dead copy rather than remove it; that was wrong and this is the correction.
  • "if it's dead code not used then we don't need to sequence it, by your own logic" (human, this work) — OBEYED. I had told the agent to land rain.deploy#26 first so nothing was "dead in between", which was incoherent given zero callers. This PR's body states no callers as its justification and explicitly says it can merge in any order relative to anything else. No sequencing claim is made or relied on.
  • "we are currently now building the canonical deploy and release process… whatever else is out there is mostly irrelevant as it will be replaced" (human, this work) — OBEYED. The canonical home is rain.deploy; this removes the superseded copy rather than reconciling two.
  • "if we can do things in sol then that's great" (human, this work) — CONSISTENT. The moved code stays Solidity; nothing was reimplemented at another layer.
  • "dont test local just let ci handle" (human, this work) — OBEYED. The verdict rests on the five green checks.
  • "merge 33" (human, this work) — this action. --merge, not squash; branch left intact.
  • Repo conventions — no rulings or invariants section was readable in this repo's CLAUDE.md. The diff removes only unreferenced code and its own tests, so it changes no convention, no public surface anyone imports, and no generated-artifact contract; copy-artifacts passing confirms the generated surface is unaffected.

@thedavidmeister
thedavidmeister merged commit 87b918b into mainAug 13, 2026
5 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@thedavidmeister