chore(pragma): pin the three concrete .sol files to =0.8.25 - #23
chore(pragma): pin the three concrete .sol files to =0.8.25#23thedavidmeister wants to merge 3 commits into
Conversation
The convention is `^` for library and abstract files, `=` for concrete contracts including concrete test mocks. All three concrete .sol files in this repo floated `^0.8.25`; they now pin exactly. `src/lib/LibRainDeploy.sol` is a library that downstream soldeer consumers compile, so it keeps `^0.8.25` — a hard pin there would break a consumer on a different 0.8.x. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Zoltu factory is a CREATE2 proxy with a zero salt, so the address it deploys to is a pure function of the creation code. Pinning the concrete test files to =0.8.25 moves the whole compilation unit from solc 0.8.35 (what ^0.8.25 floats to in the rainix toolchain) down to 0.8.25, which changes MockDeployable's creation code and therefore its deployed address and code hash. Address 0xC24016f2..0xC24016f209562fc151e5Ab7F88694ED5775feb36 becomes 0x1fa1bBf9Cf73B1aCCc1a3D9de5896E81Cd567854 and the code hash becomes 0x6ea525f6523fe148810254f04b9a74a379f59ca0f3a7fa83db90b691c78cc299. Both values are confirmed twice over: cast create2 derives the address from the CREATE2 formula given the factory, a zero salt and the 0.8.25 creation code, and live fork execution deploys to exactly that address. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe deployment test pragma changes from ChangesDeployment test consistency
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
👤 human |
|
Correcting the emphasis of the reject above, because as written it could send the rework the wrong way. The defect is the magic numbers. Not the compiler version. My note led with the suite exercising the library at 0.8.25 rather than 0.8.35, and framed that as the headline. That was wrong. It is not a defect of this PR — it is the ordinary consequence of the convention itself: concrete files pin exactly, libraries float, so tests always compile a library at the pinned version while consumers may compile it anywhere in Read literally, my note invited the rework to preserve 0.8.35 coverage by un-pinning — the opposite of what the convention asks. Do not do that. The actual finding, which stands and is the whole reason this is a reject: the test file hardcodes Derive them instead, and the compiler question stops existing: the expected value recomputes from the creation code under whatever solc is in use, so neither a pragma pin nor a future rainix bump can falsify it. So: land #21 first, keep the pragma pins from this PR, and replace every hardcoded literal with the derivation — except in |
Resolves the conflict in test/src/lib/LibRainDeploy.t.sol by taking main's side in all nine hunks. #21 landed `LibRainDeploy.zoltuAddress`, and main already derives MockDeployable's Zoltu address and codehash from `type(MockDeployable).creationCode` instead of hardcoding them, and moved the concrete mocks to test/concrete/. The hand-updated literals this branch carried are exactly what the send-back rejected, so none of them survive. What survives from this branch is the pragma pin itself: the test file is a concrete contract and pins `=0.8.25`. The three mocks already pin on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/src/lib/LibRainDeploy.t.sol (1)
305-309: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove the hardcoded address to the independent-oracle test.
testDeployZoltustill checks Line 310 with a compiler-dependent literal. UsemockDeployableAddress()in that test. Keep the literal intestZoltuAddressMatchesFactory, where it can independently validate both the factory result andLibRainDeploy.zoltuAddress. This avoids manual updates when compiler settings orMockDeployablebytecode changes.Suggested test split
- // Pinned literal, deliberately not `mockDeployableAddress()`. - // The live factory on the fork is the oracle here. - assertEq(deployed, 0x1fa1bBf9Cf73B1aCCc1a3D9de5896E81Cd567854); + assertEq(deployed, mockDeployableAddress()); function testZoltuAddressMatchesFactory() external { vm.createSelectFork(LibRainDeploy.ARBITRUM_ONE); + address deployed = this.externalDeployZoltu(type(MockDeployable).creationCode); + assertEq(deployed, 0x1fa1bBf9Cf73B1aCCc1a3D9de5896E81Cd567854); assertEq( LibRainDeploy.zoltuAddress(type(MockDeployable).creationCode), - this.externalDeployZoltu(type(MockDeployable).creationCode) + deployed );Also applies to: 523-540
🤖 Prompt for 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. In `@test/src/lib/LibRainDeploy.t.sol` around lines 305 - 309, Update testDeployZoltu to use mockDeployableAddress() instead of the compiler-dependent hardcoded address when checking the deployed result. Keep the pinned literal exclusively in testZoltuAddressMatchesFactory so that test independently compares the factory result with LibRainDeploy.zoltuAddress, including the corresponding assertions in the later referenced test section.
🤖 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.
Outside diff comments:
In `@test/src/lib/LibRainDeploy.t.sol`:
- Around line 305-309: Update testDeployZoltu to use mockDeployableAddress()
instead of the compiler-dependent hardcoded address when checking the deployed
result. Keep the pinned literal exclusively in testZoltuAddressMatchesFactory so
that test independently compares the factory result with
LibRainDeploy.zoltuAddress, including the corresponding assertions in the later
referenced test section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 950a7aec-ef78-48f4-bc56-a9caf9c2b5c7
📒 Files selected for processing (1)
test/src/lib/LibRainDeploy.t.sol
|
🤖 ai:producer |
Closes #22
The convention is
^for library and abstract files,=for concretecontracts including concrete test mocks. All three concrete
.solfiles inthis repo floated
^0.8.25. They now pin exactly.src/lib/LibRainDeploy.sollibrary^0.8.25^0.8.25— untouchedtest/src/lib/LibRainDeploy.t.solcontract^0.8.25=0.8.25test/src/lib/MockDeployable.solcontract^0.8.25=0.8.25test/src/lib/MockReverter.solcontract^0.8.25=0.8.25LibRainDeploy.solis the one file the convention exists to protect. It is alibrary, downstream soldeer consumers compile it from source, and a hard pin
there would break a consumer sitting on a different
0.8.x. It is not in thisdiff at all.
The pin is not cosmetic, and the issue's premise was wrong
#22 predicted that "an exact pin either compiles under the repo's configured
solc or it does not, so this is a change whose correctness the toolchain settles
directly". It compiles fine — and it still breaks seven tests. That is worth
spelling out, because it is the whole substance of this PR.
^0.8.25does not mean 0.8.25. It means "0.8.25 or newer", and forge resolvesit to the newest solc the rainix toolchain ships. On
mainthat is 0.8.35:=0.8.25forces 0.8.25 for the whole compilation unit, which changesMockDeployable's creation code. The Zoltu factory(
0x7A0D94F55792C434d74a40883C6ed8545E406D12) is a CREATE2 proxy with azero salt — its bytecode ends
...80368234f5..., andf5isCREATE2withsalttaken as zero from the stack — so the address it deploys to is a purefunction of the creation code it is handed. Different compiler, different
bytecode, different address.
The test file hardcoded that address in 13 places and its code hash in 5. With
the pragma change alone, seven tests fail:
So the constants are updated alongside the pragmas. They are not guessed —
two independent derivations agree exactly:
0x1fa1bBf9Cf73B1aCCc1a3D9de5896E81Cd567854cast create2 --deployer 0x7A0D94F5… --salt 0x00…00 --init-code <0.8.25 initcode>0x1fa1bBf9Cf73B1aCCc1a3D9de5896E81Cd567854cast keccak <0.8.25 deployedBytecode>0x6ea525f6523fe148810254f04b9a74a379f59ca0f3a7fa83db90b691c78cc299Note which direction this cuts. Before this PR those constants silently track
whichever solc rainix happens to ship — the suite would have gone red on its own
the next time rainix bumped solc, with no commit to blame. The exact pin is what
makes them stable. This PR is not just tidying a caret; it closes a latent
time-bomb in the test suite.
Two consequences a reviewer should rule on rather than skim:
than at whatever is newest. Testing the declared minimum is defensible and is
what the convention implies, but it is a real change in what is covered.
type(MockDeployable).creationCodeplus the CREATE2 formula would remove theduplication and the compiler dependence permanently. That is a test-oracle
refactor, deliberately not bundled into a pragma sweep — same reason All three concrete .sol files float their pragma; only the library is correct #22
itself gave for not sweeping three files into fix(deploy): report the zero address when the Zoltu factory call fails #20. Happy to open it
separately if wanted.
Relationship to #20
#20 is open and adds a fourth concrete file,
test/src/lib/MockAddressRevertingFactory.sol, with a floating pragma. Thatfile is #20's to fix and is not touched here — it does not exist on
main.The two PRs touch disjoint files and land in either order. Whichever lands
second will want a rebuild, since #20 also changes
LibRainDeploy.sol.QA
claimed. The diff against
mainis now exactly one line —test/src/lib/LibRainDeploy.t.solmoves from
pragma solidity ^0.8.25;to=0.8.25;. A pragma is acompile-time directive with no runtime footprint, so no test can observe the
change directly. It is also, on this base, bytecode-neutral: the three
concrete mocks already pin
=0.8.25onmain, and the test file importsthem, so solc already had to resolve the whole compilation unit to 0.8.25.
The pin makes that constraint explicit at the file that states it rather than
inheriting it from an import. The evidence that nothing moved is the suite
itself —
rainix / testis green on the merge commit, including every forktest, with no expected-value edit anywhere in the diff.
mutate. The mutation that mattered was run on the previous head and is why
this PR was sent back: hand-pinning the compiler moved
MockDeployable'sCREATE2 address, 7 tests failed, and the response was to hand-update 18
literals. That version is gone.
mainnow derives the address and codehashfrom
type(MockDeployable).creationCodeviaLibRainDeploy.zoltuAddress(landed in fix(deploy): check the derived Zoltu address before skipping a network #21), so a compiler bump recomputes the expected value instead of
falsifying it, and no literal in this diff can go stale.
^for library and abstract files,=forconcrete ones including test contracts — applied to the one file in this repo
that was still floating.
src/lib/LibRainDeploy.solis a library and keeps^0.8.25; it is absent from the diff. The remaining hardcoded address intestDeployZoltuis deliberately not converted to the derivation: the liveZoltu factory on an Arbitrum fork is the independent oracle there, and taking
the expected value from
zoltuAddresswould check that function againstitself.
.solfiles pin exactly,that
LibRainDeploy.solbe left alone, and that the suite stay green. Allthree hold on this head:
git grep 'pragma solidity'shows=0.8.25fortest/src/lib/LibRainDeploy.t.soland all threetest/concrete/Mock*.sol,^0.8.25forsrc/lib/LibRainDeploy.solonly, and all four checks are green.The linter follow-up All three concrete .sol files float their pragma; only the library is correct #22 mentions stays out of scope.
Verification
Run with the exact rainix pin CI uses —
rainix-sol-test.yamlsetsRAINIX_SHA=53e96a7d0a97d7c7c75c3b2412521324776fdac6and invokes these samecommands:
forge build→Compiling 23 files with Solc 0.8.25→Compiler run successful!forge test→26 passed; 0 failed; 0 skippedforge fmt --check→ clean, exit 0The suite is fork-heavy — most tests
vm.createSelectForkArbitrum One or Base— and it was run against live public RPC endpoints for
arbitrum/base/base_sepolia/flare/polygon. No fork test was skipped; all 26
genuinely executed, including the archive-dependent
testFindDeployBlockZoltuFactoryandtestIsStartBlockAtDeployBlock.Summary by CodeRabbit