refactor!: remove caller identity from the generation surface - #56
Merged
Conversation
mfw78
force-pushed
the
docs/caller-identity
branch
from
August 1, 2026 04:09
626dd7f to
54024f2
Compare
mfw78
force-pushed
the
refactor/drop-generation-sender
branch
from
August 1, 2026 04:09
991637a to
c553995
Compare
mfw78
added a commit
that referenced
this pull request
Aug 1, 2026
Documentation only, and independent of the caller-identity work now in #56. Split out so each PR is written once rather than one revising the other. The manifest documentation still described the interface as it stood before #52: - **Pagination contract.** `getManifestPage` was described as returning `(entries, hasMore, reasonCode)`, with the third value a bare selector. It has been a `ManifestStatus` since #52, and `ManifestStatus` appeared nowhere in the docs. - **`IOrderManifest` sketch.** The third return was declared `string memory status`, which predates even the typed-error work in #22. Also records that an ordinary page reports `POST` with zero `waitUntil` and `reasonCode`, including a page that is empty only because `offset` is past the end. Both return no entries, and only `code` separates "nothing here" from "nothing yet", which is the distinction the struct exists to carry. `forge fmt --check` clean, 166 tests pass. No source change.
`generateOrder` is reached three ways and `msg.sender` differs between them: the registry under `verify`, which calls it internally, and the handler itself under `poll` and `getManifestPage`, which reach it via `this.generateOrder` for try/catch. A handler branching on `msg.sender` therefore derives one order under settlement and another under polling, so a monitoring service can propose an order that fails to settle, or settlement can execute an order no consumer previewed. Documenting that alone would have left a second hole. `sender` also differed by path: the settlement caller, the poll caller, and `address(0)` on the manifest. A handler following the obvious mitigation, using `sender` in place of `msg.sender`, would still have produced a manifest disagreeing with settlement. The mitigation was itself path-dependent. So `generateOrder`, `poll` and `tryGenerateOrder` no longer take `sender`. An order that varies with the settling party is not expressible, and the rule holds structurally rather than by review. `verify` keeps `sender`. It has a single call path, so the value is unambiguous, and gating on the settling party by reverting is a legitimate use. The split follows the interface hierarchy: a handler implementing `IConditionalOrder` without being a generator makes no polling promise and may gate freely, while a generator makes that promise and now cannot break it. The accepted cost is that settlement-time gating is not previewable by `poll` or `getManifestPage`, which is inherent rather than a regression, since a preview cannot know who will eventually settle. `sender` cannot be dropped at the boundary regardless: `isValidSafeSignature` implements `ISafeSignatureVerifier` from the Safe library, so the value arrives whether or not it is forwarded. Every implementation already declared the parameter unnamed, in all five handlers and every test fixture, which is what made this a signature change rather than a behavioural one. Nothing read it in `src/` or `test/`. Gas is incidental but favourable: settlement and polling entry points drop roughly 100 to 225 gas from the shorter calldata and dispatch. Seven TWAP manifest cases rise between 22 and 198 gas, and nothing regresses beyond that. BREAKING CHANGE: `generateOrder`, `poll` and `tryGenerateOrder` lose their `sender` parameter. No deployments exist, so no consumer is affected. Closes #8.
Kept separate from the interface change so that diff stays reviewable, because most of this one is catch-up rather than consequence. The committed file was already stale before this branch: it disagreed with a fresh run on 24 of its 28 entries, and covered 28 of the 122 tests the suite now reports. It appears not to have been regenerated as the suite grew. Generated with the seed CI pins (`--fuzz-seed 672679878`), so fuzz entries are reproducible rather than varying per run.
mfw78
force-pushed
the
refactor/drop-generation-sender
branch
from
August 1, 2026 04:30
c553995 to
49b5dbc
Compare
mfw78
added a commit
that referenced
this pull request
Aug 2, 2026
Descriptors for all five order types, the generator that derives them, and a Solidity test that checks them against the contracts they describe. ## Where they live, and why `descriptors/`, at the repository root, mirroring `deployments/`. That directory already establishes the pattern: machine-readable artifacts consumed by off-chain services, with a README carrying the schema and the rules. Keeping them out of `src/types/` is not just tidiness. `forge build`, `forge fmt` and the AST walk this generator itself performs all treat `src/` as Solidity, so a JSON tree there is noise to each of them. ``` descriptors/ README.md overlays/<Handler>.json # hand-written: name, description, display, links, labels <Handler>.json # generated, canonical bytes ``` The overlay is the only hand-written part. Everything a consumer decodes with is derived from compiler output, so it cannot drift from the contract without the build noticing. ## The spec was wrong about where `errors` comes from `docs/discovery.md` §1.4 said `errors` is derived "from the handler ABI (every reason error is a declared error)". It is not there. Every reason error in this codebase is declared at **file scope**, where solc omits it from the contract ABI. The TWAP ABI carries eight errors and not one of them is a reason code; they are the framework wrappers (`OrderNotValid`, `PollTry*`) plus the descriptor errors. So `errors` is derived from the AST instead, identified as the names referenced as `X.selector` across the handler's **transitive import closure**. The closure matters: TWAP's own file references only three selectors, and the ten validation errors (`InvalidSpan`, `InvalidNumParts`, …) are raised inside the `TWAPOrder` library it calls. §1.4 is corrected in this PR. `staticInput.components` also comes from the AST, which §1.4 already specified correctly, because the struct never crosses an external ABI boundary. ## A wrong derivation that surfaced a real bug The obvious way to derive `offchainInput.required` is "does the handler declare `PollNeedsOffchainInput`". That is unsound, and it produced `required: false` for all five handlers. **`GoodAfterTime` decodes its buy amount from `offchainInput`** (`abi.decode(offchainInput, (uint256))`) and never declares that error. It is the one handler where `required` is genuinely `true`. So the field is instead derived from whether `generateOrder` actually reads its `offchainInput` parameter, which is exactly right for all five: the other four leave the parameter unnamed. That also means `GoodAfterTime` currently mis-signals at runtime. Polled with empty `offchainInput` it does not return `NEEDS_INPUT`; the decode reverts and is mapped to `TRY_NEXT_BLOCK`, telling a monitoring service to retry next block forever for a condition that no amount of waiting resolves. Filed separately as #58; not fixed here, since this PR does not change handler behaviour. ## Testing without a JS runtime Two layers, neither needing node_modules. ```sh dev/gen-descriptors.py --check # committed bytes match what the pipeline emits forge test --match-contract DescriptorDoc ``` The generator is standard library only, plus `cast sig` so selector hashing is foundry's own rather than a reimplementation. `--check` runs in CI and catches hand-edits and staleness; verified to exit 1 on a one-character edit. The Solidity test asserts on-chain facts, which is why it is Solidity: - every `errors` key equals `bytes4(keccak256("<name>()"))`, so a key and its name cannot disagree. This is the failure that matters most, since consumers key on the selector and display the name; - selectors are distinct, because a duplicate silently drops an entry; - component count matches the ABI-encoded struct width; - a provoked verdict's `reasonCode` is present in the descriptor: the divergence check §1.3 asks consumers to run, applied to ourselves. **Each assertion was mutation-checked** against a deliberately corrupted document: ``` flip one selector key SelectorMatchesDeclaredName: FAIL drop a staticInput component ComponentCountMatchesEncodedWidth: FAIL remove the observed error ObservedReasonCodeIsDocumented: FAIL restored 6 passed ``` `parseJsonKeys` is declared as a local interface: the `forge` binary supports it but the vendored `forge-std` predates it, and one cheatcode does not justify bumping the submodule. `fs_permissions` is scoped read-only to `./descriptors`. ## Second commit: the document carries no handler identity §1.3 required `handler.{chainId,address}` to match the contract the descriptor was resolved from. On the deterministic deployment path that is unsatisfiable. `OrderDescriptor` takes the digest as a constructor argument and stores it immutable, so it is part of the initcode. `deploy_ProdStack` uses `new X{salt: ...}`, and CREATE2 derives the address from `keccak256(0xff ++ deployer ++ salt ++ keccak256(initCode))`. So **address depends on digest depends on document bytes depends on address**. No fixed point exists, and a salt search does not help, since the salt does not cancel the digest's contribution. (`deploy_OrderTypes` uses plain CREATE, where the address is a function of deployer and nonce, so the cycle is specific to the path intended for production.) Only the address closes the loop; chain id is chosen up front. But chain id earns nothing either: every field in the document is chain- and deployment-independent, so requiring it would force one publication per chain for byte-identical content and defeat content addressing. Both are removed. Binding comes from resolution, which is where it already came from. A consumer reads `descriptorCommitment()` from a specific contract and verifies fetched bytes against that digest, so a document is that contract's descriptor exactly when it hashes to what the contract returns. An identity field restates what the commitment proves. **This makes the generated documents complete rather than partial**: one digest per handler version, publishable before a chain or address is chosen, valid for every deployment. The first commit's README described the missing field as a gap awaiting deployment; that was wrong, and this corrects it. The merkle payload document in §3.3 keeps its `chainId` and per-order `handler` addresses: that document enumerates orders for an owner on a chain, so identity is load-bearing there and creates no cycle. No descriptor bytes change, since the generator never emitted the field. There is still no published descriptor-v1 JSON Schema to validate against; §1.3 expects one separately. ## Third commit: the descriptor-v1 JSON Schema §1.3 referred to a schema as "published separately", so a consumer had no way to reject a malformed document short of reimplementing the rules from prose. It now lives at `descriptors/schema/descriptor-v1.json`, and CI validates every document against it plus the schema against the draft 2020-12 metaschema. It also makes the second commit's decision **enforceable rather than documented**: a `not` clause rejects any document carrying `handler`, so the CREATE2 cycle cannot be reintroduced by accident. Checked against deliberately malformed documents rather than assumed to work: ``` unmodified TWAP PASS + handler identity FAIL uppercase selector key FAIL non-selector key FAIL missing display FAIL non-canonical component type FAIL wrong version FAIL extensions carrying free data PASS (deliberately open) ``` §1.3 also called for a content-addressed `$id`. That has the same self-reference problem as `handler.address`: a digest over the schema cannot be embedded in the schema it describes, since the value would depend on bytes that depend on the value. The `$id` is a stable identifier; publishing at a content-addressed location is fine so long as the address is carried by whatever references the schema, never inside it. The schema sits under `schema/` rather than beside the documents so `descriptors/*.json` keeps meaning "documents". Validating that glob would otherwise include the schema, which does not describe itself. **Validator:** any draft 2020-12 implementation. CI uses `check-jsonschema`, a single `pipx install`, which also checks the schema against the metaschema. Locally, `nix-shell -p check-jsonschema`. No JS toolchain, and nothing added to the generator, which stays standard library only. ## Fourth commit: tersen the prose The markdown across this branch argued its case rather than stating it. Rationale belongs here, in the pull request, not in a document someone consults for facts. Cut from `descriptors/README.md`, `docs/discovery.md` §1.3 and the Caller Identity section of `docs/architecture.md`: rhetorical framing ("two consequences follow, both intended", "the upshot is", "which is the accepted cost"), restatements of points already made, and asides about what a reader would be wrong to conclude. Facts, tables, commands and rules are unchanged. Net 30 lines removed. It also fixed two errors in that prose, both introduced by the third commit: - the README referred to "the CREATE2 cycle described below" from a section above it; - the README still claimed no descriptor-v1 schema existed and that §1.3 expected one separately, which the third commit had already made false. ## Fifth commit: reason errors must be nullary All 22 reason errors are nullary today, and the protocol cannot carry anything else. A handler raises one by passing `X.selector` to a framework wrapper whose payload is `bytes4`: ```solidity require(data.sellAmount > 0, IConditionalOrder.OrderNotValid(ZeroAmount.selector)); ``` The reason error is a **tag**; it is never constructed. Arguments would be discarded at the raise site and could not be recovered from `poll`, `getManifestPage`, or even `tryGenerateOrder`, since that returns the *wrapper's* revert data and the reason error was never thrown. Nothing prevented declaring one, though. The generator would have computed the selector over the full signature and emitted it, and the Solidity test would then have failed comparing it against `keccak256("<name>()")`, reporting a selector mismatch rather than the real problem. The generator now rejects it explicitly. Verified against an injected `StrikeNotReached(uint256)`, which exits 1 with: ``` StopLoss: reason error StrikeNotReached declares parameters (uint256); reason errors must be nullary, since only the selector is propagated ``` Structured data travels in the wrapper instead, as `waitUntil` does for the timed verdicts. Recorded in `docs/discovery.md` §1.3. ## Verification 172 tests pass (166 plus 6 new), `forge build` and `forge fmt --check` clean, `--check` reports all five current, and all five validate against the schema. Rebased onto `develop` after #56 merged. The descriptor documents are byte-identical across that change (`--check` passes against a rebuilt AST), which is the expected result: removing `sender` from the generation surface touches neither the `staticInput` struct nor any reason error.
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.
Closes #8. Stacked on #55 (an unrelated manifest docs fix); merge that first.
generateOrderis reached three ways andmsg.senderdiffers between them: the registry underverify, which calls it internally, and the handler itself underpollandgetManifestPage, which reach it viathis.generateOrderfor try/catch. A handler branching onmsg.senderderives one order under settlement and another under polling, so a monitoring service can propose an order that fails to settle, or settlement can execute an order no consumer previewed.Documenting that alone, which is where this started, would have left a second hole.
senderalso differed by call path:senderargumentverify)poll)getManifestPage)address(0)So a handler following the obvious mitigation, using
senderin place ofmsg.sender, would still produce a manifest disagreeing with settlement. The mitigation was itself path-dependent, which is why documentation alone could not close #8 honestly.The change
generateOrder,pollandtryGenerateOrderno longer takesender. An order that varies with the settling party is no longer expressible, so the rule holds structurally rather than by review. This is the enforcement #8 asked for, reached by deleting a parameter rather than by adding a test or a lint.verifykeeps itssender, and the split follows the interface hierarchy rather than being a compromise:verifyis onIConditionalOrder, has a single call path, and gating on the settling party by reverting is a legitimate use. A validator-only handler makes no polling promise and may gate freely.generateOrderandpollare onIConditionalOrderGenerator. A generator makes a polling promise, and now cannot break it.The accepted cost: settlement-time gating is not previewable by
pollorgetManifestPage. That is inherent rather than a regression, since a preview cannot know who will eventually settle.sendercannot be removed at the boundary regardless.isValidSafeSignatureimplementsISafeSignatureVerifierfrom the Safe library, so the value arrives whether or not we forward it.Why this was mechanical
Every implementation already declared the parameter unnamed (
function generateOrder(address owner, address, bytes32 ctx, ...)) across all five handlers and all the test fixtures. Nothing read it anywhere insrc/ortest/, which is what made the removal a signature change rather than a behavioural one. 57 call sites updated, all argument-position edits.Gas
Incidental but favourable. Settlement and polling entry points drop roughly 100 to 225 gas from shorter calldata and dispatch:
test_settle_e2e−224 (TWAP) and −180 (GAT),test_createAndRemove_e2e−224,getTradeableOrderWithSignature−225.Seven TWAP manifest cases rise between 22 and 198 gas. Nothing regresses beyond that.
The much larger four and five figure drops elsewhere in the snapshot are contract deployment costs in test setup, not runtime savings, so the suite-wide net is not a meaningful headline number.
Second commit
.gas-snapshotis regenerated separately so this diff stays reviewable. Most of it is catch-up, not consequence: the committed file already disagreed with a fresh baseline run on 24 of its 28 entries, and covered 28 of the 122 tests the suite now reports. It had not kept pace as the suite grew. Regenerated with the seed CI pins so fuzz entries are reproducible.I measured this change's gas by regenerating the baseline from
developrather than diffing against the committed file, since the committed file's staleness would otherwise have been attributed to this change. My first pass did exactly that and produced a nonsense +1.8M "regression".Verification
166 tests pass,
forge buildandforge fmt --checkclean. The two compiler warnings are pre-existing, confirmed by building the stashed baseline. The only remainingsenderreferences insrc/are the three intended ones:isValidSafeSignature's parameter, its forwarding tohandler.verify, andIConditionalOrder.verify's declaration.Adds the Caller Identity section to
docs/architecture.mdrecording the three call paths and both rules, sharpens design principle 6 to state purity over the generated order rather than the argument list, and drops a stale TWAP NatSpec line that namedsenderas unused.An earlier revision of this pair documented the invariant first and refactored second, which meant 23 of the docs PR's added lines were deleted again here. Restructured so each PR is written once: #55 now carries only the unrelated manifest documentation fix, and everything about caller identity is in this one.