AMT coverage: g2-libicloneablefactoryv4-predi - #78
Open
thedavidmeister wants to merge 8 commits into
Open
Conversation
`testCheckImplementationCodeEtched` fuzzes arbitrary `code` bytes straight into `vm.etch`. Any draw whose first byte is `0xef` is rejected by the cheatcode itself — `0xef00` is the EOF magic and `0xef01` the EIP-7702 delegation magic, and an `0xef01…` blob that is not exactly the 23-byte designator fails with `Eip7702 is not 23 bytes long`. The test therefore failed on unmutated code for a harness reason, not because `checkImplementationCode` did anything wrong, and did so only on the fuzz seeds that happened to draw such a blob. Assume the byte away so the test covers what it says it covers: any installable non-empty code passes the size-only guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A mutation pass over LibICloneableFactoryV4 with the pre-existing suite left five mutants alive, all in `cloneAndInitialize` — the shared guard/CREATE2/emit/initialize/verify tail that only exists behind the two entry points. Each gets a test that fails on the mutant and passes on the real code. - Guard BEFORE the CREATE2. Only observable when both failure conditions hold at once, so the test builds that state: an ordinary deploy takes the effective salt, the implementation then loses its code, and the caller is told `ZeroImplementationCodeSize` rather than the `CloneDeploymentFailed` a guard placed after the deploy would report. - `create2` value is a literal `0`. The factory is dealt a balance first — it has no payable surface but can still be a `SELFDESTRUCT` or withdrawal recipient — and both the clone's balance and the factory's are asserted. - `initialize` is the first and only call on the fresh proxy, per "MUST NOT call any other functions on the cloned proxy before `initialize`". `TestCloneable` shows only end state, so `TestCloneableCallRecorder` records the selector of every call the proxy receives and the whole sequence is asserted. - `NewClone` is emitted BEFORE `initialize` runs. The recorder logs from inside `initialize`, which is what makes the relative order visible; a count of one entry cannot see it. - A reverting `initialize` bubbles verbatim rather than being swallowed, and leaves the predicted address codeless. `TestCloneableRevert` covers the refusal half of initialization failure; `TestCloneableFailure` already covered the wrong-return-value half. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
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 |
`test/src/**` mirrors `src/**` and holds the `.t.sol` suites. This repo is the library half of the split — there is no `src/concrete/`, the concrete lives in rain.factory.deploy — so `test/src/concrete/` mirrored nothing. Test SUPPORT code (harnesses, mocks, fixtures) belongs outside the mirror, in `test/concrete/`, `test/lib/`, `test/abstract/`, as in rain.deploy and rain.math.float. Pure move plus the import paths that follow it. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntable code Three parallel AMT branches (#76, #77, #78) each grew their own variant of `TestCloneable` because it satisfies neither of `ICloneableV2`'s normative MUSTs and moves in lockstep with the constant the library compares against. Fix it once, here, so the branches converge on one fixture instead of four: - `initialize` can NOT be called more than once. That is the interface's first MUST and no fixture honoured it. - The RECOMMENDED typed overload is present and reverts `InitializeSignatureFn` always, as the interface requires. - The success sentinel is written out from the LITERAL string the interface names, not imported from `ICLONEABLE_V2_SUCCESS`. Importing it put both sides of the library's comparison in lockstep: the constant could drift and every flow test would still pass, because the fixture drifted with it. A third party hard-codes `keccak256("ICloneableV2.initialize")`, so the fixture does too, and every existing flow test now discriminates a drift. Separately, `testCheckImplementationCodeEtched` could fail for a harness reason: `vm.etch` parses a `0xef01` prefix as an EIP-7702 delegation designator and rejects anything that is not exactly the 23-byte designator. EIP-3541 forbids deploying any `0xef`-leading code at all, so such code cannot exist at an implementation address on any chain and the guard is not specified over it; the fuzz domain is narrowed to code that could actually exist. The guard only ever reads code LENGTH, so nothing about the property changes. Gas snapshot regenerated for the extra `SSTORE` the initialization guard costs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # .gas-snapshot # test/concrete/TestCloneableCallRecorder.sol # test/concrete/TestCloneableRevert.sol # test/src/lib/LibICloneableFactoryV4.checkImplementationCode.t.sol # test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol
Three parallel AMT branches each invented their own `ICloneableV2` fixture on top of the two that already existed. The merge from `2026-08-24-test-fixtures-out-of-src-mirror` resolves this branch onto the shared set: - `TestCloneableCallRecorder` and `TestCloneableRevert` move to `test/concrete/`, out of the `test/src` mirror, and are the canonical versions — #76 carries the same two files byte for byte, having dropped its own `TestCloneableEmitter` and `TestCloneableReverter` duplicates, so the two branches merge without touching each other. - Both now return the success sentinel as a LITERAL rather than importing `ICLONEABLE_V2_SUCCESS`, matching `TestCloneable` after the base branch: a fixture that imports the constant the library compares against moves in lockstep with it and cannot discriminate a drift. - The `0xef` fuzz-domain narrowing on `testCheckImplementationCodeEtched` is dropped from this branch. All four AMT branches had patched that same line; it is a `main` defect and now lives there once. Gas snapshot regenerated: it moves for the extra `SSTORE` the base branch's initialization guard costs on every `TestCloneable` clone, as well as for this branch's new tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thedavidmeister
pushed a commit
that referenced
this pull request
Aug 24, 2026
…what they test The merge from `2026-08-24-test-fixtures-out-of-src-mirror` already relocated this branch's fixtures out of the `test/src` mirror, which only mirrors `src`. This finishes the job. Fixtures — three of this branch's five are gone, none of the coverage with them: - `TestCloneableConformant` was `TestCloneable` plus a one-shot init guard and the typed overload. The base branch gives `TestCloneable` both, for the whole suite rather than for two tests, so the second fixture had nothing left to add. `testInitializeOnlyOnce` and `testTypedOverloadRevertsInitializeSignatureFn` move onto it unchanged apart from the error name. - `TestCloneableReverter` and `TestCloneableEmitter` are replaced by `TestCloneableRevert` and `TestCloneableCallRecorder`, byte for byte the same files #78 carries, so the two branches merge without touching each other. - `TestCloneableRevert` and `TestCloneableFailure` stay SEPARATE. A fixture that returns a non-success sentinel cannot also revert with a typed error carrying data; collapsing them would weaken `testInitializeRevertBubblesVerbatim` to a bare decode revert. `testNothingCalledBeforeInitialize` gains a second, stronger oracle in the swap. `TestCloneableEmitter` announced a stray call by emitting from its fallback, so the whole assertion rode on a log count. `TestCloneableCallRecorder` records the SELECTOR SEQUENCE instead, and the test now asserts that exactly one call reached the clone and that it was `initialize(bytes)`. Mutation-checked: a `child.call(...)` inserted into `cloneAndInitialize` between `NewClone` and `initialize` is caught by the selector assertion ("2 != 1") and is INVISIBLE to the log-count assertion, because the recorder's fallback deliberately does not emit. `TestCloneableV1Shaped` was imported by nothing on this branch — a fixture built for a test that was never written. It now has it. `testInitializeSelectorSharedWithV1` pins that V1 and V2 publish the SAME `initialize(bytes)` selector, which is only half a hazard statement: the question it raises is whether a V4 factory silently accepts a legacy implementation and leaves a live clone that was never initialized. It does not — the `bytes32` return decode finds an empty returndata buffer and reverts before the sentinel comparison, with NO revert data, which the test asserts as observed behaviour rather than assuming `InitializationFailed`. Mutation-checked: give the fixture a `bytes32` return and the test fails. Layout, so a file name says what it tests: - `LibPublishedAbi.sol` is test support, not a mirror of any `src/lib` file, so it moves to `test/lib/`. - `ICloneableFactoryV2.sol.t.sol` and `ICloneableFactoryV3.sol.t.sol` lose the doubled extension. - `ICloneableV2.sol.t.sol` becomes `ICloneableV2.initialize.t.sol`, matching the `<unit>.<function>.t.sol` convention `test/src/lib` already uses. Every test in it is about `initialize` or its return sentinel. - `DeprecatedInterfaces.t.sol` splits into `ICloneableV1.t.sol`, `ICloneableFactoryV1.t.sol` and `IFactory.t.sol`, one per file in `src/interface/deprecated/`, which is what the mirror is for. The three tests were independent and shared nothing but the notice, which each file keeps. The `0xef` fuzz-domain narrowing this branch carried is resolved onto the base branch's version of the same line, which now holds it once for all four AMT branches instead of four times. Gas snapshot regenerated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per CodeRabbit on this PR. The finding is correct and the comment on
`vm.assume(code[0] != 0xef)` was overclaiming.
The comment said no implementation on any chain can have `0xef`-leading code.
EIP-3541 does forbid DEPLOYING it, so no CREATE or CREATE2 can produce it — but
EIP-7702 leaves exactly one way an account can hold it anyway: a delegation
designator, `0xef0100 || address`, exactly 23 bytes. `EXTCODESIZE` on a
delegated EOA returns 23, not zero. The assume was silently excluding a case
that is real, behind a comment saying it was not.
`testCheckImplementationCodeEip7702Designator` pins it as a fixed case, since
the fuzz test cannot reach it. It PASSES the guard, which is the part worth
having on the record: a code-SIZE check cannot tell an implementation contract
from an EOA that has delegated, and a delegation is REVOCABLE by the account
holder where deployed code is not. Callers wanting an immutable implementation
do not get that from this guard.
Mutation-checked, not assumed: with the guard mutated to
`code.length == 0 || code[0] == 0xef`, this is the ONLY test in the suite that
fails (42 pass, 1 fail). The fuzz test cannot kill that mutant by construction,
because its assume excludes the input that would.
Scoped honestly in the NatSpec: `foundry.toml` pins `evm_version = "cancun"`,
which predates EIP-7702, so the test asserts that the 23-byte designator is
storable and passes the SIZE check. It does not exercise, and does not claim,
the execution semantics of delegation.
The rest of the assume stands: `0xef`-leading blobs of any OTHER length cannot
exist on any chain, and `vm.etch` rejects them outright ("Eip7702 is not 23
bytes long"), which is what was breaking the fuzz test 8 runs in 8.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # .gas-snapshot
Contributor
Author
|
@coderabbitai review Queuing a review — the original attempt hit the OSS rate limit before reading the diff, so this PR has never actually been reviewed. Orientation: nothing under |
|
|
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.
Adversarial mutation-test coverage for group g2-libicloneablefactoryv4-predi
(29 behaviours across
predictCloneAddress,checkImplementationCode, theshared
cloneAndInitializetail andcloneDeterministic's derivation choice),scanned at
c1c2afd3d88405d3228cb3b21e55c9d63ba8f5be.38 exact-string mutants were probed with
mutation-probeagainst thepre-existing suite first (attribution pass), then again with the new tests.
Final score: 37/38 KILLED, 0 SURVIVED — 32 by pre-existing tests, 5 by the
tests added here. The 38th is uncompilable rather than survivable and is
recorded as EQUIVALENT below.
Adversarial findings filed during this run (
audit+adversarial): #72, #73,#74. #72 turned out to duplicate #61, filed independently by a parallel pass over
the same unit — I could not comment on or close it from this session, so it is
noted here instead; consolidate on #61.
The first commit also fixes the pre-existing test defect reported in #64 and #68.
The baseline was red before anything else could happen
testCheckImplementationCodeEtchedfuzzes arbitrarycodebytes straight intovm.etch. Any draw whose first byte is0xefis rejected by the cheatcodeitself —
0xef00is the EOF magic,0xef01the EIP-7702 delegation magic, andan
0xef01…blob that is not exactly the 23-byte designator fails withEip7702 is not 23 bytes long. It is seed-dependent, so it looked green oftenenough to be missed.
This is not cosmetic: the first probe pass ran on a seed that drew such a blob,
and three mutants —
C16(NewClone after initialize),C17(a stray call on theproxy before initialize) and
C23(a reverting initialize swallowed) — werescored KILLED by a test failing for a reason that had nothing to do with them.
With the flake fixed, all three came back SURVIVED, which is what they really
were. The attribution numbers in this PR are from the repaired baseline.
Behaviour matrix
Verdict legend: pre = killed by a pre-existing test in the attribution pass,
before any test was written here; new = killed by a test added in this PR;
equiv = the mutant cannot exist as compilable code.
predictCloneAddress(address,address,bytes32)cloneCreationCode(implementation)rather than taking a codehashabi.encodePacked(implementation)instead of the creation codetestPredictCloneAddressMatchesOZuint256 -> uint160 -> addresstakes the LOW 20 bytes>> 96first, taking the HIGH 20testPredictCloneAddressIsCreate2Formulaabi.encodePacked, notabi.encodeabi.encodetestPredictCloneAddressMatchesOZfactoryis a parameter, notaddress(this)address(0); A05factory/derivedSaltswappedtestPredictCloneAddressMatchesOZcheckImplementationCode(address)implementation.code.length(EXTCODESIZE)address(this).code.lengthtestCheckImplementationCodeZero== 0(boundary: exactly 0 reverts, exactly 1 byte passes)!= 0; B03 guard never fires; B04< 2testCheckImplementationCodeContract; B03testCheckImplementationCodeZero; B04testCheckImplementationCodeEtchedZeroImplementationCodeSize, typed, no argumentsCloneDeploymentFailedinsteadtestCheckImplementationCodeZero0x00testCheckImplementationCodeEtchedviewviewCREATE2incloneAndInitializetestCloneDeterministicCodeGuardRunsBeforeCreate2cloneAndInitialize(address,bytes32,bytes,bytes32)checkImplementationCodefirst, before any deploytestCloneDeterministicCodeGuardRunsBeforeCreate2cloneCreationCode(implementation)address(this)testCloneDeterministicMatchesPredictassembly ("memory-safe")create2(0, add(cc,0x20), mload(cc), derivedSalt)testCloneDeterministicMatchesPredict,testCloneDeterministicOpenSaltDoesNotConsumeNamespacedSaltcreate2value literal0— no ETH forwarded to the cloneselfbalance(); C041testCloneDeterministicNoEthForwarded; C04testCloneDeterministicMatchesPredictadd(creationCode, 0x20)skips the length wordcreationCodetestCloneDeterministicMatchesPredictmload(creationCode)reads the length word (55)sub(mload(creationCode), 1)testCloneDeterministicOpenSaltDoesNotConsumeNamespacedSaltsalttestCloneDeterministicOpenSaltMatchesPredictchild == address(0)=> revertCloneDeploymentFailed(typed)testCloneDeterministicOpenSaltCallerIndependent; C09testCloneDeterministicSecondDeployRevertspredictCloneAddress(...)instead of revertingtestCloneDeterministicSecondDeployRevertsemit NewClone(msg.sender, implementation, child, salt, data)testCloneDeterministicEventderivedSaltderivedSalttestCloneDeterministicEventsenderismsg.sender(nottx.origin, notaddress(this))tx.origin; C14address(this)testCloneDeterministicEventimplementation/childswappedtestCloneDeterministicEventinitializecalltestCloneDeterministicEventPrecedesInitializeICloneableV2(child).initialize(data), data forwarded verbatiminitialize(""); C19initialize(abi.encode(data))testCloneDeterministicMatchesPredictinitializechild.call("")inserted firsttestCloneDeterministicInitializeIsTheOnlyCall!= ICLONEABLE_V2_SUCCESS=> revertInitializationFailed(typed)testCloneDeterministicInitializeFailureFails; C23testCloneDeterministicInitializeRevertBubblesimplementationtestCloneDeterministicMatchesPredictcloneDeterministic(address,bytes,bytes32)effectiveSalt(namespaced), noteffectiveOpenSalttx.origintestCloneDeterministicSenderScopedEQUIVALENT: B07, dropping
viewfromcheckImplementationCodeRecorded as equivalent, not forced to a kill. Dropping
viewalone changes noruntime behaviour whatsoever: a non-
viewfunction that writes nothing executesidentically. The mutation is observable only at compile time, and the suite
already binds it there —
testCheckImplementationCodeZeroreaches the librarythrough an
external viewwrapper, so the mutant does not compile(
Error (8961): Function cannot be declared as view because this expression (potentially) modifies the state). The probe scores that NO-RUN, which iscorrect: there is no suite run to score. No test was written to chase it,
because no test can distinguish a state the mutant cannot reach.
What the five new tests actually assert
Each one fails on its mutant and passes on real code, on a different observable
VALUE rather than a bare revert:
testCloneDeterministicCodeGuardRunsBeforeCreate2— ordering is invisibleunless both failure conditions hold at once, so the test builds that state: an
ordinary deploy takes the effective salt, then
vm.etch(implementation, "")strips the implementation's code. Guard first gives
ZeroImplementationCodeSize; a guard after the deploy givesCloneDeploymentFailed. Two distinct typed errors, not one revert.testCloneDeterministicNoEthForwarded— deals the factory a fuzzedbalance (it has no payable surface, but can still receive by
SELFDESTRUCToras a withdrawal recipient) and asserts BOTH
child.balance == 0and thefactory's balance unchanged, so neither "the clone got funded" nor "the factory
got drained" passes.
testCloneDeterministicInitializeIsTheOnlyCall—TestCloneableexposesonly end state, so a call the factory made and discarded left no trace at all.
The new
TestCloneableCallRecorderappends the selector of every call theproxy receives, and the test asserts the whole recorded sequence is exactly
[ICloneableV2.initialize.selector].testCloneDeterministicEventPrecedesInitialize— the recorder logs frominside
initialize, which is what makes relative order observable; theexisting
entries.length == 1assertions pin a count and cannot see order.Asserts two entries,
NewClonefrom the factory first,Initializingfrom thechild second, both with full data.
testCloneDeterministicInitializeRevertBubbles— the suite had noimplementation that REFUSES, only one that returns the wrong value. The new
TestCloneableRevertreverts with a typed error carrying itsdata, and thetest asserts that exact error with those exact arguments comes back out of the
factory, plus the predicted address is left codeless.
Harness and evidence
Harness is the repo's own CI (
.github/workflows/rainix-sol.yaml->rainix
rainix-sol-test):forge soldeer installthenforge testinnix develop .#sol-shell.Attribution pass, pre-existing suite only, no test from this PR written yet:
Targeted re-probe of exactly those five, with the new tests:
Full final pass, all 38 mutants:
The one NO-RUN is B07, the EQUIVALENT above. No mutant survives. The mutants
file is a probe artifact and is deliberately not committed.
Nothing was weakened
No existing test was deleted, weakened, or edited to pass under a mutation. The
one edit to an existing test adds a
vm.assumeso the fuzzer stops handingvm.etchinput the cheatcode refuses — it repairs a test that was red onunmutated source, and the repaired test still kills B02, B04 and B05.
Unprobed
None. Every one of the 29 behaviours in the group has at least one mutant with a
recorded verdict.
QA
testCloneDeterministicCodeGuardRunsBeforeCreate2,testCloneDeterministicNoEthForwarded,testCloneDeterministicInitializeIsTheOnlyCall,testCloneDeterministicEventPrecedesInitialize,testCloneDeterministicInitializeRevertBubbles- each was verified to fail under its mutant and pass on unmutated source bymutation-probe, which applies the mutation, runs the whole suite, restores the file byte-exact and names the failing test. Each asserts a different observable VALUE, not a bare revert: two distinct typed errors (ZeroImplementationCodeSizevsCloneDeploymentFailed), both balances after a funded deploy, the whole recorded call-selector sequence on the proxy, two ordered log entries with full data, and the implementation's own error with its exact arguments.src/lib/LibICloneableFactoryV4.sol:151-160checkImplementationCode(implementation)moved below thechild == address(0)block ->testCloneDeterministicCodeGuardRunsBeforeCreate2src/lib/LibICloneableFactoryV4.sol:156create2(0, …)->create2(selfbalance(), …)->testCloneDeterministicNoEthForwardedsrc/lib/LibICloneableFactoryV4.sol:161a discardedchild.call("")inserted before theemit->testCloneDeterministicInitializeIsTheOnlyCallsrc/lib/LibICloneableFactoryV4.sol:161-166emit NewClone(...)moved below theinitializecheck ->testCloneDeterministicEventPrecedesInitializesrc/lib/LibICloneableFactoryV4.sol:164-168theinitializecheck replaced by a low-level call that swallows a revert ->testCloneDeterministicInitializeRevertBubblesB07, droppingview) is uncompilable and recorded EQUIVALENT.ICloneableFactoryV3.cloneDeterministic("MUST callICloneableV2.initializeatomically … MUST NOT call any other functions on the cloned proxy beforeinitializecompletes successfully … MUST ONLY consider the clone successfully created ifinitializereturns the keccak256 hash of the stringICloneableV2.initialize"),ICloneableFactoryV4(both effective-salt derivations pinned to exact bytes,NewClonecarrying the RAW caller salt), and the library's own docs for the guard-then-CREATE2-then-emit-then-initialize order and for each typed error. Expected values are computed independently of the library where a value is involved: the pre-existing prediction tests hold the line against OpenZeppelinClonesas a foreign EIP-1167 implementation and against the longhandCREATE2formula over the EIP's literal bytes; the new tests assert selectors and log topic0 hashes derived from the interface signatures, and balances fromvm.deal, never from the library's own arithmetic.g2-libicloneablefactoryv4-predi; all 29 are covered with a recorded verdict, none UNPROBED. Its first commit also repairs the pre-existing test defect reported in testCheckImplementationCodeEtched fails on unmutated main: vm.etch rejects fuzzed 0xEF-leading code #64 and testCheckImplementationCodeEtched fails on unmutated source when the fuzzer draws 0xEF-prefixed code #68. Refs InitializationFailed is not raised when the implementation does not implement ICloneableV2.initialize — caller gets bare 0x #61 (independently filed duplicate of cloneAndInitialize reverts with empty revert data, not InitializationFailed, when the clone's initialize returns nothing #72).🤖 Generated with Claude Code