feat: dual-path architecture with order manifest support [tracking] - #1
feat: dual-path architecture with order manifest support [tracking]#1mfw78 wants to merge 25 commits into
Conversation
Enable require(condition, CustomError()) syntax for cleaner error handling.
BREAKING CHANGE: Replace getTradeableOrder with generateOrder/poll split - Rename getTradeableOrder to generateOrder as single source of truth - Add generateOrder to IConditionalOrder base interface - Rename PollTryAtEpoch to PollTryAtTimestamp for clarity - Remove PollNever error (use OrderNotValid instead) - Add IConditionalOrderGenerator with poll(), getNextPollTimestamp(), describeOrder() - Add PollResultCode enum: SUCCESS, PARTIALLY_FILLED, FILLED, WAIT_TIMESTAMP, WAIT_BLOCK, TRY_NEXT_BLOCK, INVALID - Add PollResult struct with order, scheduling hints, and fill amount - Clean up documentation comments
- Add verify() that calls generateOrder() and validates hash - Add poll() that wraps generateOrder() with try/catch for structured results - Add error decoding for OrderNotValid, PollTryNextBlock, PollTryAtTimestamp, PollTryAtBlock - Add default getNextPollTimestamp() returning POLL_AT_VALIDTO (0) - Add default describeOrder() returning generic message - Define POLL_AT_VALIDTO and POLL_NEVER constants for scheduling hints - Use string constant INVALID_HASH for error messages
Add filledAmount(bytes calldata orderUid) for querying order fill status.
- Extend BaseConditionalOrder instead of IConditionalOrderGenerator - Rename getTradeableOrder to generateOrder - Use require(condition, CustomError()) syntax - Use string constants for error messages - Override getNextPollTimestamp() for multi-part scheduling - Override describeOrder() for human-readable status - Clean up documentation comments
- Extend BaseConditionalOrder instead of IConditionalOrderGenerator - Rename getTradeableOrder to generateOrder - Use require(condition, CustomError()) syntax - Use string constants for error messages - Override getNextPollTimestamp() returning POLL_NEVER for single-shot - Clean up documentation comments
- Extend BaseConditionalOrder instead of IConditionalOrderGenerator - Rename getTradeableOrder to generateOrder - Use require(condition, CustomError()) syntax - Use string constants for error messages - Override getNextPollTimestamp() returning POLL_NEVER for single-shot - Clean up documentation comments
- Extend BaseConditionalOrder instead of IConditionalOrderGenerator - Rename getTradeableOrder to generateOrder - Use require(condition, CustomError()) syntax - Use string constants for error messages - Clean up documentation comments
- Extend BaseConditionalOrder instead of IConditionalOrderGenerator - Rename getTradeableOrder to generateOrder - Use require(condition, CustomError()) syntax - Use string constants for error messages - Clean up documentation comments
- Refactor isValidSafeSignature to call handler.verify() directly - Refactor getTradeableOrderWithSignature to use handler.poll() - Add fill detection via GPv2Settlement.filledAmount() - Return PARTIALLY_FILLED or FILLED based on order kind (sell/buy) - Add checkOrder() for quick tradeable status check - Store settlement contract reference for fill queries - Use require(condition, CustomError()) syntax - Clean up documentation comments
Remove unused comments.
- Clean up unused imports and comments - Update for new interface signatures
- Add test handlers for each error type (OrderNotValid, PollTryNextBlock, etc.) - Update TestConditionalOrder to extend BaseConditionalOrder - Rename getTradeableOrder to generateOrder in mocks - Update for PollResult return type from getTradeableOrderWithSignature
- Test error decoding for OrderNotValid, PollTryNextBlock, PollTryAtTimestamp, PollTryAtBlock - Test verify() hash validation - Test that getTradeableOrderWithSignature uses poll() internally - Fuzz tests for error message propagation
- Rename test_getTradeableOrder_* to test_generateOrder_* - Update for PollResult return type from getTradeableOrderWithSignature - Update expected error types (PollTryAtTimestamp vs OrderNotValid) - Update fuzz bounds for simulate tests - Clean up documentation comments
- Document settlement path vs polling path separation - Add PollResultCode semantics table with PARTIALLY_FILLED and FILLED - Document fill detection via GPv2Settlement - Add polling path diagram with fill checking flow - Document nextPollTimestamp semantics - Add implementation checklist for new order types
Restructure validToBucket() to calculate bucket number first, then multiply. Same behavior, clearer intent, no lint warning.
Add out/ to .gitignore to exclude Foundry build artifacts.
Introduces a new interface enabling enumeration of all discrete orders that a conditional order will produce. Useful for analytics, UI previews, and order lifecycle tracking. - Cardinality enum: FINITE, BOUNDED, UNBOUNDED - ManifestInfo struct for high-level order count information - ManifestEntry struct with order details and validity window - Paginated getManifestPage() for efficient enumeration
Provides a default single-shot manifest implementation: - getManifestInfo() returns FINITE with totalOrders: 1 - getManifestPage() wraps generateOrder() for a single entry - Adds IOrderManifest to supportsInterface() Order types can override these for multi-part or unbounded orders.
TWAP: - FINITE cardinality with n parts - Full pagination support for all TWAP parts - Consolidated helpers for order construction StopLoss: - Custom manifest showing order structure even when strike not reached - Helper to check strike condition without reverting GoodAfterTime: - Sets validFrom to startTime for proper scheduling display TradeAboveThreshold: - Shows order structure even when below threshold PerpetualStableSwap: - UNBOUNDED cardinality with hasMore always true
- Emit ConditionalOrderRemoved when orders are deauthorized via remove() - Update contract natspec to reflect ERC-1271 wallet support
27 tests covering: - BaseConditionalOrder default manifest behaviour - TWAP manifest with pagination and timing verification - StopLoss manifest with oracle condition checking - GoodAfterTime manifest with validFrom handling - TradeAboveThreshold manifest with threshold states - PerpetualStableSwap unbounded manifest - ConditionalOrderRemoved event emission
Updates architecture documentation to reflect: - ERC-1271 wallet support (not just Safe wallets) - ERC1271Forwarder integration pattern - IOrderManifest interface and implementation details - ConditionalOrderRemoved event Adds comprehensive breaking changes section documenting all interface and contract changes from cowprotocol/composable-cow upstream: - IConditionalOrder: PollTryAtEpoch renamed, PollNever removed - IConditionalOrderGenerator: getTradeableOrder() replaced by poll() - ComposableCoW: return type and fill status changes - BaseConditionalOrder: manifest support added
anxolin
left a comment
There was a problem hiding this comment.
Nice to see you are taking further the learnings from building the indexers to give the composable cow handlers more semantics.
Do you think we can come up with a simpler way to automatically decode the static input? (so UIs and the indexer have an easier time explaining the order)
We have this thing that uses the SDK, but requires you to register the handler.
https://sdk-tools.cow.fi
| } | ||
| ``` | ||
|
|
||
| ### PollResultCode Semantics |
There was a problem hiding this comment.
I like the general idea, but It feels we mix some concepts inside PollResultCode
I assume the code is about some human readable message that is nice for debugging.
But for example, what if its PARTIALLY_FILLED our order from last poll but we want to also return WAIT_TIMESTAMP for example?
I understand poll result about the generation of a discrete order from the programmatic order. So I don't get too much SUCCESS , PARTIALLY_FILLED, FILLED, and instead should just be POST_NEW_ORDER or similar.
This way, you either get INVALID or it signals to wait for block/timestamp or it tells you to post an order.
There was a problem hiding this comment.
Maybe I get FILLED too, as a kind of equivalent to INVALID('already filled'), but I don't get PARTIALLY_FILLED
There was a problem hiding this comment.
PollResultCode does contain additional information, and nominally there is a discrete order that is authorised if the PollResult struct contains a PullResultCode of SUCCESS, PARTIALLY_FILLED, or FILLED.
In these cases, the nextPollTimestamp would then contain the timestamp as to when to next poll the contract for a discrete order. Semantically, type(uint256).max aligns with similar semantics in that setting type(uint32).max is the settlement contract's preferred method of invalidation (nothing can happen past this time as its the end type bounds).
Given this, there may be some duplication between waitUntil and nextPollTimestamp and we could most probably compress these down into the same part of the struct, though having waitUntil sounds nicer, though no strong opinions there.
There was a problem hiding this comment.
Sorry, I still don't get it.
Watch tower will poll, potentially every block.
Each poll will return a PollResultCode. Its possible, that is not time to trade (WAIT_TIMESTAMP/WAIT_BLOCK/TRY_NEXT_BLOCK/INVALID) or its time to trade.
If its time to trade, you are saying you want to return some order data from the handler, but also make that handler responsible of deriving the orderUid and checking in the settlement the traded amounts, so you can decided if its
- SUCCESS: Untouched order, potentially needs to be created
- PARTIALLY_FILLED: We can assume it was created. Potentially watch tower doesn't need to do anything
- FILLED: We can assume it was created. Watch tower needs to stop caring about the order
Is this your idea behind the enum?
There was a problem hiding this comment.
It is to make allowance and present to a possible UI that if the ConditionalOrderParams are indexed, a dapp could subsequently do the poll itself and determine the state of the order, potentially reducing load on indexers and shifting load to RPC endpoints.
And yes, the breakdown in all cases is similar to what you mention with some nuances:
SUCCESS: Order has been yielded and should be submitted by the off-chain indexer to the API.PARTIALLY_FILLED: Discrete is already in the order book, as evidenced by being partially filled. No need for the off-chain indexer to do anything for this specific discrete order. Off-chain indexer is to respectnextPollTimestampwhich has semantics to indicate if it should do anything, or cease monitoring theConditionalOrder.FILLED: Same semantics asPARTIALLY_FILLED, just with the difference that the order has been completed filled.
There was a problem hiding this comment.
Settled in #16 (verdict/fill split); lands with the poll-interface and registry slices.
| ### Single-Shot Orders (StopLoss, GoodAfterTime) | ||
|
|
||
| ```solidity | ||
| function generateOrder(...) public view returns (GPv2Order.Data memory) { |
There was a problem hiding this comment.
Probably not worth it, but do you think there's any value on letting a handler generate more than one order per block?
generateOrder could take a index param , and return the order and a boolean indicating if there's more orders to be generated.
This way you call it once, count=0 and if there's more orders (moreOrders=true), you call again with count=1 and so forth
There was a problem hiding this comment.
A simple salt can be used so that multiple orders can be done in the same block. This is supported already at the ComposableCoW level, above the handler.
There was a problem hiding this comment.
Not sure I get this, these are the params
struct ConditionalOrderParams {
IConditionalOrder handler;
bytes32 salt;
bytes staticInput;
}
When we create a single order, we create composableCow.create(..) and give this info. So SALT is given.
What I'm asking here, is, imagine we want to make a contract that in each polling creates 2 orders.
For the same handler/salt/staticinput poll, it will return
- Call 1: [order1, hasMoreOrders=true]
- Call 2: [order2, hasMoreOrders=false]
This way conditional order has a way to yield more than one order
There was a problem hiding this comment.
Fair in that the salt is a given. What is a concrete example of a conditional order type that would yield multiple discrete orders such as this?
There was a problem hiding this comment.
Not saying we should do it, but assuming each block generates only one order is arbitrary
- Multi-currency-DCA: Buys you each month automatically a bunch of tokens
- Simplified AMM? Creates multiple buy and sell orders
- Go long and take profit: Sell something , and creates a limit order to take profit once the bought token appreciates
There was a problem hiding this comment.
Good examples — opened #17. Short version: offchainInput-keyed orders + manifest enumeration already cover these; proposing we document the pattern rather than change the signature.
| |-------|---------| | ||
| | `0` | Use `order.validTo + 1` as default | | ||
| | `> 0` | Poll at this specific timestamp | | ||
| | `type(uint256).max` | Final order, stop polling after fill | |
There was a problem hiding this comment.
not strong preference, but wouldn't be slightly more natural to make
0--> stop polling1--> validTo+1
There was a problem hiding this comment.
I do prefer the semantics of having "stop" being indicated by the "maximum" timestamp (nothing can happen beyond that point of time if that's the time's bounds), ie. with the purpose / point as outlined in the previous comment.
There was a problem hiding this comment.
My thinking was, watch-tower will probably stop watching if the time is in the past. So 0 was saying, don't poll any more. But im fine with yours
|
|
||
| ## Order Type Patterns | ||
|
|
||
| ### Single-Shot Orders (StopLoss, GoodAfterTime) |
There was a problem hiding this comment.
Can we make a simpler alternative way for these orders?
It has been once of the biggest criticism of composable cow.
It would be a scape hatch, for example
- A method in composable cow contract
createSingleShotOrder(order) - The method emits an event that is picked by composable cow
- The owner of the order is a random contract which implements 1271
- This contract doesn't need to do the delegation to composable cow at all
- Composable cow post the order to the API (if ready), if not, tries on every block
What this allow is to make a contract that creates the order by calling this simple method with the order you want to place. It allows a trivial way to integrate.
I see that someone could spam with this (if we keep trying forever), however the indexer can have some policies to pause polling for some orders after a while. Even if we decide to drop it after 30min is already super useful for quick integrations.
There was a problem hiding this comment.
ComposableCoW was never designed inherently to support single-shot, and these were somewhat "wedged" into the design in retrospect. It was designed to support primarily generational conditional orders that generate multiple discrete orders.
I don't think that we should be opening some escape hatch mechanism like this, and perhaps should drive more of a defining line that separates multi-discrete order generators and single-shot. The concern with single-shot has always been with trying to define a nullifier that ensures the single-shot property. For example, StopLoss will continue to generate discrete orders so long as the conditions are met, even if the position has already been sold, and so to handle this, it was often done that there would be some intermediate contract whose job was specifically just to hold the funds for that purpose (this is really a mess, and IMO having specific contracts spun up for specific orders such as this is more indicative of an architectural smell as opposed to a sound system).
I would think that there may be scope available for the single-shot conditional order mechanism to be refined / clarified with a combination of indexing work + generalised wrappers, so as to be able to then ensure that some requisite post-hook has run, adjusting state to nullify the single-shot conditional order, though I would determine generalised wrapper implementation for such to be out-of-scope on the current grant (but indexing / architectural design to be within scope).
There was a problem hiding this comment.
Not sure I got all this.
The point is, currently is super complex for a smart contract to create an order.
They need to use ComposableCoW, which has a lot of indirection built in and people struggle to understand.
So for the simpler cases, of a SC creating an order, and a watch tower picking it, would be nice to have a contract that receives the params, emits the event, and someone creates the order for them.
It doesn't need to be in Composable CoW it can be an independent contract, I just thought watch-tower could index and post order for both flows.
If this is out of scope, fine, I just bring it up as it has been a feedback, and if you are modifying the contract, which will require re-audit and has breaking changes, might be worth it to add this alternative simpler flow.
| return buildPartOrder(currentPart); | ||
| } | ||
|
|
||
| function getNextPollTimestamp(...) external view returns (uint256) { |
There was a problem hiding this comment.
i love moving the polling logic to the handler. Not sure people will put the effort into optimising for this in the contract, but it really makes the indexers simpler.
| | Cardinality | Description | Example | | ||
| |-------------|-------------|---------| | ||
| | `FINITE` | Known fixed number of orders | TWAP with n parts | | ||
| | `BOUNDED` | Upper bound known; actual count is dynamic | Future order types | |
There was a problem hiding this comment.
I didn't get exactly what you mean? can you give one example
There was a problem hiding this comment.
Cardinality defines the number in a set. Let's consider some hypothetical "range trading" conditional order that trades for side-ways pairs. It may attempt to do so every n period, but only if certain conditions are established (price at certain range), for some time t. In which case, the upper bound (cardinality) for this conditional order may be t / n, but the price case may not always be met, so it may be less than t / n.
There was a problem hiding this comment.
Maybe what confuses me is that BOUNDED is by defiition FINITE.
So, I suppose we could add a 'Metadata' abstract contract interface, but this would present only a string-based representation of the conditional order. Nonetheless, this is the nature of ABI and IMO SDK interfaces coupled with dynamic discovery method to retrieve some application specific module that is capable of decoding (this would then allow, for example, a handler to register via ENS the location of their decoder) is the way to go. Such design is non-trivial, but could be included from an architectural perspective so that some discovery mechanism was able to be implemented at a later date. |
What about we force handlers to implememt: function staticInputAbi() external pure returns (string memory);This way TWAP can return the string: TWAPOrder(address sellToken,address buyToken,address receiver,uint256 partSellAmount, ...)Any web3 lib could use this and the static input and make sense of it |
This would have to take a function staticInputAbi(bytes calldata) external pure returns (string memory);The interface would make no enforcement at a smart contract level as to what the string itself would return, other than that the developer should "convey a human readable understanding of the static input". |
|
|
||
| ## Order Type Patterns | ||
|
|
||
| ### Single-Shot Orders (StopLoss, GoodAfterTime) |
There was a problem hiding this comment.
Can you confirm if the single shot order enforcement is more flexible or if the same pattern will be used -- handler must return a constant order?
In my opinion single-shot orders handlers should be more easier / more flexible to create. Stop loss is a good example that you must hard-code the entire order to make it work (so you can't calculate the amounts based on current price + slippage for example). The current interface limits the flexibility of single shot orders a lot in my understanding.
I don't have full context on how hard would be possible to create an easier way to do it with this new interface. Since the pooling and verifier interfaces changed, my understand is that this could be solved by invalidating the composable cow after its verification. This would only work for fill or kill orders but it still looks like a good feature from my point of view.
There was a problem hiding this comment.
I don't have full context on how hard would be possible to create an easier way to do it with this new interface. Since the pooling and verifier interfaces changed, my understand is that this could be solved by invalidating the composable cow after its verification. This would only work for fill or kill orders but it still looks like a good feature from my point of view.
As isValidSignature is view only, it is not possible to modify any state after its verification. The only method to modify any state is by reference to a post-hook, which then runs into authorisation / griefing attack problems (anyone can execute the post-hook that does the voiding action from the HooksTrampoline).
Essentially I think that with the difficulty associated with this, it's more than likely that this kind of design is an anti-pattern when it comes to CoW Protocol in its current incarnation. A much simpler solution, albeit one that requires the use of gas, is to have a conditional order that is triggered via account abstraction or similar, which does setPreSignature and then subsequently voids the conditional order atomically.
There was a problem hiding this comment.
As isValidSignature is view only, it is not possible to modify any state after its verification. The only method to modify any state is by reference to a post-hook, which then runs into authorisation / griefing attack problems (anyone can execute the post-hook that does the voiding action from the HooksTrampoline).
This could be convined with Generalise Wrappers to make it work.
We add a wrapper hint to a contract that, marks the order as traded after the settlement. The composable cow handler, only allows to trade if we haven't traded a similar order.
There was a problem hiding this comment.
Given the advent of the wrappers, does this create any technical debt on the indexing methods that have been proposed in this PR?
There was a problem hiding this comment.
I don't think so. The indexer would pick order1 post it to the API, while in-flight, will pick a second order order2, because the 1st one is not executed, it will post it to the API too.
The API accept both, they both are accepted by the contract.
The first one executing, will automatically invalidate the other. The order will remain in the orderbook until expiration.
Completes the `out/` untracking: ignore the directory and remove generated artifacts from version control. Artifacts are reproducible from source via `forge build`. Part of #1.
- Pin solc 0.8.30 and `evm_version = "cancun"` (PUSH0 + MCOPY codegen; supported on all deployment chains) - Add `foundry.lock` for reproducible builds Part of #1.
The tree mixed `///` and `/** */` NatSpec, and in sixteen files both forms appeared side by side. Standardise on the block form: - 57 declaration-level `///` runs converted to `/** */` across `src`, `test` and `script` - block form was already the majority (85 of 142 declaration-level doc comments) and is what the newer interface and handler work in this stack already uses - records the rule in `docs/style.md` so it does not drift again Deliberately out of scope: comments **inside** a function body. solc ignores NatSpec tags there, so those are implementation comments rather than documentation, and expanding a one-line note into a three-line block would make the code harder to read. There are 16 of them and they are handled separately at the tip of the stack, so that PR can be dropped independently. One case needed care: a `solhint-disable-next-line` directive sat in the middle of a `///` run in `GPv2TradeEncoder.sol`. Converting naively split the header into two blocks, which silently orphans the leading one, because only the block immediately preceding a declaration is NatSpec. The halves are rejoined and the directive dropped (solhint is not configured or run anywhere in this repo). ## How to test The change is comment-only, which is machine-checkable: ```sh git diff chore/solc-0830-toolchain..style/natspec-block-form -U0 \ | grep '^[+-]' | grep -v '^\(+++\|---\)' \ | grep -v '^[+-][[:space:]]*\(///\|/\*\*\|\*/\|\*\)' ``` That prints nothing: no non-comment line is touched. `forge build` and `forge test` are green (74 passing). Part of #1.
Replaces the `mfw78@rndlabs.xyz` address with `mfw78@nxm.rs` in the NatSpec `@author` tags across 24 files in `src`, `test` and `script`. Left alone deliberately: - the `github.com/rndlabs/safe-contracts` reference in the README, which is a live repository URL rather than an address - `broadcast/`, which holds historical deployment and verification records of what was actually deployed, embedded source included ## How to test ```sh rg 'rndlabs\.xyz' -g '!lib/**' -g '!out/**' -g '!broadcast/**' ``` That returns nothing. `forge build` and `forge test` are green (74 passing). Part of #1.
Solidity's CapWords convention does not carry branding capitalisation into an identifier, and the registry was the last place the fork still spelled it `ComposableCoW`. The surrounding code had already moved on: the poller work uses `ComposableCowPoller`, and `TWAP` holds a `composableCow` while `ERC1271Forwarder` held a `composableCoW`, which is the same inconsistency in miniature. Changes: - `ComposableCoW` -> `ComposableCow`, `composableCoW` -> `composableCow`, `ComposableCoWLib` -> `ComposableCowLib`, and the `*Test` contract names - 14 file renames: `src/ComposableCow.sol`, `script/deploy_ComposableCow.s.sol`, `test/ComposableCow.*.t.sol`, `test/libraries/ComposableCowLib.t.sol` - the `networks.json` top-level key - prose in `README.md`, `docs/architecture.md`, `docs/discovery.md` - `.gas-snapshot` keys Left alone deliberately: - the `CoW Protocol` spelling in prose, and vendored upstream identifiers such as `GPv2Settlement` and `CoWSettlement` - the audit PDFs, which are published artefacts and keep their filenames. Only the link text in the README is updated, so `gnosis-ComposableCoWMayJul2023.pdf` still resolves - `broadcast/`, which records what was actually deployed under the old name `dev/verify-contracts.sh` derives its `networks.json` lookup key from the contract filename, so renaming the key alongside the file keeps verification working rather than needing a mapping shim. **BREAKING**: the contract, its build artefact and the `networks.json` key are all renamed. The manifest describes a version that is not yet deployed and will be superseded by `deployments/networks.json`, so no live consumer is affected. ## How to test `.gas-snapshot` keys were rewritten textually rather than regenerated, on the grounds that a rename cannot change gas. That is verifiable: ```sh forge snapshot && git diff --exit-code .gas-snapshot ``` Clean: the textual rewrite and a regeneration agree exactly. `forge build` and `forge test` are green (74 passing). Part of #1.
Nothing in this tree is deployed, and the artefacts still said otherwise. - **removes the legacy root `networks.json`** (274 lines, 8 contracts x 8 chains). Those are *upstream's* addresses, for contracts this fork no longer matches after the generateOrder rename, typed errors, poll interface and proof-payload changes. Leaving the file invited them to be read as this fork's own deployments. When this fork does deploy, the canonical record is `deployments/networks.json`, landing in #41. - **`dev/verify-contracts.sh`** was the file's only consumer. It now fails with a clear message instead of feeding `null` addresses into `forge verify-contract`. - **drops the README address table** and states plainly that there are no deployments. - **reframes the audit list.** It previously read "The above deployed contracts have been audited by" directly under the address table. Those audits cover upstream before this work, so leaving it unqualified alongside an unaudited warning would have been contradictory. - **adds a warning under the README title** that the fork is unaudited, not deployed, and substantially changed. **BREAKING**: `networks.json` is removed. Any consumer resolving addresses from it was resolving upstream deployments, not this fork's. ## How to test ```sh rg '0x[0-9a-fA-F]{40}' README.md # no addresses left test -f networks.json # gone ./dev/verify-contracts.sh 1 # fails with the no-deployments message ``` Part of #1.
Rename the order-generation entry point to reflect its role as the single source of truth shared by verification and polling: - `getTradeableOrder` -> `generateOrder` (interface, base, all order types, registry call site) - `PollTryAtEpoch` -> `PollTryAtTimestamp` - drop the unused `PollNever` error **BREAKING**: handler ABI renames `generateOrder`; the `IConditionalOrderGenerator` ERC-165 interface id changes. Part of #1.
Reason payloads become `bytes4` selectors of handler-declared custom errors instead of free strings: - `OrderNotValid`, `PollTryNextBlock`, `PollTryAtTimestamp`, `PollTryAtBlock` carry `bytes4 reasonCode` - every reason string constant becomes a declared error (`StrikeNotReached()`, `BeforeTwapStart()`, ...) — part of the handler ABI, so consumers resolve codes to names without a bespoke table - revert data is fixed-size; no stringly errors remain on the error surface **BREAKING**: error signatures change. Part of #1.
The polling half of the dual-path architecture: - `generateOrder` moves into `IConditionalOrder` as the shared source of truth - `GeneratorResult`/`GeneratorResultCode`: `poll()` never reverts for order conditions, returning a structured verdict; fill state is deliberately absent from the generator surface (composed by the registry) - `getNextPollTimestamp` / `describeOrder` hooks with base defaults - revert decoding: only `OrderNotValid` is terminal; `Panic`, `Error(string)` and unknown reverts map to `TRY_NEXT_BLOCK` carrying the caught selector, so transient faults reschedule instead of permanently killing an order (closes #3) - `tryGenerateOrder` probe returns `(success, order, revertData)` as ordinary return data — full `Panic` sub-codes and inner-error arguments retrievable without RPC revert handling **BREAKING**: the `IConditionalOrderGenerator` ERC-165 interface id changes; existing deployed handlers will not satisfy the new id. ## Issues Closes #3. Advances #16 (lands the generator half - `GeneratorResultCode` / `GeneratorResult`; the orthogonal fill overlay that completes the split lands in #24). ## Interface placement `generateOrder` stays on `IConditionalOrderGenerator` rather than moving up to `IConditionalOrder`. An earlier revision of this PR moved it into the base on the grounds that it is "the single source of truth for order derivation, shared by `verify` and `poll`". That reasoning does not survive scrutiny: `verify()` calling `generateOrder()` is how `BaseConditionalOrder` *implements* verify, not something the interface requires. `generateOrder` is `public` on the base, so verify reaches it as an ordinary member call, and nothing casts a handler to `IConditionalOrder` to call it -- the registry gates on `type(IConditionalOrderGenerator).interfaceId` before polling. The move was purely taxonomic and would have cost the distinction the split exists to express: - `IConditionalOrder` -- verifies a discrete order. A handler whose order is fixed rather than derived implements this alone and is not pollable. - `IConditionalOrderGenerator` -- derives discrete orders on-chain, possibly a series of them (TWAP), and is therefore pollable by a monitoring service. Promoting `generateOrder` to the base would have made derivation mandatory for every handler and left the generator interface named after a capability it no longer carried. The divergence risk that motivated the move is real and is addressed directly instead: the `generateOrder` NatSpec now states that a generator's `verify` **MUST** accept exactly the orders it produces, since the two are separate call paths and a mismatch lets a watch-tower propose an order settlement rejects. Non-generators carry no such obligation. `test/ComposableCow.base.t.sol`'s `MirrorConditionalOrder` is the concrete non-generator case and keeps the plain `IConditionalOrder` shape covered. ### Error placement Only `OrderNotValid` stays on `IConditionalOrder`: `verify` raises it directly (`BaseConditionalOrder.sol` on hash mismatch), so it is not generator-specific. The polling errors (`PollTryNextBlock`, `PollTryAtBlock`, `PollTryAtTimestamp`) move down to the generator. They are raised only from `generateOrder` implementations and decoded only by `_decodeErrorToPollResult`; a plain `IConditionalOrder` handler is never polled, so a reschedule hint from its `verify` would have no consumer. This does not change any selector: Solidity derives an error selector from its signature, not from the interface that declares it. The ABI is untouched and only the qualified Solidity reference changes. Part of #1.
Reshape `getTradeableOrderWithSignature` around the generator verdict plus an orthogonal, registry-observed fill overlay: - `PollResult` composes the handler `GeneratorResult` with a `FillStatus` (NONE / PARTIALLY_FILLED / FILLED / INVALIDATED) derived from `GPv2Settlement.filledAmount`; `invalidateOrder` reports as INVALIDATED, not FILLED (closes #11) - a partially filled `partiallyFillable` order keeps returning its signature, so the remainder is no longer stranded (advances #10) - `checkOrder` returns the same composed `PollResult` and applies the ERC-165 handler gate via the shared `_poll` helper (closes #9, closes #6) - `create`/`setRoot` write the cabinet before emitting and carry the resolved value in a new `bytes context` event field (closes #14) - `ConditionalOrderRemoved` emitted on removal - fill-overlay matrix covered by tests (advances #2) Together with the generator half this delivers the verdict/fill split (closes #16). **BREAKING**: `getTradeableOrderWithSignature` returns `(PollResult, bytes)`; `ConditionalOrderCreated` and `MerkleRootSet` gain a `bytes context` parameter (topic0 changes). ## Issues Closes #6. Closes #9. Closes #11. Closes #14. Closes #16 (completed jointly with #23, which landed the generator half). Advances #2 (adds the FILLED / PARTIALLY_FILLED / INVALIDATED overlay matrix, but not the two remaining checks that issue asks for: the orderUid verified against a known-good GPv2 uid - `_mockFilledAmount` mocks `filledAmount(bytes)` with no calldata argument, so any uid matches - and a KIND_BUY vs KIND_SELL total-amount case; every overlay test uses KIND_SELL). Advances #10 (fixes the stranding: a partially filled `partiallyFillable` order keeps its signature. The issue's second half is untouched - `_getFilledAmount` still keys on the per-poll orderUid, so a bounded-total intent whose orderUid moves between polls still cannot be tracked). ### `_setCabinet` helper `setRootWithContext` and `createWithContext` performed the same three steps -- resolve the factory value, write it to the caller's cabinet slot, abi-encode it for the event -- differing only in the key. That is now one internal helper taking the key; the caller is always `msg.sender`. Measured, since the motivation was gas: this is **not** a gas win. On the one fully deterministic test exercising the path, `test_setRootWithContext_e2e`, it costs **+51 gas** (13,465,019 -> 13,465,070); `createWithContext` costs roughly +29. Runtime bytecode drops **214 bytes** (10,055 -> 9,841). At `optimizer_runs = 20000` the optimizer favours runtime speed over size, so a helper called from two sites is emitted once and jumped to rather than inlined: the duplication comes back as size, and the jump plus stack and memory handling is the gas. Kept for the deduplication rather than the gas. Both are setup calls rather than settlement-path calls, so ~50 gas is immaterial, and the contract has ~14.7KB of headroom so the size saving is not load-bearing either. ### Single hash on the create path `createWithContext` hashed `params` twice: once for the cabinet key, and again inside `_create` for the `singleOrders` slot. `hash` takes `memory`, so each call copies calldata to memory, abi-encodes it again (including the dynamic `staticInput`), then keccaks. This predates the `_setCabinet` extraction. `_create` now takes the hash, and `_validateAndHash` produces it: ```solidity function _validateAndHash(IConditionalOrder.ConditionalOrderParams calldata params) internal pure returns (bytes32) { require(address(params.handler) != address(0), InvalidHandler()); return hash(params); } ``` Validating before hashing matters. Threading the hash in naively left `_create`'s null-handler check running *after* the hash, so a rejected call paid for a hash it discarded: `test_create_RevertOnInvalidHandler` regressed **+1210 gas**. Producing the hash only via `_validateAndHash` means the check cannot be skipped and the revert path never hashes. Measured against the previous revision, pinned fuzz seed: | test | delta | | --- | --- | | `test_createWithContextAndRemove_FuzzSetAndEmit` | **-964** | | `test_create_RevertOnInvalidHandler` | -31 | | `test_setRootWithContext_e2e` | -26 | | `test_setRoot_e2e` | -26 | | `test_createAndRemove_e2e` | -12 | Net against #24 as originally written, before either change: **-935 gas** on `createWithContext` and **-190 bytes** of runtime bytecode. ### Typed muxer payload `_buildSignature` built the `SignatureVerifierMuxer` payload with `abi.encodeWithSignature` and a hand-written signature string, which the compiler does not check against the arguments passed alongside it. It now uses `abi.encodeCall`. `safeSignature` is not a declared function anywhere: the muxer compares the first four bytes against a private `SAFE_SIGNATURE_MAGIC_VALUE = 0x5fd7e97d` constant and abi-decodes the remainder. `encodeCall` needs something to point at, so this adds a declaration for exactly that wire format: ```solidity interface ISafeSignaturePayload { function safeSignature(bytes32 domainSeparator, bytes32 typeHash, bytes calldata encodeData, bytes calldata payload) external; } ``` The arguments are now type-checked and the selector is derived by the compiler rather than copied as a string. That moves the failure mode rather than removing it: a drifted declaration would silently change the selector, the muxer would stop routing, and signature verification would fail somewhere unrelated. `test_safeSignaturePayload_SelectorMatchesMuxerMagicValue` pins the selector to `0x5fd7e97d` so that surfaces as a named failure. The constant is `private` upstream and so cannot be imported and compared directly. Encoding is unchanged, verified byte-for-byte against the previous form across 256 fuzzed inputs before the old call was removed. Part of #1.
Override `getNextPollTimestamp` and `describeOrder` where the order type knows better than the validTo default: - TWAP schedules the next part (underflow-safe), and treats before-start and span gaps as WAIT signals rather than terminal failures - StopLoss and GoodAfterTime are single-shot: stop polling after fill Part of #1.
A fill-or-kill order with a zero amount settles without ever incrementing `filledAmount`, so the native replay guard never trips and the order is indefinitely replayable. Reject zero amounts at handler validation (closes #12), enforce the TWAP parameter gate on every entry path (advances #4), and bound the simulation fuzz so the suite runs green within test memory limits. ## Issues Closes #12. Closes #4. ### TWAP degenerate-bundle guard `generateOrder` reaches `TWAPOrder.validate` through `orderFor`, but `getNextPollTimestamp` and `describeOrder` did not validate at all. Both call `_currentPart`, which divides by `twap.t`: - `t == 0` panicked with `0x12` (division by zero) instead of reverting `OrderNotValid(InvalidFrequency)` - `n == 0` returned `POLL_NEVER` silently rather than rejecting Both now call `TWAPOrder.validate` before any part maths. The manifest paths landing in #28 already validate through `try this.validateTwapData`, and `generateOrder` is unaffected, so the settlement path takes no extra validation. `poll()` calls `getNextPollTimestamp` inside the try's *success* block, where a revert would not be caught, but it is only reachable once `generateOrder` has succeeded and therefore already validated the bundle. Direct callers were the exposed path. Three tests cover it. Each was confirmed to fail without the guard: the `t == 0` cases with the raw `0x12` panic, and the `n == 0` case with "next call did not revert as expected". ### Unrelated test fix `test_generateOrder_e2e_fuzz_WithContext` carried a `vm.assume` commented "guard against overflows" that overflowed itself: ```solidity vm.assume(currentTime < _blockTimestamp + (FREQUENCY * NUM_PARTS)); ``` `_blockTimestamp` is a `uint32`, so the sum wraps for timestamps near the type maximum, and the test panicked `0x11` on such an input. Widened to `uint256`. This predates the stack (it is in `develop`); it surfaced here because the new tests shift the fuzz input sequence. `test_simulate_fuzz` is no longer excluded from CI-equivalent runs: it began passing at #24, which changed the registry poll path. Part of #1.
Sidecar interface (own ERC-165 id, feature-detected, never on the settlement path) for enumerating the discrete orders a conditional order will produce: - `Cardinality` names the `totalOrders` semantics directly: EXACT / CAPPED / UNBOUNDED (closes #13) - explicit pagination contract: an empty page with `hasMore == true` is unreachable, so a naive offset walker always terminates - `getManifestPage` carries a `bytes4 reasonCode` so a not-yet-active order is distinguishable from a permanently invalid one on an empty page (advances #5) - `BaseConditionalOrder` defaults to a single-shot manifest (EXACT, 1) ## Issues Closes #13. Advances #5 (the catch now surfaces the handler's `bytes4 reasonCode`, but `_decodeErrorToGeneratorResult`'s `code` and `waitUntil` are both discarded, so a consumer still cannot classify "not yet active" against "permanently invalid" without per-handler knowledge of the error names, and the preferred `validFrom` on the page is not carried). Part of #1.
- TWAP enumerates all n parts with per-part validity windows, validating parameters before any part math so degenerate bundles yield an empty manifest instead of underflowing - PerpetualStableSwap exposes only the current discrete order and terminates pagination on every branch (closes #7); empty pages carry the decoded reason (e.g. `NotFunded`) ## Issues Closes #7. Advances #4 (gates `getManifestInfo` and `getManifestPage` on `validateTwapData` before any part math, satisfying that issue's manifest requirement; the poll-hint entry points remain unvalidated). Part of #1.
Architecture documentation for the dual-path design: settlement/polling split, generator verdict and registry fill overlay, typed-error decoding policy and `tryGenerateOrder` diagnostics, order manifest pagination contract, the multi-order pattern via `offchainInput` keying (closes #17), ERC-1271 integration, the handler purity invariant (advances #8), and the full breaking-changes migration guide from upstream. ## Issues Closes #17. Advances #8 (design principle 6 in `docs/architecture.md` states the invariant, but the NatSpec note on `IConditionalOrder.generateOrder` is unchanged and there is no test or lint guard - the issue explicitly prefers those over documentation alone). Part of #1.
Solidity >=0.8.27 supports `require(condition, CustomError())` in the legacy pipeline; convert the remaining `if (!(condition)) revert` sites so preconditions read as preconditions. Bytecode-equivalent. Part of #1.


Summary
bytes4 reasonCodeselectors end to end, no stringly errorsIOrderManifestinterface for order enumeration across all order typesConditionalOrderRemovedevent on order deauthorizationThis PR stays open as the delivery tracker and review record. The work lands as a stack of slices into
develop; review threads here remain the canonical design discussion. This branch is frozen and will not be merged.Slices
Stacked in dependency order; each merges into
developvia squash after review.Discovery train (spec-first; implements #15):
Design gates
Breaking Changes
See
docs/architecture.md(#29) for the full migration guide from upstream.Test Plan