docs: describe the manifest status in the architecture doc - #55
Merged
Conversation
mfw78
force-pushed
the
docs/caller-identity
branch
from
August 1, 2026 02:58
2e5467f to
626dd7f
Compare
The pagination contract still had `getManifestPage` returning `(entries, hasMore, reasonCode)` and described the third value as a bare selector. It has been a `ManifestStatus` since #52, and `ManifestStatus` appeared nowhere in the docs. The `IOrderManifest` sketch declared the third return as `string memory status`, which predates even the typed-error work in #22. Also records that an ordinary page reports `POST` with zeroes, including a page empty only because `offset` is past the end, since that is what separates "nothing here" from "nothing yet".
mfw78
force-pushed
the
docs/caller-identity
branch
from
August 1, 2026 04:09
626dd7f to
54024f2
Compare
mfw78
changed the title
mfw78
added a commit
that referenced
this pull request
Aug 1, 2026
Closes #8. Stacked on #55 (an unrelated manifest docs fix); merge that first. `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` 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, which is where this started, would have left a second hole. `sender` also differed by call path: | Path | `sender` argument | |------|-------------------| | Settlement (`verify`) | the settlement caller | | Polling (`poll`) | the poll caller | | Manifest (`getManifestPage`) | `address(0)` | So a handler following the obvious mitigation, using `sender` in place of `msg.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`, `poll` and `tryGenerateOrder` no longer take `sender`. 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. `verify` keeps its `sender`, and the split follows the interface hierarchy rather than being a compromise: - `verify` is on `IConditionalOrder`, 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. - `generateOrder` and `poll` are on `IConditionalOrderGenerator`. A generator makes a polling promise, and now cannot break it. The accepted cost: settlement-time gating is not previewable by `poll` or `getManifestPage`. That is inherent rather than a regression, since a preview cannot know who will eventually settle. `sender` cannot be removed at the boundary regardless. `isValidSafeSignature` implements `ISafeSignatureVerifier` from 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 in `src/` or `test/`, 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-snapshot` is 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 `develop` rather 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 build` and `forge fmt --check` clean. The two compiler warnings are pre-existing, confirmed by building the stashed baseline. The only remaining `sender` references in `src/` are the three intended ones: `isValidSafeSignature`'s parameter, its forwarding to `handler.verify`, and `IConditionalOrder.verify`'s declaration. Adds the **Caller Identity** section to `docs/architecture.md` recording 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 named `sender` as 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.
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.
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:
getManifestPagewas described as returning(entries, hasMore, reasonCode), with the third value a bare selector. It has been aManifestStatussince feat!: carry the generation verdict on a manifest page #52, andManifestStatusappeared nowhere in the docs.IOrderManifestsketch. The third return was declaredstring memory status, which predates even the typed-error work in feat!: replace string error reasons with typed error selectors #22.Also records that an ordinary page reports
POSTwith zerowaitUntilandreasonCode, including a page that is empty only becauseoffsetis past the end. Both return no entries, and onlycodeseparates "nothing here" from "nothing yet", which is the distinction the struct exists to carry.forge fmt --checkclean, 166 tests pass. No source change.