Skip to content

docs: say what findDeployBlock, isStartBlock and the deploy actually do - #33

Merged
thedavidmeister merged 4 commits into
mainfrom
2026-08-14-issue-18-docs-match-code
Aug 14, 2026
Merged

docs: say what findDeployBlock, isStartBlock and the deploy actually do#33
thedavidmeister merged 4 commits into
mainfrom
2026-08-14-issue-18-docs-match-code

Conversation

@thedavidmeister

Copy link
Copy Markdown
Contributor

Closes #18

Every one of the five was re-established against main at d08203e before
anything was changed. Two are already fixed and are untouched here.

Already fixed on main — no change

5, and the CLAUDE.md half of 2. bf5ac30 replaced "uses CREATE with a
predictable nonce" with CREATE2 over the factory's calldata under a zero salt,
and added Base Sepolia to the network list in the same edit; 10b14d5 added
BASE_SEPOLIA_RPC_URL to the sample .env. Every site the issue names for
those two is correct on main.

The checkDependencies half of 1. #26 rewrote the architecture section:
neither checkDependencies nor "re-verifies dependencies" survives it.

1 — dependency checking is per network, and only where the deploy runs

The Key Design Patterns bullet the issue also quotes is untouched since the file
was created:

Dependency checking: Before deploying to any network, all dependencies
(contract addresses) are verified to have code on-chain.

It reads as an all-network pre-flight, which is the one thing the
deployToNetworks NatSpec itself says does not exist — "which is why no separate all-network
pre-flight is needed". Replaced with what the code does: per network, on that
network's own fork, immediately before broadcasting, and only on the branch that
broadcasts — factory code and ZOLTU_FACTORY_CODEHASH, then code at every
dependency. The architecture bullet for deployToNetworks carried the same
overstatement one line long and is fixed with it; the code hash check is the only
part of it that runs on both branches.

2 — the second site: a test comment

testDeployZoltu's doc comment still said the returned address is "the
deterministic address predicted by the factory's nonce". Under that model the
address depends on per-chain deploy order; under the actual CREATE2-salt-0 model
it is a pure function of the creation code, which is the property this whole
library rests on. Now says what the factory does.

3 — the sentence is dropped rather than implemented

findDeployBlock's NatSpec claimed "The result is validated via isStartBlock".
It is not, and implementing it would be worse than the false claim.

The check cannot fail. high starts at a block where the code hash matches
(checked on entry) and is only ever assigned a mid where it matches. low
starts at startBlock, where the hash does not match (checked before the loop),
and thereafter is only ever mid + 1 for a mid where it did not match. low
cannot exit still equal to startBlock, because that needs high to descend to
startBlock and high only ever takes blocks where the hash matches. So at exit
low == high is a block where the hash matches whose predecessor is a block
where it does not — isStartBlock's exact condition. Calling isStartBlock on
the result is a tautology.

It would be a tautology dressed as a guard. What actually makes a result
meaningless is a non-monotone history (item 4), and isStartBlock reads two
adjacent blocks, so it is blind to precisely that. A check that can only ever
agree, sitting where a reader expects the guard against the real hazard, is worse
than no check: it enshrines the assurance it does not provide.

And it is the redundant re-read this library already argues against by name.
deployToNetworks explains it reads each dependency exactly once so "a transient
RPC inconsistency on a redundant second read cannot report an already-deployed
dependency as missing and abort an otherwise-valid deploy". Three more archive
round trips that can only agree carry that same failure mode and none of the
upside.

testFindDeployBlockZoltuFactory already asserts isStartBlock of the result on
a Base fork. That is where the sentence came from, and where the round trip
belongs.

4 — the monotonicity requirement holds, and is now stated

Checked before writing it down. The search needs the predicate
codehash == expectedCodeHash to be monotone over [startBlock, block.number]:
once true, true at every later block. Worked through on the non-monotone history
F F T T F F F T T T T over blocks 0..10, the search returns 7 — a block that
satisfies isStartBlock and is not the first appearance, which is 2.

isStartBlock needs it too, in its own form. It answers on two adjacent blocks,
so against a target that held the hash, lost it and holds it again it answers
true at every reappearance rather than only the earliest, while its NatSpec calls
what it identifies "the first". Both now say so, with a pre-Cancun
SELFDESTRUCT and CREATE2 redeploy of the same code at the same address as the
concrete instance.

Verification

Via nix develop -c, on the merge of main:

  • forge fmt --check — clean
  • reuse lint — compliant, 55/55 files
  • slither . — 44 contracts, 100 detectors, 0 results
  • forge test — 95 passed, 47 failed, 142 total

Bare origin/main at d08203e, same checkout, same shell: 95 passed, 47 failed,
142 total. Identical. Every one of the 47 is
vm.createSelectFork: environment variable *_RPC_URL not found — there is no
.env in this environment, so every fork test fails before it runs. Filtering
the failure list for anything that is not an RPC-env error returns nothing, on
both sides.

QA

  • Discriminating tests: n/a — the diff is comments and Markdown only, so no
    compiled statement changes and no test can distinguish it from base. The
    property the rewritten findDeployBlock NatSpec asserts is already covered:
    testFindDeployBlockZoltuFactory asserts isStartBlock of the search result
    on a Base fork.
  • Mutations applied: n/a — a docs-only diff has no line whose mutation changes
    behaviour. Verified as such rather than assumed: forge test is 95 passed / 47
    failed / 142 total on this branch and 95 / 47 / 142 on bare origin/main at
    d08203e, run in the same checkout and the same nix develop shell, which is
    what a comment-only diff must produce.
  • Oracle: the code, not the issue's report of the code. Each of the five claims
    was re-derived against main at d08203e before anything was edited — the
    address derivation from zoltuAddress and ZOLTU_FACTORY_BYTECODE, the
    dependency-check branch structure from deployToNetworks itself, and the
    isStartBlock-is-a-tautology result from the binary search's own loop
    invariants, worked through on an explicit non-monotone history
    (F F T T F F F T T T T over blocks 0..10 returns 7, not 2).
  • Category check: the issue asks for 1, 2, 3, 4, 5; covered 1, 2, 3, 4 here. 5,
    and the CLAUDE.md sites of 1 and 2, were already fixed on main by
    bf5ac30, 10b14d5 and Address registry: interface, concrete, reader lib and post-deploy cross-network verification #26 — reported above rather than silently dropped, and
    deliberately not "re-fixed". The second site of 2 that the issue mentions in
    passing, the testDeployZoltu comment, is included because Address registry: interface, concrete, reader lib and post-deploy cross-network verification #26 did not reach
    test/.

🤖 Generated with Claude Code

thedavidmeister and others added 3 commits August 14, 2026 16:00
Four of the five doc-vs-code claims in #18 that current main still carries.

findDeployBlock's NatSpec claimed the result is validated via isStartBlock. It
is not, and adding the call would not be a check: the loop exits with the code
hash matching at the result and not matching at the block before it whatever
the history, so isStartBlock holds of the result by construction and could only
agree. Dropped, and the requirement that is real is documented instead — both
functions read the code hash as if it were monotone over the search range, and
a target that held the hash, lost it and holds it again defeats them silently.

The Zoltu factory derives with CREATE2 under a zero salt; a test comment still
said the factory's nonce predicted the address. CLAUDE.md described dependency
checking as an all-network pre-flight, when it is per network and only on the
branch that deploys.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@thedavidmeister thedavidmeister self-assigned this Aug 14, 2026
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@thedavidmeister, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bce774f1-fc5b-4408-80e1-ab946d6dfdc5

📥 Commits

Reviewing files that changed from the base of the PR and between d08203e and 7630749.

📒 Files selected for processing (3)
  • CLAUDE.md
  • src/lib/LibRainDeploy.sol
  • test/src/lib/LibRainDeploy.t.sol

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.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@thedavidmeister
thedavidmeister merged commit a7e7cb2 into main Aug 14, 2026
4 checks passed
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.

Docs describe behaviour the code does not have: checkDependencies, CREATE vs CREATE2, findDeployBlock validation, Base Sepolia

1 participant