docs: drop the false "AddressRegistry is the only concrete" premise - #105
Conversation
`MigrationRegistry` is a second concrete in `src/concrete/`, declared as a candidate in `RegistryDeploySuites` and carrying its own generated snapshot, so every doc that justifies itself with "AddressRegistry is the only concrete/creation code in this repo" is asserting something false. Three instances, all documentation: - `ExampleDeploySuites` used the false premise as the reason its second address is a mock, which invited swapping `MockDeployableV2` for `MigrationRegistry` and collapsing the loop-does-not-stop-at-first-entry coverage. Restated as the reason that still holds: this is a fixture for the abstracts, not a copy of `RegistryDeploySuites`, so what the abstracts are tested against is written in the file rather than tracking whatever `src/concrete/` currently holds. - `RainDeployVerifyChainTest.secondDeployedAddress` called `MockDeployableV2` "the only other creation code in this repo". Restated as the load-bearing fact: it is the same derivation `ExampleDeploySuites` declares, which is why the `setUp` etch lands on the address the matrix reads. - `package-release.yaml` named a single deployed concrete; its sibling `manual-sol-artifacts.yaml` already names both. Closes #75 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
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 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 (3)
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 |
|
Reviewed 7e2c845: ready — Closes #75. Comments only, and both corrections are factually right: this repo carries TWO deployed concretes, The replacement is better than a count fix. It says why the second address comes from a mock rather than from the other real concrete — the fixture borrows ONE real snapshot to keep the released and candidate paths real, and derives everything else inline, so what the abstracts are tested against does not move when CI green, 0 unresolved threads — vacuous, CodeRabbit reports |
Closes #75
What was wrong
MigrationRegistryis a second concrete insrc/concrete/, declared as a candidate inRegistryDeploySuitesand carrying its own generated snapshot undersrc/generated/candidate/. Every doc that justifies itself with "AddressRegistryis the only concrete/creation code in this repo" is therefore asserting something false.Departure from the issue's proposed wording — please read
The issue proposes replacing the false premise with a chain-safety rationale: the second address must be a freely-etchable mock "rather than a second real registry whose pinned address the chain matrix also asserts about".
I did not use that wording, because it would have replaced one false statement with another. Two sibling files argue at length for the opposite, and they are right:
test/src/abstract/RainDeployVerifyChain.t.sol:52-55— "Pointing the fixture at a mock nobody deploys would move that dependency rather than remove it: the Zoltu factory is permissionless, so no address is structurally unoccupiable. Etching the state the assertion is about is what removes it."test/src/abstract/RainDeployVerifyChainCandidate.t.sol:75-80— makes the same argument for the candidate's absence.So a mock is not structurally safer than a real registry here; the etch is what makes either safe. The proposed rationale also does not distinguish the two entries, since the fixture's first entry is already a real deployed address (
AddressRegistry) that the fixtures etch and empty. AndRegistryDeployChainTestreadsreleasedSuites(), which is empty — this repo has released nothing — so "the real chain group asserts about it too" is not true of any address today.Restated instead as the reason that actually holds and is checkable from the code: this is a fixture for the abstracts, not a copy of
RegistryDeploySuites, so what the abstracts are tested against is written in the file rather than tracking whateversrc/concrete/currently holds. That is what makes swappingMockDeployableV2forMigrationRegistrythe wrong move — the thing the paragraph exists to prevent.Scope: three instances, not one
The issue cites one line; the same falsehood appears three times, so this covers the category.
test/abstract/ExampleDeploySuites.sol:28-30— the cited one, and the one that matters: it argued for a change that would collapse the "loop does not stop at the first entry" coverage.test/src/abstract/RainDeployVerifyChain.t.sol:63-64— calledMockDeployableV2"the only other creation code in this repo". Restated as the load-bearing fact: it is the same derivationExampleDeploySuitesdeclares, which is why thesetUpetch lands on the address the matrix reads..github/workflows/package-release.yaml:2-3— named a single deployed concrete. Its siblingmanual-sol-artifacts.yamlalready names both.QA
ls src/concrete/returnsAddressRegistry.solandMigrationRegistry.sol;ls src/generated/candidate/returns snapshots for both;RegistryDeploySuites.candidateSuites()builds a length-2 array fromaddressRegistryCandidate()andmigrationRegistryCandidate();CLAUDE.mdnames both as ordinary deployed contracts. That is what falsifies "the only concrete". For the replacement text, the oracle is the sibling reasoning quoted above (RainDeployVerifyChain.t.sol:52-55,RainDeployVerifyChainCandidate.t.sol:75-80) plusmigrationRegistryCandidate()'sdependencies: new address[](0)— which is why the issue's proposed chain-safety rationale has no mechanical basis and was not used.ExampleDeploySuites.sol:28-30). The category is "docs assertingAddressRegistryis the repo's only concrete/creation code".grep -rn "only concrete\|only other creation code" --include=*.solfinds 2 hits, both fixed; a sweep for single-concrete claims in workflows/docs found a 3rd inpackage-release.yaml, also fixed. Covered: all 3. Not covered: nothing in this category remains — the same greps return only the corrected text.Verification
Documentation only — no behavioural change, no deployed bytecode touched.
forge fmt --check— cleanforge build— cleanforge testwith all five fork RPCs configured — 215 passed, 0 failed, 0 skipped across 17 suites, including the chain/fork suites that consume both edited Solidity filesyamlfmt,no-consumer-prettier) — passed on the workflow edit🤖 Generated with Claude Code