feat: concrete Extrospect contract for Zoltu deployment (#25) - #26
Merged
Merged
Conversation
Closes #25. The repo's libraries are all internal — Solidity inlines them into the caller's frame. That's correct for inter-library composition but leaves no external entry point for: - Offchain consumers calling extrospection at a deterministic Zoltu address rather than embedding the library bytecode in every consumer. - Tests in dependent repos that need vm.expectRevert to catch a library-internal revert. expectRevert requires the revert at a depth below the cheatcode call; an inlined library call shares the test contract's frame so the cheatcode never sees a frame deeper than its own. Mirrors the rain.math.float pattern: src/concrete/DecimalFloat.sol exposes that library's surface as a parameterless-constructor contract for Zoltu. Smoke tests in test/src/concrete/Extrospect.t.sol pin external dispatch for one method per library; the libraries themselves have exhaustive tests under test/src/lib/.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis pull request adds IExtrospectV1 and a concrete Extrospect contract delegating bytecode inspection to existing libraries, wiring deterministic deployment (rain.deploy submodule, Forge script, and a manual GitHub Actions workflow), and a comprehensive equivalence test suite using a shared CBOR fixture. ChangesExtrospect Concrete Contract & Deployment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Consumers (cross-repo tests, offchain tooling) should depend on the interface so they can swap implementations without re-importing the concrete contract. The V1 suffix follows the rain convention — additive changes append to a V2 interface; V1 stays frozen for downstream consumers.
One test contract per IExtrospectV1 method, asserting the external dispatch matches the underlying library function — return value for pure/view fns, revert behaviour for the check* fns. Fuzz-driven where applicable. Replaces the smoke test file with finer-grained per-function coverage.
Deploys Extrospect via the Zoltu deterministic-deployment factory at 0x7A0D94F55792C434d74a40883C6ed8545E406D12 — same address on every EVM chain that has the factory. Salt is zero (Extrospect has a parameterless constructor) so the deployment address is determined entirely by the creation bytecode.
Use the shared LibRainDeploy.deployAndBroadcast helper instead of an inline call to the Zoltu factory. Brings address pinning, code-hash verification, and dependency checks across all Rain-supported networks.
Mirrors the standard rain workflow_dispatch action that runs the rainix deploy command inside nix develop, broadcasting Extrospect to every Rain-supported network via the Zoltu factory. Requires repo vars/secrets: CI_DEPLOY_*_RPC_URL, CI_DEPLOY_*_ETHERSCAN_API_KEY, PRIVATE_KEY (for main), PRIVATE_KEY_DEV (for non-main), and EXPECTED_EXTROSPECT_ADDRESS / EXPECTED_EXTROSPECT_CODEHASH for post-deploy verification.
Interface and concrete contract function order now mirrors the filesystem ordering of the per-fn equivalence test files (test/src/concrete/Extrospect.<fn>.t.sol). Lets a reviewer scan side-by-side without re-sorting in their head.
Each per-fn equivalence test had the same Extrospect instance + setUp. Hoist into an abstract base under test/concrete so the per-fn files focus on the equivalence assertions.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 @.github/workflows/manual-sol-artifacts.yaml:
- Line 27: Confirm whether DEPLOY_SKIP_SIMULATION is required for the Zoltu
factory and either remove it or add a clear inline comment explaining why
skipping forge's on-chain simulation is necessary; specifically, if you must
keep DEPLOY_SKIP_SIMULATION (the environment variable used to set
--skip-simulation in the deploy workflow), add an inline comment next to
DEPLOY_SKIP_SIMULATION explaining that Zoltu factory eth_call/eth_simulation can
falsely fail (e.g., unpredictable blockhash or factory behavior) and that
skipping simulation is an intentional trade-off for this factory, otherwise
remove DEPLOY_SKIP_SIMULATION so production deploys retain the simulation safety
net.
- Line 28: The current inline expression setting DEPLOYMENT_KEY can silently
fall back to secrets.PRIVATE_KEY_DEV on main; replace it with an explicit
"Resolve deployment key" step (referenced as a step id like key) that checks
github.ref for "refs/heads/main", verifies secrets.PRIVATE_KEY is non-empty on
main (exit 1 and emit an error if missing), and otherwise sets the output
deployment_key to either secrets.PRIVATE_KEY (for main) or
secrets.PRIVATE_KEY_DEV (for other branches); then use
steps.key.outputs.deployment_key as DEPLOYMENT_KEY in subsequent steps.
- Line 28: The current DEPLOYMENT_KEY expression silently falls back to DEV on
main when secrets.PRIVATE_KEY is empty; replace the inline expression with a
dedicated step (e.g., step id "key") that checks GITHUB_REF (or github.ref) and
the env vars MAIN_KEY/DEV_KEY (mapped from secrets.PRIVATE_KEY and
secrets.PRIVATE_KEY_DEV), exit non-zero with an error log if MAIN_KEY is empty
on refs/heads/main, and otherwise set an output deployment_key
(steps.key.outputs.deployment_key) to be consumed by the deploy step instead of
DEPLOYMENT_KEY.
In `@lib/rain.deploy`:
- Line 1: The submodule reference in lib/rain.deploy points to a non-existent
commit (43a6ed3a98f0141e1963b4b9136e8c80e2889bd1); fix it by checking out the
rain.deploy repository, finding the intended valid commit or branch, updating
the submodule reference to that commit/branch, and committing the updated
submodule pointer. Concretely: verify the correct commit/branch exists in the
rain.deploy remote, run git submodule update --init --remote or manually set the
submodule to the correct SHA/branch and git add lib/rain.deploy then commit and
push so the PR contains a valid, accessible submodule commit.
In `@test/src/concrete/Extrospect.checkNotMetamorphic.t.sol`:
- Around line 21-25: The tests only assert that extrospect.checkNotMetamorphic
and this.libCheckNotMetamorphicExternal revert, but not that they revert with
the same payload; change the assertions to perform low‑level calls and capture
the revert data for both calls and then assert the returned bytes are equal.
Concretely, replace vm.expectRevert();
extrospect.checkNotMetamorphic(withDelegatecall); with a low‑level call to
address(extrospect).call(abi.encodeWithSelector(extrospect.checkNotMetamorphic.selector,
withDelegatecall)) and capture (bool ok1, bytes memory data1), and replace
vm.expectRevert(); this.libCheckNotMetamorphicExternal(withDelegatecall); with
(bool ok2, bytes memory data2) =
address(this).call(abi.encodeWithSelector(this.libCheckNotMetamorphicExternal.selector,
withDelegatecall)); then assert ok1==false && ok2==false and data1 equals data2
(bytewise equality) to ensure revert payload equivalence.
In `@test/src/concrete/Extrospect.scanEVMOpcodesPresentInBytecode.t.sol`:
- Around line 13-16: When fuzzing the revert path, capture the revert data from
the external wrapper call and assert the library wrapper reverts with identical
data: wrap the call to extrospect(bytecode) in a try/catch that uses the
catch(bytes memory err) branch to capture the revert bytes, then call
vm.expectRevert(err) before invoking this._libScan(bytecode) so the test asserts
parity of revert-data between extrospect and _libScan.
In `@test/src/concrete/Extrospect.scanEVMOpcodesReachableInBytecode.t.sol`:
- Around line 14-18: The test currently only checks that _libScan(bytecode)
reverts; instead, for each fuzz input call both the external entrypoint and the
library entrypoint (use this._scan(...) and this._libScan(...)) via low-level
calls to capture their (bool success, bytes memory returndata) results, then
assert equality of the success flags (ok1 == ok2) and equality of the returndata
(compare keccak256(ret1) == keccak256(ret2)); update the test around the
try/catch block to perform these two low-level calls and assert both success and
returned data match for the same bytecode input.
In `@test/src/concrete/Extrospect.scanMetamorphicRisk.t.sol`:
- Around line 13-16: The catch branch currently only asserts a revert for
this._libScan(bytecode), which can give false positives if both the reference
and SUT revert for different reasons; change the test to perform low-level calls
for both targets (the reference call and this._libScan(bytecode)), capture their
success flags and returndata, and assert that both the success booleans and the
returned returndata bytes are equal (compare lengths and contents) instead of
only expecting a revert; locate usages of this._libScan and the corresponding
reference call and replace the vm.expectRevert() + catch logic with a direct
comparison of (success, returndata).
🪄 Autofix (Beta)
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
Run ID: c3265e67-e6c6-456c-86a0-0efe9fb457b2
📒 Files selected for processing (19)
.github/workflows/manual-sol-artifacts.yaml.gitmoduleslib/rain.deployscript/Deploy.solsrc/concrete/Extrospect.solsrc/interface/IExtrospectV1.soltest/concrete/ExtrospectEquivalence.soltest/src/concrete/Extrospect.checkCBORTrimmedBytecodeHash.t.soltest/src/concrete/Extrospect.checkNoSolidityCBORMetadata.t.soltest/src/concrete/Extrospect.checkNotEOFBytecode.t.soltest/src/concrete/Extrospect.checkNotMetamorphic.t.soltest/src/concrete/Extrospect.isBeaconImplementationBytecode.t.soltest/src/concrete/Extrospect.isBeaconOwner.t.soltest/src/concrete/Extrospect.isEOFBytecode.t.soltest/src/concrete/Extrospect.isERC1167Proxy.t.soltest/src/concrete/Extrospect.scanEVMOpcodesPresentInBytecode.t.soltest/src/concrete/Extrospect.scanEVMOpcodesReachableInBytecode.t.soltest/src/concrete/Extrospect.scanMetamorphicRisk.t.soltest/src/concrete/Extrospect.tryTrimSolidityCBORMetadata.t.sol
Same 53-byte CBOR-tagged Solidity runtime literal appeared 7 times across 5 files (3 new in this PR, 2 pre-existing upstream). Extract to test/concrete/SolidityCBORFixture.sol as SOLIDITY_CBOR_RUNTIME_FIXTURE so the trim/check semantics are pinned once and a regression in the fixture surface (e.g. solc upgrade that changes the trailer length or CBOR shape) only needs updating in one place.
Two CodeRabbit findings on the manual deploy workflow: 1. Silent fallback was branching on ref to pick between PRIVATE_KEY and PRIVATE_KEY_DEV; we only ever deploy with PRIVATE_KEY, so collapse to that single secret and add a fail-fast guard. 2. Document why DEPLOY_SKIP_SIMULATION is set: Zoltu factory eth_call dry-runs can fail on networks where block-derived state isn't predictable in simulation even when the actual broadcast succeeds.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/manual-sol-artifacts.yaml:
- Around line 2-6: The workflow currently allows workflow_dispatch from any ref
and the deploy job (job name "deploy") runs unguarded; add a guard to prevent
non-main manual deploys by adding an explicit condition on the deploy job (e.g.,
set an if: that checks github.ref == 'refs/heads/main') so the deploy job only
runs when the dispatched ref is main; apply the same change for the second
workflow_dispatch occurrence (the other deploy job block around lines 31-41) so
both manual-dispatch deploys are restricted to main.
In `@test/src/concrete/Extrospect.checkCBORTrimmedBytecodeHash.t.sol`:
- Line 10: Add the standard forge-lint suppression annotation for the
mixed-case-function rule directly above the
libCheckCBORTrimmedBytecodeHashExternal external wrapper so the intentional
mixed-case name is ignored by the linter; locate the declaration of
libCheckCBORTrimmedBytecodeHashExternal and insert the repository's usual
forge-lint suppression comment for mixed-case-function immediately before it.
In `@test/src/concrete/Extrospect.checkNoSolidityCBORMetadata.t.sol`:
- Line 10: The function libCheckNoSolidityCBORMetadataExternal uses mixed-case
"CBOR" in its name and needs a forge-lint suppression; add the standard
forge-lint suppression annotation for mixed-case-function immediately above the
function declaration for libCheckNoSolidityCBORMetadataExternal to silence the
expected warning (follow the same pattern used for other wrappers that also
suppress incorrect-shift and assembly-usage if applicable).
🪄 Autofix (Beta)
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
Run ID: 806e39e4-6f8a-43d7-8bfd-b580f63694b0
📒 Files selected for processing (7)
.github/workflows/manual-sol-artifacts.yamltest/concrete/SolidityCBORFixture.soltest/src/concrete/Extrospect.checkCBORTrimmedBytecodeHash.t.soltest/src/concrete/Extrospect.checkNoSolidityCBORMetadata.t.soltest/src/concrete/Extrospect.tryTrimSolidityCBORMetadata.t.soltest/src/lib/LibExtrospectBytecode.checkNoSolidityCBORMetadata.t.soltest/src/lib/LibExtrospectBytecode.tryTrimSolidityCBORMetadata.t.sol
slither's unused-return detector treats the tuple pass-through (both components re-emitted via the return statement) as discarding the return. Suppress with the standard rain comment + annotation.
Three fixes: - Workflow: gate the deploy job on github.ref == refs/heads/main so a manual dispatch from a non-main ref can't broadcast non-main code with the real key. - Tests: add forge-lint mixed-case-function suppression on the libCheck*External wrappers so the lint policy stays consistent with the rest of the codebase. - Format: forge fmt collapsed a multi-arg encodeWithSelector onto one line.
Zoltu's CREATE2-style factory makes the deployed address a deterministic function of creation bytecode and salt — re-broadcasting from a non-main ref either lands the same bytecode at the same address (no-op once code exists) or deploys a different ref's bytecode to its own deterministic address. The "deploy non-main code with the prod key to the prod address" failure mode CodeRabbit imagined doesn't exist on Zoltu.
2 tasks
This was referenced Aug 18, 2026
Merged
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.
Adds
src/concrete/Extrospect.sol— a parameterless-constructor contract that exposes every library function asexternalso:vm.expectRevertagainst library-internal reverts.expectRevertrequires the revert at a depth below the cheatcode call; an inlined library call shares the test contract's frame, which is why the existingLibExtrospectBytecode.checkNoSolidityCBORMetadata.t.solalready had to add an inline external wrapper.Extrospectmakes per-project wrappers unnecessary.Mirrors the
rain.math.floatpattern (src/concrete/DecimalFloat.sol).Surface exposed
LibExtrospectBytecodeisEOFBytecode,checkNotEOFBytecode,tryTrimSolidityCBORMetadata,checkCBORTrimmedBytecodeHash,checkNoSolidityCBORMetadata,scanEVMOpcodesReachableInBytecode,scanEVMOpcodesPresentInBytecodeLibExtrospectMetamorphicscanMetamorphicRisk,checkNotMetamorphicLibExtrospectERC1167ProxyisERC1167ProxyLibExtrospectERC1967BeaconProxyisBeaconImplementationBytecode,isBeaconOwnerTest plan
Summary by CodeRabbit
New Features
Tests
Chores