feat: rotatable handler variants and CREATE2 deployment scripts by mfw78 · Pull Request #59 · nullislabs/composable-cow

mfw78 · 2026-08-02T00:40:09Z

Handlers whose descriptor can be set after deployment, and the scripts to deploy them.

What

Commitment library holds the {uris, digest, kind} struct, its URI invariants and validation. OrderDescriptor and OrderModule share it instead of carrying a copy each.
Each mixin gains an owned extension in the same file: a setter behind onlyOwner, using Solady's Ownable.
OwnedTWAP, OwnedStopLoss and OwnedGoodAfterTime extend the handler they are named for, so order generation is the same code and only the discovery surface differs.
Both surfaces take a named Commitment.Data rather than three loose values, and Commitment.none() says a surface is uncommitted.
script/deploy_OwnedStack.s.sol deploys via CREATE2, so addresses match across chains:
- DeployMainnetStack deploys the registry and TWAP
- DeployGnosisStack deploys the registry and all three
- predict(settlement, admin) returns every address without deploying
Set TWAP_DESCRIPTOR_URI to an https permalink and the script publishes the TWAP descriptor in the same run. The digest is computed as sha256 over descriptors/TWAP.json in this repository, not supplied, so what goes on chain cannot drift from the generated document. Leave it unset and the handler deploys uncommitted as before.

Why

The descriptor digest is a constructor argument on the immutable handlers, so under CREATE2 their address would depend on a descriptor that cannot be built until the address is known. Deploying uncommitted and calling setDescriptor afterwards breaks that cycle, which is what these variants are for. It also keeps the handler address stable when the descriptor is later rotated.

Testing

197 tests pass. forge fmt --check clean. Descriptors regenerate byte-identically. The deployment guards, the address derivation and the descriptor digest are mutation-checked.

Nothing is deployed by this PR.

Two things to check before broadcasting: SETTLEMENT and ADMIN must match across chains or the handler addresses diverge, so run predict and compare; and publishing the descriptor in the same run needs the deploying key to be ADMIN, since setDescriptor is onlyOwner.

AI Assistance

AI Assistance: Claude Code used for the implementation, the tests and this PR description.

Based on `develop`. **#59 is stacked on this**, so this merges first. Two ABI-neutral token changes to `ERC1271Forwarder.isValidSignature`: `bytes memory` to `bytes calldata`, and `virtual`. Same selector, same external ABI, body untouched. ```diff -function isValidSignature(bytes32 _hash, bytes memory signature) public view override returns (bytes4) +function isValidSignature(bytes32 _hash, bytes calldata signature) public view virtual override returns (bytes4) ``` ## Why A contract co-inheriting this forwarder with another ERC-1271 implementation cannot compile otherwise. Solc forbids co-inheriting public functions whose data locations differ, and the function was not overridable. That is not hypothetical. An owner that wants to answer both the ComposableCow order payload and a direct signature of its own has to implement `isValidSignature` twice over and dispatch between them, which requires overriding this one. Nothing here assumes a `Safe`, and it never did: `isValidSafeSignature` takes one only as a typed address, `_auth` reads the registry's own `roots` and `singleOrders`, `test/ComposableCow.base.t.sol` already has `TestNonSafeWallet`, and `_buildSignature` already has a catch branch commented "Assume a non-Safe wallet". This makes that path usable by an owner that also has signature logic of its own. #60 carried more alongside this and is closed. ## Verification 172 tests pass, `forge build` and `forge fmt --check` clean, descriptors current. No behaviour changes, so no test changes.

#62) Based on `develop`. **#59 is stacked on this**, so this merges first. Drops the `safe-contracts` fork, moves to upstream Safe v1.5.0, bumps `forge-std`, and makes `via_ir` the default. Four commits, readable in order. ## Why - `via_ir` produces smaller bytecode. That matters most for `OwnedTWAP`: at 15,478 bytes it is the only contract with a real EIP-170 ceiling to worry about. - It could not be enabled, and every blocker sat in a dependency rather than in this repository. ## What **1. Vendor the Safe interfaces** - `src` imported the fork for six symbols. All are small, and `Safe` is only ever a **typed address**: `isValidSafeSignature` takes one as a parameter, and `_auth` reads this registry's own `roots` and `singleOrders`. - `src/vendor/Safe.sol` declares them, identically to upstream. `rg 'safe/' src/` now returns nothing. - `ExtensibleFallbackHandler` is not vendored at all. It was only ever asked `supportsInterface`, so `IERC165` says the same thing. **2. Upstream Safe v1.5.0, forge-std v1.11.0, via_ir** - `lib/safe` was `cowdao-grants/extensible-fallback-handler`, pinned by commit on `main`. Its `Safe.sol` uses inline assembly that is not annotated memory-safe, so the IR pipeline cannot allocate its stack: ``` Cannot swap Variable var_data_offset with Variable _1: too deep in the stack by 5 slots No memoryguard was present. ``` - Upstream absorbed the `ExtensibleFallbackHandler` work, and in v1.5.0 every assembly block in `Safe.sol` is `assembly ("memory-safe")`. `lib/safe` is now `safe-global/safe-smart-account` at tag `v1.5.0`, so the repository depends on a tagged release rather than a commit on a fork's branch. - `forge-std` v1.5.1 then hit the same limit in `StdStorage.find`, which reaches every test through `Test.sol`. Bumped to v1.11.0, which also declares `parseJsonKeys` natively, so the locally declared interface in the descriptor test is gone. - With all three cleared, **`via_ir` is the default**. There is one bytecode again: tests, the gas snapshot, the descriptors and whatever gets deployed are all built the same way. The Safe bump is not source compatible: | Change | Files | |--------|-------| | `safe/common/Enum.sol` to `safe/libraries/Enum.sol` | 4 | | `MarshalLib` no longer re-exported by `ExtensibleFallbackHandler.sol` | 1 | | `Safe.sol` no longer re-exports `IERC165` or `Enum` | 1 | | a relative `../lib/safe/contracts/...` import in `deploy_ProdStack` | 1 | - That last one is worth knowing about: it bypassed the remapping entirely, so the fork kept being compiled no matter what `safe/` pointed at. One behavioural difference, accepted knowingly: - The fork replaced the manual calldata offsets in `SignatureVerifierMuxer` with a single `abi.decode`. **Upstream v1.5.0 still computes offsets by hand, so this reverts that change.** It sits on the Safe-owner settlement path. - The other two fork commits do not matter: the `ExtensibleBase` rename is present upstream, and the remaining one only corrects a comment. **3. Regenerate the gas snapshot** - Every one of the 122 shared entries moved, so it is a separate commit. - **The delta should not be read as a `via_ir` result.** It conflates two changes: tests dominated by contract deployment fall sharply, up to 195k, because the bytecode is smaller, while the Safe-based end to end tests rise by around 110k, which is upstream v1.5.0 being different code from the fork. - The median entry moves **+2.72%**, so runtime gas is not uniformly better. | Contract | before | after | | |----------|-------:|------:|-------:| | TWAP | 12,629 | 11,483 | -9.1% | | GoodAfterTime | 9,870 | 9,024 | -8.6% | | StopLoss | 10,183 | 9,360 | -8.1% | | OwnedTWAP | 15,478 | 14,316 | -7.5% | | ComposableCow | 10,659 | 10,047 | -5.7% | **4. Merkle tree shape note (docs only, touches no code)** - An unrelated rider, kept here rather than in its own PR. Say the word and it moves. - Solady's `MerkleTreeLib` does not produce the `leafEncoding: "v1"` shape that `docs/discovery.md` pins: it builds a complete `2n-1` node tree, which diverges from the odd-promotion rule at 5, 7 and 9 leaves (2, 3, 4, 6 and 8 agree, measured). - Nothing is broken by this. `_auth` verifies through `MerkleProofLib.verify`, which is sorted-pair and therefore shape-agnostic, so any self-consistent tree passes. - The trap is the payload rule requiring consumers to recompute `root` from `leaves`: an integrator reaching for `MerkleTreeLib` computes a non-conforming root at those sizes and rejects valid payloads. - Now named alongside OpenZeppelin's `StandardMerkleTree`, which was already called out. ## Testing - 182 tests pass at the tip of #59, 172 here. - `forge fmt --check` clean, descriptors current. - CI does a clean checkout with `submodules: recursive`, so a green run is also the proof that the new submodule pins resolve. - Gas and size numbers above are measured on this branch, not estimated. - The tree shape divergence was measured over leaf counts 2 to 9 rather than reasoned about. ## Note for anyone touching this branch `jj` does not record submodule gitlink changes. Both bumps had to be written with git plumbing, and **`jj git push` on this branch will silently revert `lib/safe` and `lib/forge-std` to the old pins.** Use `git push`. ## AI Assistance AI Assistance: Claude Code used for the dependency migration, the vendored interface file, the gas and size measurements, the merkle shape measurement, and this PR description.

`OrderDescriptor` and `OrderModule` each inherited `BaseConditionalOrder`, so a handler reached that base twice the moment it wanted both, and Solidity then demanded an explicit override of every inherited function. The mixins now inherit only their own interface, and a handler composes them with `BaseConditionalOrder` directly. The two differ in what they commit to and in nothing else, so the commitment itself moves to a `Commitment` library: the `{uris, digest, kind}` struct, the three URI invariants, validation, and the advertised predicate. Each mixin holds its own `Commitment.Data`, so the two occupy distinct slots and are set independently while sharing one implementation. The three paired errors collapse to one set; which surface rejected a write is evident from the function called. Each mixin file then holds two contracts: the read-only one, which is what the existing handlers use, and an owned extension that adds a setter behind `onlyOwner`. Ownership is Solady's `Ownable`. A two-step handover earns its place: the commitment is the only mutable state here, and a mistyped transfer would strand it. Solady inverts the direction relative to OpenZeppelin's `Ownable2Step`, in that the recipient requests and the owner completes, which is the stronger of the two since control can only move to an address that has proven it holds the key. Holding the commitment in `internal` storage rather than immutables is what makes the extension free. The setter writes the slots the read-only accessors already read, so the owned variants override nothing: `OwnedTWAP` carries a constructor and one `supportsInterface`. The previous shape needed three delegating accessor overrides per variant. `OwnedTWAP`, `OwnedStopLoss` and `OwnedGoodAfterTime` extend the handler they are named for, so order generation is the same code, and one owner governs both commitments. Together this is 346 lines to 257, with the duplicated half of two files replaced by one shared implementation. Both surfaces now take `Commitment.Data` rather than three loose values. A handler previously took `(string[], bytes32, PackageKind)` and a rotatable one passed two identical triples to two different bases, with nothing at the call site saying which was the descriptor and which the module. The parameter is now named for its surface, and `Commitment.none()` states that a surface is uncommitted rather than leaving an arbitrary `PackageKind` beside a zero digest to be read as meaningful. `deploy_OwnedStack` holds both deployments over one shared base. `DeployMainnetStack` deploys the registry and TWAP only; `DeployGnosisStack` adds `StopLoss` and `GoodAfterTime`. Each pins its chain id, so a stale RPC URL cannot point it at the wrong network. Preflight also rejects a `SETTLEMENT` with no code, which is the wrong address or the wrong chain and would leave every order unsettleable, and a zero `ADMIN`, which would strand both commitments with no key able to call `setDescriptor`. The checks are split from the environment read so they are testable without `vm.setEnv`, which writes the host process environment and is not rolled back between tests. All three are mutation-checked. Deployment is CREATE2, so the registry and the handlers land on the same addresses on every chain. Nothing in the initcode is chain-specific: `GPv2Settlement` is at one address on every chain CoW deploys to, so the registry's constructor argument does not vary, which fixes the registry address, which in turn fixes each handler's constructor argument. The registry's chain-specific part, the settlement domain separator, is read in the constructor and held as an immutable, so it reaches the deployed code without reaching the address. That is available to these handlers precisely because they deploy uncommitted. On the immutable handlers the descriptor digest is a constructor argument, so the address would depend on a descriptor that cannot be built until the address is known; the `Owned*` variants set theirs afterwards, which breaks the cycle. `predict` returns every address without deploying, and each deployment returns what is already there rather than reverting, so re-running across a set of chains converges instead of failing on the second attempt. `ComposableCow` takes no commitment mixin; it has no descriptor. Descriptor documents regenerate byte-identically. Nothing is deployed by this commit.

lgahdl

A few observations from review, focused on the descriptor-publishing script and the ownership-safety claim in the new Owned* mixins.

lgahdl · 2026-08-06T17:39:25Z

+ *      Ownership is `Ownable2Step`: the commitment is the only mutable state
+ *      here, so a mistyped transfer would strand it. The recipient must accept.


Solady's Ownable (imported here) is not Ownable2Step — it keeps the immediate, single-step transferOwnership(address) live alongside the request/complete handover functions, and nothing in this diff overrides or disables it. So the exact scenario this comment says can't happen — a mistyped transfer stranding the commitment — is still reachable through the ordinary Solady API. test_owned_TransferIsTwoStep only exercises the safe handover path; it never asserts transferOwnership is blocked, because it isn't.

Suggested change

* Ownership is `Ownable2Step`: the commitment is the only mutable state

* here, so a mistyped transfer would strand it. The recipient must accept.

* Ownership uses Solady's two-step handover

* (`requestOwnershipHandover`/`completeOwnershipHandover`), but the

* single-step `transferOwnership` stays live too — a mistyped direct

* transfer can still strand the commitment.

lgahdl · 2026-08-06T17:39:25Z

+ *      Ownership is `Ownable2Step`: the commitment is the only mutable state
+ *      here, so a mistyped transfer would strand it. The recipient must accept.


Same inaccuracy as OrderDescriptor.sol's OwnedOrderDescriptor doc — Solady's Ownable here is not Ownable2Step, and the single-step transferOwnership is still reachable, so a mistyped direct transfer can still strand the module commitment despite this comment's claim.

Suggested change

* Ownership is `Ownable2Step`: the commitment is the only mutable state

* here, so a mistyped transfer would strand it. The recipient must accept.

* Ownership uses Solady's two-step handover

* (`requestOwnershipHandover`/`completeOwnershipHandover`), but the

* single-step `transferOwnership` stays live too — a mistyped direct

* transfer can still strand the commitment.

lgahdl · 2026-08-06T17:39:25Z

+
+        set = true;
+        descriptor = Commitment.Data({
+            uris: uris, digest: sha256(bytes(vm.readFile(TWAP_DESCRIPTOR))), kind: PackageKind.SHA256


descriptor.digest is always sha256(vm.readFile(TWAP_DESCRIPTOR)) — the local file — and uri (from TWAP_DESCRIPTOR_URI) is only used as the advertised string; it's never fetched or compared against that file. A stale or mistyped URL still passes _isHttps and the owner-key check, and the script reports success while publishing a commitment whose advertised URI may not actually serve the committed bytes — which is exactly the drift the PR description says this design prevents. Worth at least a loud console warning that the URI itself is unverified, if fetching it in-script (FFI + curl, or an HTTP cheatcode) isn't practical here.

lgahdl · 2026-08-06T17:39:25Z

+    ///      already carries this exact document.
+    function _publish(OwnedTWAP twap, Commitment.Data memory descriptor) internal {
+        (bytes32 current,) = twap.descriptorCommitment();
+        if (current != descriptor.digest) twap.setDescriptor(descriptor);


The republish-skip only compares digest, not uris. If an operator changes only TWAP_DESCRIPTOR_URI (e.g. moves hosting) while the document content is unchanged, this treats the run as a no-op and never calls setDescriptor — the old, possibly-dead URI stays the only one advertised on-chain even though the operator intended to update it. Consider also comparing the URI (e.g. keccak256(abi.encode(current_uris)) != keccak256(abi.encode(descriptor.uris))), or just always calling setDescriptor and letting the owner absorb the extra gas on a true no-op.

lgahdl · 2026-08-06T17:39:25Z

+            "digest does not match sha256sum of the document"
+        );
+        assertEq(bytes(vm.readFile("descriptors/TWAP.json")).length, 1825, "readFile did not return the file exactly");
+        assertEq(d.digest, sha256(bytes(vm.readFile("descriptors/TWAP.json"))), "digest is not the document's");


This line does exactly the "second sha256(vm.readFile(...))" comparison the doc comment above (line 50) says was deliberately avoided ("rather than against a second sha256(vm.readFile(...)) in the test, which would only prove the test and the script agree with each other"). Either drop this assertion to match the stated design, or reword the comment to say both checks are intentional (one against the pinned literal, one against the live file).

lgahdl · 2026-08-06T17:39:25Z

+    }
+}
+
+contract OwnedStopLoss is StopLoss, OwnedOrderDescriptor, OwnedOrderModule {


The contract-level doc above OwnedTWAP (line 15, "The handlers with rotatable commitments...") is clearly meant to describe all three variants, but NatSpec only attaches to the immediately following declaration — OwnedStopLoss and OwnedGoodAfterTime carry no doc comment of their own, so a docgen run leaves them undocumented.

Suggested change

contract OwnedStopLoss is StopLoss, OwnedOrderDescriptor, OwnedOrderModule {

/// @dev See {OwnedTWAP}.

contract OwnedStopLoss is StopLoss, OwnedOrderDescriptor, OwnedOrderModule {

mfw78 force-pushed the feat/admin-commitments branch from 137c2c3 to bc6e2c4 Compare August 2, 2026 00:56

mfw78 changed the title ~~feat: rotatable commitments and a Gnosis integration deployment~~ refactor!: compose the commitment mixins, add rotatable handlers and a Gnosis deployment Aug 2, 2026

mfw78 force-pushed the feat/admin-commitments branch from bc6e2c4 to e2a7b50 Compare August 2, 2026 01:17

mfw78 changed the title ~~refactor!: compose the commitment mixins, add rotatable handlers and a Gnosis deployment~~ feat: owned handler variants and a Gnosis integration deployment Aug 2, 2026

mfw78 force-pushed the feat/admin-commitments branch 2 times, most recently from 1edb708 to d93ae4f Compare August 2, 2026 02:00

This was referenced Aug 2, 2026

feat: EIP-7702 account, proxy delegation, and ERC-1271 without a Safe #60

Closed

refactor: make ERC1271Forwarder.isValidSignature overridable #61

Merged

Base automatically changed from feat/order-descriptors to develop August 2, 2026 11:42

mfw78 force-pushed the feat/admin-commitments branch from d93ae4f to 1047713 Compare August 2, 2026 12:12

mfw78 changed the base branch from develop to refactor/forwarder-overridable August 2, 2026 12:12

Base automatically changed from refactor/forwarder-overridable to develop August 2, 2026 12:28

mfw78 mentioned this pull request Aug 2, 2026

build!: upstream Safe v1.5.0, forge-std v1.11.0, and via_ir by default #62

Merged

mfw78 force-pushed the feat/admin-commitments branch from 1047713 to c211b8a Compare August 2, 2026 23:27

mfw78 changed the base branch from develop to build/vendor-safe-via-ir August 2, 2026 23:27

mfw78 force-pushed the feat/admin-commitments branch 4 times, most recently from ba32f78 to acbd222 Compare August 3, 2026 02:48

mfw78 force-pushed the build/vendor-safe-via-ir branch from 47f68b9 to 398dc6e Compare August 3, 2026 02:48

mfw78 force-pushed the feat/admin-commitments branch from acbd222 to fb8c7be Compare August 3, 2026 02:54

mfw78 force-pushed the build/vendor-safe-via-ir branch from 398dc6e to 5218889 Compare August 3, 2026 02:54

mfw78 force-pushed the feat/admin-commitments branch from fb8c7be to b7755e7 Compare August 3, 2026 03:11

Base automatically changed from build/vendor-safe-via-ir to develop August 3, 2026 04:20

mfw78 force-pushed the feat/admin-commitments branch 3 times, most recently from d7ca770 to f5b1b3f Compare August 3, 2026 11:40

mfw78 changed the title ~~feat: owned handler variants and a Gnosis integration deployment~~ feat: rotatable handler variants and CREATE2 deployment scripts Aug 3, 2026

mfw78 force-pushed the feat/admin-commitments branch 2 times, most recently from 2398211 to 926e36a Compare August 3, 2026 12:15

mfw78 force-pushed the feat/admin-commitments branch from 926e36a to f92ddd2 Compare August 3, 2026 12:18

lgahdl reviewed Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat: rotatable handler variants and CREATE2 deployment scripts - #59

feat: rotatable handler variants and CREATE2 deployment scripts#59
mfw78 wants to merge 1 commit into
developfrom
feat/admin-commitments

mfw78 commented Aug 2, 2026 •

edited

Loading

Uh oh!

lgahdl left a comment

Uh oh!

lgahdl Aug 6, 2026

Uh oh!

lgahdl Aug 6, 2026

Uh oh!

lgahdl Aug 6, 2026

Uh oh!

lgahdl Aug 6, 2026

Uh oh!

lgahdl Aug 6, 2026

Uh oh!

lgahdl Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		* Ownership is `Ownable2Step`: the commitment is the only mutable state
		* here, so a mistyped transfer would strand it. The recipient must accept.

- *      Ownership is `Ownable2Step`: the commitment is the only mutable state
- *      here, so a mistyped transfer would strand it. The recipient must accept.
+ *      Ownership uses Solady's two-step handover
+ *      (`requestOwnershipHandover`/`completeOwnershipHandover`), but the
+ *      single-step `transferOwnership` stays live too — a mistyped direct
+ *      transfer can still strand the commitment.

	contract OwnedStopLoss is StopLoss, OwnedOrderDescriptor, OwnedOrderModule {
	/// @dev See {OwnedTWAP}.
	contract OwnedStopLoss is StopLoss, OwnedOrderDescriptor, OwnedOrderModule {

Uh oh!

Conversation

mfw78 commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Testing

AI Assistance

Uh oh!

lgahdl left a comment

Choose a reason for hiding this comment

Uh oh!

lgahdl Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

lgahdl Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

lgahdl Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

lgahdl Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

lgahdl Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

lgahdl Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mfw78 commented Aug 2, 2026 •

edited

Loading