From ff64bdcdc7957ea3bcd3e1351e3e64f1fcc3a3f1 Mon Sep 17 00:00:00 2001 From: David Meister Date: Mon, 21 Sep 2026 11:25:19 +0000 Subject: [PATCH] test: pin atomicity by redeploying at the address, not by reading code length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three tests read `predicted.code.length == 0` after a `vm.expectRevert`ed call as evidence that "the address is left free rather than occupied by an uninitialized clone". The EVM discards the reverted frame including the `CREATE2` before that line runs, and `predicted` was codeless beforehand, so the assertion holds for every possible library and cannot fail if atomicity breaks. Each now deploys at that address instead. The namespaced failure test retries with data the implementation initializes successfully on, `data` being outside that derivation. The other two swap the implementation's code, since the clone address commits to the implementation's address and not to its code — the open-salt derivation holds `data` fixed, and `TestCloneableRevert` refuses every input. Co-Authored-By: Claude Opus 5 (1M context) --- ...loneableFactoryV4.cloneDeterministic.t.sol | 30 ++++++++++++++----- ...FactoryV4.cloneDeterministicOpenSalt.t.sol | 14 +++++++-- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol b/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol index ecf9125..cba1db4 100644 --- a/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol +++ b/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol @@ -192,8 +192,13 @@ contract LibICloneableFactoryV4CloneDeterministicTest is Test { } /// An implementation that initializes to a non-success code reverts - /// `InitializationFailed`, so clone-and-initialize stays atomic and the - /// address is left free rather than occupied by an uninitialized clone. + /// `InitializationFailed`, and clone-and-initialize stays atomic: the + /// address is left free rather than occupied by an uninitialized clone, so + /// the same `(deployer, salt)` still deploys there afterwards. `data` is + /// outside the namespaced derivation, so the second attempt reaches the + /// same address with data this implementation initializes successfully on. + /// Reading `predicted.code.length` instead would assert nothing: the revert + /// has already rolled the `CREATE2` back whatever the library did. function testCloneDeterministicInitializeFailureFails(bytes32 notSuccess, bytes32 salt) external { vm.assume(notSuccess != ICLONEABLE_V2_SUCCESS); TestCloneableFailure implementation = new TestCloneableFailure(); @@ -203,7 +208,10 @@ contract LibICloneableFactoryV4CloneDeterministicTest is Test { vm.expectRevert(abi.encodeWithSelector(InitializationFailed.selector)); I_CLONE_FACTORY.cloneDeterministic(address(implementation), abi.encode(notSuccess), salt); - assertEq(predicted.code.length, 0); + assertEq( + I_CLONE_FACTORY.cloneDeterministic(address(implementation), abi.encode(ICLONEABLE_V2_SUCCESS), salt), + predicted + ); } /// A zero-code implementation reverts `ZeroImplementationCodeSize`. @@ -292,10 +300,14 @@ contract LibICloneableFactoryV4CloneDeterministicTest is Test { /// An implementation whose `initialize` REVERTS bubbles that revert /// verbatim — error selector and arguments — rather than being swallowed or - /// re-wrapped, and the clone-and-initialize stays atomic: the predicted - /// address is left codeless, so the salt is still free. `…InitializeFailureFails` - /// covers the other half of initialization failure, where `initialize` - /// returns a non-success value instead of refusing. + /// re-wrapped, and clone-and-initialize stays atomic: the salt is still + /// free, so the same `(deployer, salt)` deploys at the address it always + /// predicted once the implementation at that address initializes instead of + /// refusing. The clone address commits to the implementation's ADDRESS and + /// not to its code, which is what lets the code there be swapped without + /// moving the prediction. `…InitializeFailureFails` covers the other half + /// of initialization failure, where `initialize` returns a non-success + /// value instead of refusing. function testCloneDeterministicInitializeRevertBubbles(bytes32 salt, bytes memory data) external { TestCloneableRevert implementation = new TestCloneableRevert(); @@ -304,7 +316,9 @@ contract LibICloneableFactoryV4CloneDeterministicTest is Test { vm.expectRevert(abi.encodeWithSelector(TestCloneableRevertInitialize.selector, data)); I_CLONE_FACTORY.cloneDeterministic(address(implementation), data, salt); - assertEq(predicted.code.length, 0); + vm.etch(address(implementation), address(new TestCloneable()).code); + assertEq(I_CLONE_FACTORY.cloneDeterministic(address(implementation), data, salt), predicted); + assertEq(TestCloneable(predicted).sData(), data); } /// The prediction reads `deployer`, never the caller: from any account it diff --git a/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol b/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol index fe67bc1..128b030 100644 --- a/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol +++ b/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol @@ -369,8 +369,14 @@ contract LibICloneableFactoryV4CloneDeterministicOpenSaltTest is Test { } /// An implementation that initializes to a non-success code reverts - /// `InitializationFailed`, so clone-and-initialize stays atomic and the - /// address is left free rather than occupied by an uninitialized clone. + /// `InitializationFailed`, and clone-and-initialize stays atomic: the + /// address is left free rather than occupied by an uninitialized clone, so + /// the same `(implementation, data, salt)` still deploys there afterwards. + /// `data` is inside this derivation and cannot change between the attempts, + /// but the clone address commits to the implementation's ADDRESS and not to + /// its code, so the code there is swapped for one that initializes instead. + /// Reading `predicted.code.length` instead would assert nothing: the revert + /// has already rolled the `CREATE2` back whatever the library did. function testCloneDeterministicOpenSaltInitializeFailureFails(bytes32 notSuccess, bytes32 salt) external { vm.assume(notSuccess != ICLONEABLE_V2_SUCCESS); TestCloneableFailure implementation = new TestCloneableFailure(); @@ -381,7 +387,9 @@ contract LibICloneableFactoryV4CloneDeterministicOpenSaltTest is Test { vm.expectRevert(abi.encodeWithSelector(InitializationFailed.selector)); I_CLONE_FACTORY.cloneDeterministicOpenSalt(address(implementation), data, salt); - assertEq(predicted.code.length, 0); + vm.etch(address(implementation), address(new TestCloneable()).code); + assertEq(I_CLONE_FACTORY.cloneDeterministicOpenSalt(address(implementation), data, salt), predicted); + assertEq(TestCloneable(predicted).sData(), data); } /// A zero-code implementation reverts `ZeroImplementationCodeSize`.