Skip to content

Replace LibStringSet with LibMemoryKV - #210

Merged
thedavidmeister merged 2 commits into
mainfrom
2026-09-15-string-set-to-memkv
Sep 15, 2026
Merged

thedavidmeister merged 2 commits into
mainfrom
2026-09-15-string-set-to-memkv

Conversation

@thedavidmeister

@thedavidmeister thedavidmeister commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

What

src/lib/LibStringSet.sol is deleted. Its sixteen call sites use LibMemoryKV
from rain-lib-memkv 0.1.4, newly pinned, which is the org's memory key/value
store.

A string becomes a key as its content hash. A list becomes a set by folding
set over it from an empty store, and a question is has, the bool read added
in rainlanguage/rain.lib.memkv#28. Where a test asks the same list more than
once the set is built once; a both-direction pair builds one set per side.

Why

Nothing about whole-string membership is deploy machinery. The library arrived
here as a test helper, was promoted into the published source tree when the
snapshot verifier needed it, and was the only file in that tree with no deploy
semantics. Every consumer of this package received it.

It was also not the only copy. raindex hand-rolls the same nested scan twice in
its own build test, which is the second consumer that makes this a primitive
rather than a helper.

The store is the right home because a set is a map whose values carry nothing.
It is audited, it is already a dependency of rainlang, and its lookup is a
linked list spread across fifteen of them rather than a linear scan. Nothing in
this diff walks a list or compares a key, so a lookup bug is a bug there, under
that library's own tests, rather than a second implementation of the same idea
kept in step by hand.

No replacement helper was added, in src or in test. The fold sits at each
call site, because the point of the change is to remove a library rather than
move one.

QA

  • Discriminating tests: none added. This is a substitution behind sixteen
    existing assertions, whose messages and meanings are byte-identical before and
    after. The tests that already covered each site are named in the table below,
    and they are the discriminators: each fails under a mutation of the fold that
    replaced the call.
  • Mutations applied: fifteen, all KILLED, table below. Read the two groups
    differently. M01 and M02 target src/abstract/RainDeployVerifySnapshot.sol
    and are coverage evidence in the ordinary sense. M03 to M15 target folds that
    now sit inside test files, so breaking one makes that test fail by
    construction. They are EXECUTION PROBES, not coverage: each shows its
    converted site is reached and that the surrounding assertion depends on the
    set being built correctly. Counting them as coverage would overstate this
    change, which adds no test.
mutation target verdict killed by
M01 supported-network fold records nothing src KILLED testSupportedNetworksAreFullyConfigured
M02 supported-network fold skips the first network src KILLED testSupportedNetworksAreFullyConfigured
M03 frozen-name fold records nothing test KILLED testSnapshotContractNamesAreTheGeneratedContracts
M04 candidate-side fold records nothing test KILLED testEveryCandidateHasASnapshot
M05 snapshot-side fold records nothing, candidate test test KILLED testEveryCandidateHasASnapshot
M06 snapshot-side fold records nothing, aggregate test test KILLED testEverySnapshotIsInTheReleasedAggregate
M07 aggregate-side fold records nothing test KILLED testEverySnapshotIsInTheReleasedAggregate
M08 record-walk fold records nothing test KILLED testFrozenSnapshotPathsFindsEveryReleaseAndNothingElse
M09 record-walk fold skips the first path test KILLED testFrozenSnapshotPathsFindsEveryReleaseAndNothingElse
M10 rolling snapshot seeded into the set it must be absent from test KILLED testFrozenSnapshotPathsExcludesTheRollingSnapshot
M11 nested-release fold records nothing test KILLED testFrozenSnapshotPathsIgnoresWhatIsNestedInsideARelease
M12 loose-in-root fold records nothing test KILLED testFrozenSnapshotPathsIgnoresAFileLooseInTheRoot
M13 multi-contract freeze fold records nothing test KILLED testFreezeCutsEveryNamedContract
M14 multi-contract freeze fold skips the first record entry test KILLED testFreezeCutsEveryNamedContract
M15 append-freeze fold records nothing test KILLED testFreezeLeavesEarlierReleasesAlone

Probe suite excluded the fork tests, so a rate-limited endpoint could not be
scored as a kill. Its baseline was green at 419 passed.

  • Oracle: the NatSpec on the store's get, which states that a key set to zero
    and a key never set are indistinguishable by value and that existence must be
    read separately. That is why every member is stored against a zero value and
    every question reads has rather than comparing a value.
  • Category check: the ask was to take the string library out of this repo. It is
    gone from src, from test, and from every other reference: REUSE metadata,
    the slither configuration, the workflows and the README were all checked and
    hold none. No helper replaced it.
  • Evidence: the whole suite including fork tests, run twice on the final tree,
    481 passed, 0 failed, 0 skipped across 22 suites. The run before the change
    showed 18 failures, every one an HTTP 429 from a public endpoint.

🤖 Generated with Claude Code

https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation of configured RPC and verification network names, helping identify unsupported network configuration earlier.
  • Refactor

    • Updated internal network and snapshot membership handling for more efficient and consistent checks.
    • Removed the legacy string-set implementation.
  • Chores

    • Added the memory key-value library dependency required by the updated validation and snapshot processing.

LibStringSet was a linear keccak scan over a string[], doing what
rain-lib-memkv already does. 0.1.4 adds has, which reports membership as a
bool rather than through a tuple, so the shared library can be called from
inside an assertion and the local one has nothing left of its own.

Each of the sixteen call sites folds its list into a MemoryKV and asks that.
Nothing replaces the deleted library: carrying one fewer is the point.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Warning

Review limit reached

Next included review available in 18 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1e7b4838-83f2-4729-842c-616c0b57ba18

📥 Commits

Reviewing files that changed from the base of the PR and between 3f23d80 and 2899d8b.

📒 Files selected for processing (1)
  • slither.config.json

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1ed67414-d58f-44f9-893a-b93f824db1d9

📥 Commits

Reviewing files that changed from the base of the PR and between b44a449 and 3f23d80.

⛔ Files ignored due to path filters (1)
  • soldeer.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • foundry.toml
  • remappings.txt
  • src/abstract/RainDeployVerifySnapshot.sol
  • src/lib/LibStringSet.sol
  • test/script/Build.t.sol
  • test/src/lib/GeneratedSnapshotShape.t.sol
  • test/src/lib/LibRainDeploySnapshot.t.sol
💤 Files with no reviewable changes (1)
  • src/lib/LibStringSet.sol

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The project adds rain-lib-memkv and replaces LibStringSet membership checks with hashed MemoryKV sets in production validation and snapshot-related tests. The local LibStringSet library is deleted.

Changes

MemoryKV membership migration

Layer / File(s) Summary
Dependency and production membership migration
foundry.toml, remappings.txt, src/abstract/RainDeployVerifySnapshot.sol, src/lib/LibStringSet.sol
The project adds the rain-lib-memkv dependency and remapping. Network configuration checks use hashed MemoryKV lookups. LibStringSet is removed.
Test membership migration
test/script/Build.t.sol, test/src/lib/GeneratedSnapshotShape.t.sol, test/src/lib/LibRainDeploySnapshot.t.sol
Snapshot, generated-contract, and path assertions build hashed MemoryKV sets and use has for membership checks.

Priority: ⬇️ Low

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

Change: Refactor

Suggested reviewers: claude

Merge Risk: ⚪ Minimal · up to 3f23d

The membership migration has no established correctness or merge-blocking risk.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: replacing the deleted LibStringSet library with LibMemoryKV across the project.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2026-09-15-string-set-to-memkv

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.

It declares a permissive pragma that the solc-version detector flags, the same
reason forge-std is already filtered. Analysing a published dependency says
nothing about this repo.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
Sign up for free to 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.

2 participants