From ba290ebbcd91c34729735549c320ac131c0ed4f8 Mon Sep 17 00:00:00 2001 From: David Meister Date: Mon, 21 Sep 2026 11:19:37 +0000 Subject: [PATCH 1/3] test: drop the no-op bytes32(uint256(...)) round trip on topic 0 keccak256 returns bytes32 and topics[0] is bytes32, so the uint256 hop converted a value to another type and straight back. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol | 6 +++--- .../LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol b/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol index ecf9125..7f40323 100644 --- a/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol +++ b/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol @@ -187,7 +187,7 @@ contract LibICloneableFactoryV4CloneDeterministicTest is Test { assertEq(entries.length, 1); assertEq(entries[0].emitter, address(I_CLONE_FACTORY)); - assertEq(entries[0].topics[0], bytes32(uint256(keccak256("NewClone(address,address,address,bytes32,bytes)")))); + assertEq(entries[0].topics[0], keccak256("NewClone(address,address,address,bytes32,bytes)")); assertEq(entries[0].data, abi.encode(address(this), address(implementation), child, salt, data)); } @@ -282,11 +282,11 @@ contract LibICloneableFactoryV4CloneDeterministicTest is Test { assertEq(entries.length, 2); assertEq(entries[0].emitter, address(I_CLONE_FACTORY)); - assertEq(entries[0].topics[0], bytes32(uint256(keccak256("NewClone(address,address,address,bytes32,bytes)")))); + assertEq(entries[0].topics[0], keccak256("NewClone(address,address,address,bytes32,bytes)")); assertEq(entries[0].data, abi.encode(address(this), address(implementation), child, salt, data)); assertEq(entries[1].emitter, child); - assertEq(entries[1].topics[0], bytes32(uint256(keccak256("Initializing(bytes)")))); + assertEq(entries[1].topics[0], keccak256("Initializing(bytes)")); assertEq(entries[1].data, abi.encode(data)); } diff --git a/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol b/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol index fe67bc1..fbc232f 100644 --- a/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol +++ b/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol @@ -364,7 +364,7 @@ contract LibICloneableFactoryV4CloneDeterministicOpenSaltTest is Test { assertEq(entries.length, 1); assertEq(entries[0].emitter, address(I_CLONE_FACTORY)); - assertEq(entries[0].topics[0], bytes32(uint256(keccak256("NewClone(address,address,address,bytes32,bytes)")))); + assertEq(entries[0].topics[0], keccak256("NewClone(address,address,address,bytes32,bytes)")); assertEq(entries[0].data, abi.encode(address(this), address(implementation), child, salt, data)); } From 52127b505a2788475484dd218e9d0840970e2d18 Mon Sep 17 00:00:00 2001 From: David Meister Date: Mon, 21 Sep 2026 11:23:24 +0000 Subject: [PATCH 2/3] test: one CloneFactoryTest base for the factory fixture and the NewClone assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TestCloneFactory immutable and its constructor were copy-pasted in four test contracts, and the NewClone emitter/topic0/data trio — with the event signature as a hand-written literal — in three places. Co-Authored-By: Claude Opus 5 (1M context) --- test/abstract/CloneFactoryTest.sol | 43 +++++++++++++++++++ .../interface/ICloneableV2.initialize.t.sol | 14 +----- ...loneableFactoryV4.cloneAndInitialize.t.sol | 12 +----- ...loneableFactoryV4.cloneDeterministic.t.sol | 22 +++------- ...FactoryV4.cloneDeterministicOpenSalt.t.sol | 18 ++------ 5 files changed, 56 insertions(+), 53 deletions(-) create mode 100644 test/abstract/CloneFactoryTest.sol diff --git a/test/abstract/CloneFactoryTest.sol b/test/abstract/CloneFactoryTest.sol new file mode 100644 index 0000000..10fb104 --- /dev/null +++ b/test/abstract/CloneFactoryTest.sol @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: LicenseRef-DCL-1.0 +// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd +pragma solidity ^0.8.25; + +import {Test, Vm} from "forge-std-1.16.1/src/Test.sol"; + +import {TestCloneFactory} from "test/concrete/TestCloneFactory.sol"; + +/// @title CloneFactoryTest +/// @notice Base for the tests that drive the library through +/// `TestCloneFactory`. Holds the factory instance they all need and the +/// `NewClone` log assertion, so the event signature is written out once. +abstract contract CloneFactoryTest is Test { + /// The `TestCloneFactory` instance under test. Stateless, so reused + /// everywhere. + TestCloneFactory internal immutable I_CLONE_FACTORY; + + constructor() { + I_CLONE_FACTORY = new TestCloneFactory(); + } + + /// Asserts `entry` is the factory's `NewClone` for this deploy: the + /// factory as emitter, the event signature as topic 0, and the five + /// arguments as the data. + /// @param entry The recorded log to check. + /// @param sender The caller the event should name. + /// @param implementation The implementation the clone delegates to. + /// @param clone The deployed clone. + /// @param salt The raw caller salt. + /// @param data The initialization data. + function assertNewClone( + Vm.Log memory entry, + address sender, + address implementation, + address clone, + bytes32 salt, + bytes memory data + ) internal view { + assertEq(entry.emitter, address(I_CLONE_FACTORY)); + assertEq(entry.topics[0], keccak256("NewClone(address,address,address,bytes32,bytes)")); + assertEq(entry.data, abi.encode(sender, implementation, clone, salt, data)); + } +} diff --git a/test/src/interface/ICloneableV2.initialize.t.sol b/test/src/interface/ICloneableV2.initialize.t.sol index eb232f3..8f9e771 100644 --- a/test/src/interface/ICloneableV2.initialize.t.sol +++ b/test/src/interface/ICloneableV2.initialize.t.sol @@ -2,25 +2,15 @@ // SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd pragma solidity =0.8.25; -import {Test} from "forge-std-1.16.1/src/Test.sol"; - import {ICloneableV2} from "src/interface/ICloneableV2.sol"; -import {TestCloneFactory} from "test/concrete/TestCloneFactory.sol"; +import {CloneFactoryTest} from "test/abstract/CloneFactoryTest.sol"; import {TestCloneable, TestCloneableAlreadyInitialized} from "test/concrete/TestCloneable.sol"; /// @title ICloneableV2InitializeTest /// @notice `ICloneableV2`'s two MUSTs on `initialize`, exercised on /// `TestCloneable` and on clones of it that `TestCloneFactory` deployed and /// initialized. -contract ICloneableV2InitializeTest is Test { - /// The `TestCloneFactory` instance under test. Stateless, so reused - /// everywhere. - TestCloneFactory internal immutable I_CLONE_FACTORY; - - constructor() { - I_CLONE_FACTORY = new TestCloneFactory(); - } - +contract ICloneableV2InitializeTest is CloneFactoryTest { /// `initialize` can NOT be called more than once: the factory's call is /// the one that succeeds, a second call on the clone reverts /// `TestCloneableAlreadyInitialized`, and the clone keeps the data the diff --git a/test/src/lib/LibICloneableFactoryV4.cloneAndInitialize.t.sol b/test/src/lib/LibICloneableFactoryV4.cloneAndInitialize.t.sol index cff811e..5b8a703 100644 --- a/test/src/lib/LibICloneableFactoryV4.cloneAndInitialize.t.sol +++ b/test/src/lib/LibICloneableFactoryV4.cloneAndInitialize.t.sol @@ -2,24 +2,16 @@ // SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd pragma solidity =0.8.25; -import {Test} from "forge-std-1.16.1/src/Test.sol"; - import {ICLONEABLE_V2_SUCCESS} from "src/interface/ICloneableV2.sol"; import {DelegatedImplementation, InitializationFailed} from "src/lib/LibICloneableFactoryV4.sol"; -import {TestCloneFactory} from "test/concrete/TestCloneFactory.sol"; +import {CloneFactoryTest} from "test/abstract/CloneFactoryTest.sol"; import {TestCloneable} from "test/concrete/TestCloneable.sol"; import {TestCloneableRawAnswer} from "test/concrete/TestCloneableRawAnswer.sol"; /// @title LibICloneableFactoryV4CloneAndInitializeTest /// @notice How `cloneAndInitialize` guards the implementation and treats each /// `initialize` answer, through both clone entry points. -contract LibICloneableFactoryV4CloneAndInitializeTest is Test { - TestCloneFactory internal immutable I_CLONE_FACTORY; - - constructor() { - I_CLONE_FACTORY = new TestCloneFactory(); - } - +contract LibICloneableFactoryV4CloneAndInitializeTest is CloneFactoryTest { /// Both entry points revert with exactly `expected` when `initialize` /// reverts with (`reverts`) or returns `answer`. function checkBothEntryPointsRevert(bool reverts, bytes memory answer, bytes32 salt, bytes memory expected) diff --git a/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol b/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol index 7f40323..411ddf2 100644 --- a/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol +++ b/test/src/lib/LibICloneableFactoryV4.cloneDeterministic.t.sol @@ -2,7 +2,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd pragma solidity =0.8.25; -import {Test, Vm} from "forge-std-1.16.1/src/Test.sol"; +import {Vm} from "forge-std-1.16.1/src/Test.sol"; import {Clones} from "@openzeppelin-contracts-5.6.1/proxy/Clones.sol"; import {ICloneableV2, ICLONEABLE_V2_SUCCESS} from "src/interface/ICloneableV2.sol"; @@ -13,7 +13,7 @@ import { InitializationFailed, ZeroImplementationCodeSize } from "src/lib/LibICloneableFactoryV4.sol"; -import {TestCloneFactory} from "test/concrete/TestCloneFactory.sol"; +import {CloneFactoryTest} from "test/abstract/CloneFactoryTest.sol"; import {TestCloneable} from "test/concrete/TestCloneable.sol"; import {TestCloneableCallRecorder} from "test/concrete/TestCloneableCallRecorder.sol"; import {TestCloneableFailure} from "test/concrete/TestCloneableFailure.sol"; @@ -26,15 +26,7 @@ import {TestCloneableRevert, TestCloneableRevertInitialize} from "test/concrete/ /// namespacing and the `NewClone` event only exist across an external call. /// The defining property is that the address commits to WHO deployed — /// `(deployer, salt)` — and not to WHAT was initialized. -contract LibICloneableFactoryV4CloneDeterministicTest is Test { - /// The `TestCloneFactory` instance under test. Stateless, so reused - /// everywhere. - TestCloneFactory internal immutable I_CLONE_FACTORY; - - constructor() { - I_CLONE_FACTORY = new TestCloneFactory(); - } - +contract LibICloneableFactoryV4CloneDeterministicTest is CloneFactoryTest { /// The effective `CREATE2` salt is exactly the derivation /// `ICloneableFactoryV4` fixes: /// `keccak256(abi.encode(ICLONEABLE_FACTORY_V4_NAMESPACED_DOMAIN, msg.sender, salt))`. @@ -186,9 +178,7 @@ contract LibICloneableFactoryV4CloneDeterministicTest is Test { Vm.Log[] memory entries = vm.getRecordedLogs(); assertEq(entries.length, 1); - assertEq(entries[0].emitter, address(I_CLONE_FACTORY)); - assertEq(entries[0].topics[0], keccak256("NewClone(address,address,address,bytes32,bytes)")); - assertEq(entries[0].data, abi.encode(address(this), address(implementation), child, salt, data)); + assertNewClone(entries[0], address(this), address(implementation), child, salt, data); } /// An implementation that initializes to a non-success code reverts @@ -281,9 +271,7 @@ contract LibICloneableFactoryV4CloneDeterministicTest is Test { assertEq(entries.length, 2); - assertEq(entries[0].emitter, address(I_CLONE_FACTORY)); - assertEq(entries[0].topics[0], keccak256("NewClone(address,address,address,bytes32,bytes)")); - assertEq(entries[0].data, abi.encode(address(this), address(implementation), child, salt, data)); + assertNewClone(entries[0], address(this), address(implementation), child, salt, data); assertEq(entries[1].emitter, child); assertEq(entries[1].topics[0], keccak256("Initializing(bytes)")); diff --git a/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol b/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol index fbc232f..eac8d9c 100644 --- a/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol +++ b/test/src/lib/LibICloneableFactoryV4.cloneDeterministicOpenSalt.t.sol @@ -2,7 +2,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd pragma solidity =0.8.25; -import {Test, Vm} from "forge-std-1.16.1/src/Test.sol"; +import {Vm} from "forge-std-1.16.1/src/Test.sol"; import {Clones} from "@openzeppelin-contracts-5.6.1/proxy/Clones.sol"; import {ICLONEABLE_V2_SUCCESS} from "src/interface/ICloneableV2.sol"; @@ -16,7 +16,7 @@ import { InitializationFailed, ZeroImplementationCodeSize } from "src/lib/LibICloneableFactoryV4.sol"; -import {TestCloneFactory} from "test/concrete/TestCloneFactory.sol"; +import {CloneFactoryTest} from "test/abstract/CloneFactoryTest.sol"; import {TestCloneable} from "test/concrete/TestCloneable.sol"; import {TestCloneableFailure} from "test/concrete/TestCloneableFailure.sol"; @@ -29,15 +29,7 @@ import {TestCloneableFailure} from "test/concrete/TestCloneableFailure.sol"; /// guarantees. So the two derivations are also tested against each other here, /// including the one squat that the pair of distinct domain tags exists to /// close. -contract LibICloneableFactoryV4CloneDeterministicOpenSaltTest is Test { - /// The `TestCloneFactory` instance under test. Stateless, so reused - /// everywhere. - TestCloneFactory internal immutable I_CLONE_FACTORY; - - constructor() { - I_CLONE_FACTORY = new TestCloneFactory(); - } - +contract LibICloneableFactoryV4CloneDeterministicOpenSaltTest is CloneFactoryTest { /// The effective `CREATE2` salt is exactly the derivation /// `ICloneableFactoryV4` fixes: /// `keccak256(abi.encode(ICLONEABLE_FACTORY_V4_OPEN_SALT_DOMAIN, salt, keccak256(data)))`. @@ -363,9 +355,7 @@ contract LibICloneableFactoryV4CloneDeterministicOpenSaltTest is Test { Vm.Log[] memory entries = vm.getRecordedLogs(); assertEq(entries.length, 1); - assertEq(entries[0].emitter, address(I_CLONE_FACTORY)); - assertEq(entries[0].topics[0], keccak256("NewClone(address,address,address,bytes32,bytes)")); - assertEq(entries[0].data, abi.encode(address(this), address(implementation), child, salt, data)); + assertNewClone(entries[0], address(this), address(implementation), child, salt, data); } /// An implementation that initializes to a non-success code reverts From 3a030776cb855cc9182cfffab7852057ecbf43ff Mon Sep 17 00:00:00 2001 From: David Meister Date: Mon, 21 Sep 2026 11:25:27 +0000 Subject: [PATCH 3/3] test: pin the CloneFactoryTest base to =0.8.25 CLAUDE.md pins tests and concretes at =0.8.25; the float is for src lib/abstract files that downstream consumers compile. Co-Authored-By: Claude Opus 5 (1M context) --- test/abstract/CloneFactoryTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/abstract/CloneFactoryTest.sol b/test/abstract/CloneFactoryTest.sol index 10fb104..5f76552 100644 --- a/test/abstract/CloneFactoryTest.sol +++ b/test/abstract/CloneFactoryTest.sol @@ -1,6 +1,6 @@ // SPDX-License-Identifier: LicenseRef-DCL-1.0 // SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd -pragma solidity ^0.8.25; +pragma solidity =0.8.25; import {Test, Vm} from "forge-std-1.16.1/src/Test.sol";