Address registry: interface, concrete, reader lib and post-deploy cross-network verification - #26
Open
thedavidmeister wants to merge 5 commits into
Open
Address registry: interface, concrete, reader lib and post-deploy cross-network verification#26thedavidmeister wants to merge 5 commits into
thedavidmeister wants to merge 5 commits into
Conversation
…ork gate Deterministic deploys and configured addresses are in tension: the CREATE2 address is a function of the creation code, so any address baked into a contract is part of its identity. Configured addresses therefore get hardcoded, one copy per repo, and can never be changed without moving deployments. `IAddressRegistryV1` is the read-at-run-time alternative. An immutable root authority binds an opaque `bytes32` name to an address, once; nothing, root included, can change one after; and reading an unbound name reverts rather than answering with the zero address. `LibAddressRegistry.resolve` reads it at its deterministic Zoltu address, verifying the registry's code hash first, the same way `LibRainDeploy` verifies `ZOLTU_FACTORY_CODEHASH`. It resolves a name and stops there. `LibRainDeploy.checkRegisteredAddressesOnNetworks` is the deploy-time gate that belongs beside the multi-network broadcast rather than in every consumer's deploy script: every name must resolve to the address the deployment expects, on every target network. Write-once is what makes that pre-flight as strong as an inline check — an answer that exists cannot change, and one that does not exist reverts. The implementation, `AddressRegistry`, lives in rain.factory.deploy; `ADDRESS_REGISTRY` and `ADDRESS_REGISTRY_CODEHASH` pin it, derived from its creation code.
|
Warning Review limit reached
Next review available in: 23 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 (18)
WalkthroughChangesAddress registry validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant LibRainDeploy
participant Vm
participant NetworkFork
participant LibAddressRegistry
LibRainDeploy->>LibRainDeploy: Validate names and expected addresses
loop Each requested network
LibRainDeploy->>Vm: Create network fork
Vm-->>NetworkFork: Select network fork
LibRainDeploy->>LibAddressRegistry: Resolve registered names
LibAddressRegistry-->>LibRainDeploy: Return addresses or revert
end
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 11-15: Update the rain.deploy description in CLAUDE.md to state
that the Zoltu deterministic deployment proxy derives its address using CREATE2
with a zero salt, replacing the incorrect CREATE and predictable nonce
description. Preserve the surrounding explanation about identical addresses
across supported networks.
In `@src/lib/LibRainDeploy.sol`:
- Around line 248-270: Make registry validation mandatory by adding names and
expectedAddresses to the deployToNetworks/deployAndBroadcast entry point, then
call checkRegisteredAddressesOnNetworks for all target networks before the first
deployment broadcast. Propagate the new arguments through both deployment
functions and add an integration test proving a mismatched binding prevents
deployAndBroadcast.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d64bd9a3-b7e5-41ba-bbf9-6546e63877af
📒 Files selected for processing (8)
CLAUDE.mdREADME.mdsrc/interface/IAddressRegistryV1.solsrc/lib/LibAddressRegistry.solsrc/lib/LibRainDeploy.soltest/lib/AddressRegistryPins.soltest/src/lib/LibAddressRegistry.t.soltest/src/lib/LibRainDeploy.t.sol
… five
The multi-network test forked every entry of `supportedNetworks()`, and CI's
`base_sepolia` endpoint times out on its free plan ("Request timeout on the free
plan"), so the test failed on infrastructure rather than on the code.
What the test is for is that the loop visits every network it is given, which
two prove as well as five. The roster itself is `testSupportedNetworks`'s job.
Arbitrum and Base are the networks the rest of the suite already forks, so the
test no longer depends on endpoints nothing else here touches.
… nonce The address is a pure function of the creation code, which is the property the whole library rests on; describing it as a nonce-based CREATE would lead a consumer to derive the wrong address. Also lists Base Sepolia, which supportedNetworks() has returned all along.
…moves here
Three changes that only make sense together.
WRITE-ONCE WAS WRONG. The name a consumer resolves is in its creation code, so
a binding welded to one address forever cannot express an ordinary owning-Safe
rotation: it would need a new name, hence new creation code and a new
deterministic address. That is the exact problem the registry exists to remove,
relocated. What write-once bought was narrower than it looked - it protected
bindings on chains already in use from a compromised root, and never protected
a fresh chain, since an attacker registers the name there first either way.
Root may now re-register a name. Everything else stands: immutable root,
reverts on unset, and `register` still rejects the zero address, which still
matters because unset reads as zero.
THE GATE MOVES AFTER THE DEPLOY. A pre-deploy check against a mutable registry
is TOCTOU and guarantees nothing. Deploy first, verify, then migrate onto it -
so verification reads the value the deployed contract already snapshotted in
its constructor, which is settled state and cannot move underneath the check. A
poisoned deploy is then a burned deterministic address found before anything
depends on it, rather than a compromise. `checkRegisteredAddresses{,OnNetworks}`
are replaced by `checkResolvedAddresses{,OnNetworks}`, which are deliberately
source-agnostic: only the consumer knows where it stored what it resolved, so
the consumer supplies the reads and this library supplies the fork loop and the
comparison. Re-reading the registry post-deploy would assert a value that can
move rather than the value the deployment actually took.
THE CONCRETE MOVES INTO THIS REPO. The address and codehash are a function of
the creation code, which is a function of the compiler settings that compiled
it. With the concrete, the settings and the pins all here, there is no boundary
across which they can silently diverge and nothing depends on
`rain-factory-deploy` for them. `foundry.toml` pins solc/optimizer/evm_version
exactly for that reason, and the release lifecycle moves to `rainix-tag-release`
to match what this repo now is: a repo carrying a deployed concrete whose pins
consumers rely on. Already-published versions stay published and consumers pin
exact versions, so nothing downstream changes.
Slither's low-level-calls detector is excluded: the post-deploy read is a
staticcall with consumer-supplied calldata by design, its success and return
length are both checked, and there is no typed alternative when the consumer is
the one who knows what to read.
thedavidmeister
changed the title
… settings The literal is the address the live factory returns for MockDeployable's creation code, which is a function of the settings that compile it. Pinning solc/optimizer/evm_version in foundry.toml - needed because this repo's deploy pins depend on them - moved it. The comment now says which settings it is a function of, since that is what makes a literal here stable at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #25 (issue text updated
to match). Supersedes
rainlanguage/rain.factory.deploy#7, which held the
concrete before it moved here.
What this adds
src/interface/IAddressRegistryV1.solandsrc/concrete/AddressRegistry.sol— an immutable root authority binds an opaque
bytes32name to an address(
register); anyone reads a bound name (get); reading an unbound namereverts. Three errors, one event, two functions. No removal, no upgrade, no
authority besides root — root is a compile-time constant, so it cannot even hand
itself over.
src/lib/LibAddressRegistry.sol—resolve(bytes32 name)reads the registryat its deterministic address, verifying its code hash first, exactly as
LibRainDeployverifiesZOLTU_FACTORY_CODEHASH. It resolves a name and stopsthere: it sets no owner, knows nothing about
Ownableor RBAC.LibRainDeploy.checkResolvedAddresses{,OnNetworks}— post-deployverification across every target network.
Bindings are mutable, and that is the point
The name a consumer resolves is in that consumer's creation code. A write-once
binding therefore cannot express an ordinary rotation of an owning Safe: it would
need a new name, hence new creation code and a new deterministic address — the
exact problem the registry exists to remove, relocated.
Write-once also bought less than it looked like it did. It protected bindings on
chains already in use from a compromised root, and never protected a fresh
chain, since an attacker who is root binds the name there first either way.
Mutability costs nothing already deployed, because a consumer resolves once,
in its constructor, and stores the answer.
MockResolvedOwnerin the test treeis that shape, and
testCheckResolvedAddressesUnaffectedByRebindingis the proof:re-bind the name after deploying, and the registry answers differently while the
deployed contract does not move.
Kept from the original design: immutable root, revert on unset, and
registerrejecting the zero address — that one still matters, because unset reads as
zero, so a zero binding would be a name both bound and unreadable.
testRegisterZeroAccountCannotUnbindpins that there is no way to unbind.The check moved after the deploy
A pre-deploy check against a mutable registry is TOCTOU and guarantees nothing:
the binding can move between the check and the constructor that consumes it. So
checkRegisteredAddresses{,OnNetworks}are gone, replaced bycheckResolvedAddresses{,OnNetworks}, which run after the deploy against thevalue the deployed contract has already snapshotted — settled state that cannot
move underneath the check. A network where the deployment took something else is
a burned deterministic address, found while nothing points at it yet.
Shape. Only the consumer knows where it stored what it resolved, so the
consumer supplies the reads (
abi.encodeCall(IOwnable.owner, ())and the like)and this library supplies the fork loop and the comparison. It is deliberately
source-agnostic: it asserts that the deployed contract holds the expected
address, not where that address came from. Re-reading the registry here would
assert a value that can move rather than the value the deployment actually took,
so no registry-specific cross-network helper survives — the honest answer was
that the old gate should be removed rather than reshaped in place.
Guarantee, stated: on every listed network,
targetanswers each supplied readwith exactly the expected address; a read that reverts, that hits an address with
no code, or that answers anything other than one word is a failure, never a
pass.
The concrete lives here
The address and codehash are a function of the creation code, which is a
function of the compiler settings that compiled it. With the concrete, the
settings and the pins all in this repo, there is no boundary across which they
can silently diverge, and nothing depends on
rain-factory-deployto get them.foundry.tomlnow pinssolc = "0.8.25",optimizer_runs = 100000andevm_version = "cancun"exactly, so the pins cannot move under a compiler ordefault-target change.
Pins, both derived and both checked in-repo by
AddressRegistryDeployPinsTest:ADDRESS_REGISTRY_DEPLOYED_ADDRESS = 0x0B8CAaDADF7c53a1b0Af8A7A8E7F3ca90DE517d6ADDRESS_REGISTRY_DEPLOYED_CODEHASH = 0xd9a6a2f03c1e1851becfedba819a436dcccc81c40ac8df02be6815f0b261d042zoltuAddress(type(AddressRegistry).creationCode)for the address (the factoryis
CREATE2over its calldata with a zero salt),keccak256of the runtime codefor the hash, and both re-derived by actually deploying through the etched Zoltu
factory. They move if the root constant changes, and the root is currently a
loud placeholder (
0xdeaDDeADDEaDdeaDdEAddEADDEAdDeadDEADDEaD) because I do nothave the real one and will not invent one.
Release lifecycle: autopublish → tag release
This repo now carries a deployed concrete whose pins consumers rely on, which is
what
rainix-tag-releaseexists for and whatrainix-autopublish'smerge-driven, next-version lifecycle is wrong for.
package-release.yamlmovesto
rainix-tag-release,[package].versionbecomes the LAST released version(
0.1.5), andscript/BuildPointers.solis thesnapshot-generate-cmd.Switching retracts nothing — every published version stays published and
consumers pin exact versions, so
st0x.deployat 0.1.4 andflowat 0.1.2 areuntouched. It changes who cuts a release, not how anyone consumes one.
No
src/generated/<tag>/snapshot is committed yet, deliberately. Thatdirectory is append-only and the root is still a placeholder, so a snapshot
frozen now could never be corrected.
LibAddressRegistryDeploytherefore carriesthe two literals by hand, at the exact import path the generated version will
occupy, so consumers' imports do not move when the first real release replaces
it. See the two flagged items at the bottom.
Slither's
low-level-callsdetector is excluded: the post-deploy read is astaticcall with consumer-supplied calldata by design, both its success and its
return length are checked, and there is no typed alternative when the consumer is
the one who knows what to read.
QA
Discriminating tests
36 non-fork tests pass; the registry is never mocked —
AddressRegistryisdeployed through the Zoltu factory so every test runs at the pinned address with
the pinned code hash.
testRegisterRebind,testRegisterRebindSameAccount,testRegisterRebindRepeatedly(only the most recent binding counts),testRegisterRebindEvent(a re-binding emits, so an indexer is not leftserving the original),
testRegisterRebindOnlyRoot(a bound name gives nobodyelse authority; the failed attempt leaves the binding intact).
testRegisterZeroAccount,testRegisterZeroAccountCannotUnbind.testGetUnsetReverts,testGetNoGeneratedMappingGetter,testGetNoOtherEntryPoint(fuzzed selectoroutside the two interface selectors).
testCheckResolvedAddressesUnaffectedByRebindingisthe load-bearing one — after deploying, root re-binds the name; the registry
answers differently, the deployed contract does not, the check against the
deployment's own value still passes, and a check against the new registry
value fails. That is the whole argument for verifying after rather than before,
as a test.
testCheckResolvedAddressesUnreadableTargetReverts(an address with no codestatic-calls successfully and returns nothing),
testCheckResolvedAddressesRevertingReadReverts....ChecksEveryRead,...LengthMismatchReverts,...OnNetworksNoNetworksReverts,...OnNetworksLengthMismatchRevertsBeforeForking(the network is adeliberately unconfigured RPC alias, so reaching a fork at all is the failure).
...OnNetworksEachNetworkand...OnNetworksMismatchRevertsmakethe deployment persistent across forks so the multi-network loop runs end to
end. Arbitrum and Base rather than all of
supportedNetworks()— the loop iswhat is under test, and the earlier five-network version failed on CI's
base_sepoliaendpoint answering408 Request timeout on the free plan.Mutations applied, and what killed each
Applied to the committed tree one at a time, whole suite run, tree restored,
baseline green.
Carried over from the write-once design and re-run against the new code: root
check removed →
testRegisterOnlyRoot; root check inverted → all register tests;zero-address check removed →
testRegisterZeroAccount; event not emitted →testRegisterEvent; event emitted withmsg.sender→testRegisterEvent;getreturns zero instead of reverting →
testGetUnsetReverts; mapping madepublic→
testGetNoGeneratedMappingGetter; root constant changed →testAddressRegistryPinsDeriveFromThisSource.New for this design:
registerre-introduces a write-once reverttestRegisterRebind,testRegisterRebindSameAccount,testRegisterRebindRepeatedlyregisterskips the store when the name is already bound (silent no-op rebind)testRegisterRebind,testRegisterRebindRepeatedlycheckResolvedAddressesdrops thesuccesschecktestCheckResolvedAddressesRevertingReadRevertscheckResolvedAddressesdrops thereturnData.lengthchecktestCheckResolvedAddressesUnreadableTargetRevertscheckResolvedAddressesloop stops after the first readtestCheckResolvedAddressesChecksEveryReadcheckResolvedAddressesaddress comparison invertedtestCheckResolvedAddressesMatch,...MismatchRevertstestCheckResolvedAddressesLengthMismatchRevertsNoNetworksguard removedtestCheckResolvedAddressesOnNetworksNoNetworksReverts...OnNetworksLengthMismatchRevertsBeforeForkingresolvecode hash check removed / invertedtestResolveNoRegistry,testResolveWrongCode/testResolveRegisteredNo survivors.
Oracle
The issue is the oracle, not the code. The pins have a second, independent
oracle — the Zoltu factory itself, deployed to and read back, rather than a
recomputation of the same formula the library uses.
Category check
The issue's example is initial ownership; it says the problem is not
owner-specific and nothing here is — the key is an opaque
bytes32, the value isan address, and neither lib has a notion of an owner, a role or an initializer.
The post-deploy check is source-agnostic for the same reason: it covers "the
deployment holds what it should" as a category rather than "the registry said the
right thing" as one instance. Every error either contract can revert with has a
test.
n/a
AddressRegistrycannot be deployed until thereal root is supplied.
Two things for you, not for me
rain.deployis now a mixed shape. The deploy-repo convention is"audited code only, internally consistent, version↔snapshot↔pins enforced by a
test".
AddressRegistryfits it. The existing tooling libs do not, and werenever meant to:
isStartBlock,findDeployBlock,deployToNetworks,deployAndBroadcastand the newcheckResolved*areforge script/test-timehelpers that take a
Vm— they are never deployed, never audited as deployedcode, and have no pins. So this repo is now one audited deployed concrete plus
a body of scripting tooling under one
[package].version, which theconvention did not anticipate. It is coherent (the tooling has no on-chain
surface to be inconsistent with) but it does mean a
sol-v*tag nowsimultaneously means "these pins are live" and "here is a tooling release",
and those two have no reason to move together. Reporting, not resolving.
rainix-tag-releaserequiressnapshot-generate-cmd, soBuildPointers.solis here and wired — but the first
sol-v*tag is what writessrc/generated/<tag>/and regeneratesLibAddressRegistryDeployinto itsaliasing form. Until a human supplies the real root, tagging would freeze a
placeholder-derived address into an append-only directory, and the workflow's
verifystep would fail anyway since nothing is deployed. There is also noscript/Deploy.solsuite ormanual-sol-artifacts.yamldispatch here yet;both want adding as part of the change that supplies the root and cuts the
first release, not before it.