feat(matching): FOK matching gate + execution wiring (3/3)#169
feat(matching): FOK matching gate + execution wiring (3/3)#169gregorydemay wants to merge 10 commits into
Conversation
…vation on kill Add an `execute(order, require_full)` entry point on the order book: it plans the fills read-only and, when `require_full` is set and the plan does not fully fill, returns a `Killed` outcome before any mutation, leaving the book untouched. `process_pending_orders` derives `require_full` from each order's `time_in_force` and records killed FOKs in `MatchingOutput::expired_orders`. `record_matching_event` maps those to `status = Expired` and releases the full placement reservation, reusing the cancel path's unreserve/refund. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…utcome Unit tests assert a FOK fully fills (Filled / filled == quantity), kills with zero execution and no book trace when liquidity is absent or insufficient (Expired / filled == 0, reservation released), and that a FOK fill bills the FOK side the taker rate and its resting counterparty the maker rate. End-to-end PocketIC tests place FOKs via add_limit_order and observe the terminal Filled / Expired outcome via get_my_orders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A single fill-or-kill order that sweeps the entire fragmented ask side of the Binance depth snapshot in one message, confirming the atomic FOK stays well within the per-message instruction limit. Regenerate canbench_results.yml. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Record the asynchronous FOK model (evaluated upon execution, never resting), the Expired terminal state and how it differs from the user-initiated Canceled, and that time_in_force constrains how long an order may rest in the book. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
| balance_operations.push(event::BalanceOperation::Unreserve { | ||
| order: *seq, | ||
| token: refund_token, | ||
| amount: refund_amount, |
There was a problem hiding this comment.
🧐 🟠 Medium — the FOK-kill refund computation here is a near-exact copy of the cancel-path block in record_cancel_limit_order (Side::Buy => (PairToken::Quote, price.checked_mul_quantity_scaled(&qty, base_scale).expect(...)); Side::Sell => (PairToken::Base, qty)), and a third near-twin lives in the placement-reservation path. The spec frames this as "reuses the same unreserve/refund computation the cancel path already performs" — but it is realized by copy-paste, not a shared helper, so the three sites can now silently drift (e.g. a future scaling fix lands in one and not the others). Suggest extracting a small fn refund_for(side: Side, price: Price, qty: &Quantity, base_scale) -> (PairToken, Quantity) and calling it from all three sites; the expect("BUG: ... overflow") message is then defined once. Behaviour is correct today (mutation-tested: dropping this loop fails both reservation-release tests), so this is a maintainability gate, not a correctness bug.
There was a problem hiding this comment.
🤖 Done in b399d6d. Extracted refund_for(side, price, remaining_quantity, base_scale) -> (PairToken, Quantity) (a free fn alongside compute_balance_operations in state/mod.rs) and called it at both PairToken refund sites: the cancel path in record_cancel_limit_order and the FOK-kill path in record_matching_event. Behavior is identical — all reservation/refund tests pass unchanged. I left the placement-reservation path (record_limit_order) and validate_limit_order as-is: those resolve to concrete TokenIds (pair.quote/pair.base), not PairToken, so they don't share the helper's return type.
| /// `price × remaining_quantity` (or just `remaining_quantity` for Sell) is | ||
| /// the amount to unreserve. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| #[derive(Debug, Clone, PartialEq, Eq, Encode, Decode)] |
There was a problem hiding this comment.
🧐 🔵 Nit — this PR adds Encode, Decode to RemovedOrder only because it is now embedded in MatchingOutput.expired_orders (#[n(3)]), and MatchingOutput derives Encode/Decode. But MatchingOutput is never actually serialized — it is produced by process_pending_orders and consumed transiently by record_matching_event; only MatchingEvent { orders: Vec<OrderSeq> } is persisted. I confirmed by removing both derives (and the #[n(..)] attrs): nothing at a call site needs them. So the new RemovedOrder Encode/Decode is dead capability, inherited from an already-unused derive on MatchingOutput. Optional: drop Encode/Decode + the #[n] field tags from both MatchingOutput and RemovedOrder. Pre-existing on MatchingOutput, so fine to leave — flagging since the PR propagates it.
There was a problem hiding this comment.
🤖 Done in b92a81d. Confirmed neither MatchingOutput nor RemovedOrder is ever serialized — no minicbor::encode/decode, to_vec, from_slice, or stable-memory persistence references either type; both are only constructed and consumed in-memory. Dropped the Encode, Decode derives from both types and removed the now-dead #[n(..)] field attrs (including expired_orders'). The minicbor::{Decode, Encode} import stays — other types in the module still use it.
| FOK can't fully fill | ||
| |----------------------+ | ||
| v v | ||
| +------------+ +------------+ |
There was a problem hiding this comment.
🧐 🔵 Nit — the diagram branches processes queue into Open vs Expired, but a FOK that fully fills goes Pending → Filled directly without passing through Open (and without resting). The prose just below (point 5 + the Time-in-Force section: "transitions only Pending → Filled or Pending → Expired; it never reaches Open") is accurate, so this is purely a diagram-simplification nit — a fully-filling FOK has no arrow to Filled that bypasses Open in the picture. Non-blocking.
There was a problem hiding this comment.
🤖 Done in d747d24. The diagram now branches the matching outcome three ways — fully fills (direct → Filled), rests/partial (→ Open), and FOK can't fully fill (→ Expired) — so a fully-filling FOK or GTC reaches Filled without passing through Open.
|
🧐 Review — FOK matching gate + execution wiring (3/3) — VERDICT: CHANGES_REQUESTED (Posted as a comment: GitHub rejects a formal request-changes review on a self/bot-authored PR.) 1 🟠 Medium, 2 🔵 Nits (all inline). CI fully green. The core safety property is sound; the gate is a maintainability one, not correctness. Correctness — verified by mutation testing, not inspection:
Killed-book snapshot: zeroing Maintainability sweep:
Tests earn their place: each FOK test covers a distinct R; Addressing the 🟠 (extract the shared refund helper) clears the verdict. |
Extract a single helper shared by the cancel path and the FOK-kill path so the two reservation-refund computations cannot drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dOrder Neither type is ever serialized; the derives (and their CBOR field indices) were dead weight inherited from MatchingOutput's derive list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A fully-filling order (FOK or GTC) transitions Pending to Filled without passing through Open; the diagram now branches on the matching outcome. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gregorydemay
left a comment
gregorydemay
left a comment
There was a problem hiding this comment.
🧐 Re-review — FOK matching gate + execution wiring (3/3) — VERDICT: READY
All three prior findings resolved; CI fully green (unit-tests, integration-tests, benchmark, lint, reproducible-build, candid check all pass). No new comments since the last pass. 388 canister unit tests pass on the head commit (d747d24).
Prior findings — verified resolved:
- 🟠 Refund duplication (
b399d6d) —refund_for(side, price, remaining_quantity, base_scale) -> (PairToken, Quantity)extracted; bothPairTokenrefund sites (cancel path inrecord_cancel_limit_order, FOK-kill path inrecord_matching_event) now call it, and theexpect("BUG: …overflow")message is defined once. The diff is a verbatim extraction of the two identical match arms — behavior-preserving by construction; all reservation/refund tests pass unchanged. Leaving the placement /validate_limit_orderpath as-is is the right call: it resolves to concreteTokenIds (pair.quote/pair.base), notPairToken, so it cannot share this helper's return type, and the shared scaling math it does duplicate already lives inchecked_mul_quantity_scaled— no meaningful drift risk. - 🔵 Dead derives (
b92a81d) — confirmed neitherMatchingOutputnorRemovedOrderis serialized anywhere;Encode/Decodederives and#[n(..)]field tags dropped from both. Theminicbor::{Decode, Encode}import stays (still used byOrderBookSnapshotand others); lint green. - 🔵 Design diagram (
d747d24) — the lifecycle diagram now branches the matching outcome three ways, with a directPending → Filledarrow that bypassesOpenfor a fully-filling FOK/GTC.
Nothing regressed — the FOK feature/test/benchmark/docs commits are unchanged from the first pass; the only delta is the three targeted fix commits above (no scope creep). The core guarantees still hold: structural mutation-free kill (gate short-circuits before apply_plan), single reservation release on kill, both-legs fee assignment (FOK→taker, resting→maker) with no FOK-specific fee code, and the R10 worst-case sweep (~7.24B instructions) well within the per-message limit.
Maintainability sweep: duplication — the flagged 🟠 is now cleared (shared refund_for); no other substantial duplication. Unused derives — cleared (dead Encode/Decode removed). Primitive-obsession params — none (require_full: bool is the spec's deliberate gate, not a quantity). Divergent invariant handling — none (placement/cancel/FOK-kill refunds handle Buy→quote(scaled)/Sell→base identically; consistent expect("BUG: …")). Silent fallbacks — none (kill/overflow paths expect-trap; no unwrap_or_default/.ok() discard/NaN/let _ =).
No blockers, no mediums, no nits remaining. Ready for human approval.
|
🤖 Spec-driven loop complete — ready for your review. PR 3 (3/3) of the DEFI-2853 stack: the FOK matching gate + execution wiring. FOK orders now route through matching — a FOK either fully fills (
The 3 review threads are addressed + replied, left unresolved for you to close. Draft — marking ready, approval, and merge are yours. |
# Conflicts: # canister/canbench_results.yml
|
🤖 Synced onto the updated #164: merged |
Pin the user-initiated cancel path for a fill-or-kill order, distinct from the engine's expire/kill path: - cancelling a Pending FOK ends Canceled (not Expired) and refunds the placement reservation, and - cancelling a FOK the engine already killed is rejected with OrderAlreadyTerminal. The Pending window is only deterministically observable at the unit level, where matching is driven explicitly; at the integration level the eager post-placement matching timer races the cancel, so no integration test is added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gregorydemay
left a comment
gregorydemay
left a comment
There was a problem hiding this comment.
🧐 Re-review (scoped to the two changes since my prior READY): base-sync merge a66bf98 + new test-only commit f6bde59. CI fully green.
Base-sync (a66bf98 + 55d6851): the merge touches only canister/src/order/tests.rs (drops the redundant should_preserve_time_in_force_of_resting_order resting-TIF test). No book.rs / state / execute files in the merge — the plan/execute matching gate, the require_full/Killed path, and record_matching_event are byte-untouched. Logic undisturbed, confirmed.
New tests (f6bde59, state/tests.rs::cancel_limit_order): correct, non-tautological, and they earn their place.
- Coverage established by mutation, not inspection:
validate_cancel_limit_ordermade to acceptExpired(Pending|Open|Expired => Ok) → onlyshould_reject_canceling_expired_fokfails across the entire suite (392 pass, 1 fail). No existing terminal-cancel test (Filled / second-cancel) covers theExpiredarm, so this closes a real gap.- Refund amount zeroed in
record_cancel_limit_order→should_cancel_pending_fok_into_canceled_and_refundfails. The reservation refund is genuinely asserted (via the sharedassert_cancel_refunds), not pulled from the actual. - Cancel made to write
Expiredinstead ofCanceled→ the Pending-FOK test fails, so it does pin the Canceled-vs-Expired distinction (theCanceledassertion is also shared with the GTC pending-cancel tests; the FOK test's unique contribution is exercising a FOK-typed order through the cancel/refund path, which nothing else does).
- No redundancy with the existing
fill_or_killintegration tests (those cover Filled / Expired-no-liquidity / Expired-insufficient) or the GTC cancel tests. place_fok_orderis a clean parameterization overplace_order_with_tif— no copy-paste.
Integration-test skip — justified. add_limit_order fires drive_matching via set_timer(Duration::ZERO, …) (lib.rs:129), so advancing PocketIC time to land a cancel would also fire matching; the Pending window is not deterministically observable end-to-end, and a test relying on the race would be flaky. The spec's R8 integration AC ("a user cancel still yields Canceled") is already met by the existing GTC cancel integration test; the FOK Pending-cancel is correctly pinned at the unit level where matching is driven explicitly. No real end-to-end coverage missing.
Maintainability sweep: duplication: none found (shared helpers, ~6-line tests). Unused derives: none (no new types). Primitive-obsession params: none (place_fok_order mirrors the typed place_order signature). Divergent invariant handling: none — validate_cancel handles Filled|Canceled|Expired uniformly as OrderAlreadyTerminal at the single site. Silent fallbacks: none.
No blockers, no mediums, no nits. CI green.
VERDICT: READY
|
🤖 Stack upkeep + new coverage — still READY.
Reviewer re-confirmed by mutation testing — the expired-cancel test closes a real gap (no prior test covered the |
1 participant
Routes fill-or-kill (FOK) limit orders through the matching engine, completing the FOK feature. A FOK either fully fills against resting liquidity at its price or better, or is killed with zero execution — it never rests and never settles a partial fill. The kill is structural: matching plans the fills read-only first, and when the full quantity is not satisfiable the book is left provably untouched. A killed FOK reaches the
Expiredterminal state with its placement reservation fully released; the caller observes the outcome asynchronously viaget_my_orders. GTC behavior is unchanged.Spec:
docs/src/development/specs/DEFI-2853-add-fill-or-kill-orders-fok.md(#156).Requirement coverage:
Filled; never rests.Expiredwith zero fill, no book trace, and the reservation released.add_limit_orderreturns an order ID immediately; the FOK transitions onlyPending → FilledorPending → Expired, neverOpen.CanceledvsExpireddistinction, and thattime_in_forceconstrains how long an order may rest.📚 PR stack