refactor(settlement): extract a settlement module and harden fill-persistence tests#195
Draft
gregorydemay wants to merge 140 commits into
Draft
refactor(settlement): extract a settlement module and harden fill-persistence tests#195gregorydemay wants to merge 140 commits into
gregorydemay wants to merge 140 commits into
Conversation
Add `filled_quote` and `filled_fee` to the order record so `get_my_orders` exposes per-order realized notional and fee summaries (DEFI-2901, PR 1 of 3). `filled_quote` is the cumulative realized quote notional `Σ (maker_price × quantity / base_scale)`; a buy taker's released reservation surplus is excluded, so VWAP is derivable as `filled_quote × base_scale / filled_quantity`. `filled_fee` is the cumulative realized fee in the order's receive token (base for a buy, quote for a sell), the amount actually withheld. `OrderUpdate` gains matching `quote_delta` / `fee_delta`, folded into the same single read-modify-write as `filled_quantity` and `status`, each guarded by an always-on overflow trap. Settlement computes the per-fill notional, fees, and maker/taker roles once and feeds both the balance operations and the per-order deltas from that single source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…USDT Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`record_matching_event` materialized every fill's settlement into a `Vec<FillSettlement>` and then iterated it twice — once to accrue the per-order scalar deltas and once to build the balance operations. The struct carries several u256-backed `Quantity` fields, so the allocation, store, and reload were paid for on the matching hot path for every fill. Fold the compute and both consumers into one pass over `output.fills`: each fill's notional, fees, and roles are computed exactly once and feed both the balance operations (always) and the per-order deltas (only under `StableMemoryOptions::Write`), preserving the R11 single-source guarantee without the intermediate `Vec`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regenerate `canbench_results.yml` to capture the expected cost of the `filled_quote`/`filled_fee` order-level rollup. The remaining increase on `process_pending_orders` (matching ~+10-14%, the per-fill `qty` rollup +28-35%, `order_history::apply_update` +3-6%) is the inherent per-fill u256 notional/fee arithmetic over up to 1000 fills, which the single-pass settlement already keeps to one computation per fill. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Compute the proptest generator's `filled_quote` the way the engine does (`maker_price × filled_quantity / base_scale`, cf. `Fill::quote_amount`), so generated `OrderRecord`s are semantically consistent with realized quote notional instead of inflated by `base_scale`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Register the new ICP (8-dec) / ckUSDT (6-dec) fixture in the `realized_scalars` worked-example test and restore the ckUSDT labels, so the smallest-unit prices/fees literally mean ckUSDT amounts as in the DEFI-2901 spec. Rewrite the module doc accordingly; base stays ICP so `base_scale = 10^8` and every stored integer is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review comment 3460414419: drop the base_scale formula from the filled_quote doc (it is plainly cumulative quote in quote-smallest units), spell out VWAP as volume-weighted average price, and remove the confusing reservation-surplus sentence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…me scope Address review comments 3460598457, 3460594366, and 3460487183 on the settlement path in record_matching_event: - FillSettlement now borrows the Fill it derives from and stores only the extra computed values (notional, taker/maker fee, surplus) instead of duplicating the Fill's seqs, side, and quantity. - Consolidate the per-order apply-update under a single write gate; the cheap delta accumulation runs in the single fills pass and only the stable-memory writes are gated, so replay still does not double-count. - Rename the stale bench scope status to apply_order_updates, reflecting the whole per-order apply-update it wraps (apply_update keeps its own inner scope). - Drop the R# spec-requirement tags from code comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review comment 3460709715: remove the standalone realized_scalars module name and assert filled_quote/filled_fee from existing tests where they extend naturally — fill_deducts_fees_on_both_sides now checks each side's roll-up (catching a notional/quantity confusion and a taker/maker fee swap), and should_write_once_for_taker_spanning_multiple_fills asserts the summed filled_quote across distinct maker prices. The multi-level-sweep worked example (with surplus exclusion and VWAP) and the both-ways single-batch case move into the fees module as the only dedicated tests, since no existing test covers them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review comment 3460438909: register the trading pair with non-zero maker/taker fee rates so the partially filled buy accrues a non-trivial filled_fee (the maker-rate base fee) alongside a consistent filled_quote, and assert the withheld fee on the buyer's settled base balance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The status bench scope is renamed to apply_order_updates and the FillSettlement rework changes the settlement path, so the persisted baseline keys/values shift accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r-level-scalars # Conflicts: # canister/src/state/mod.rs
The merge of origin/main shifted instruction counts within noise (max +0.02%). Persist the refreshed canbench_results.yml so the regression gate stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e_operations Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review comment 3465971111. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review comment 3465986823: `FillSettlement` now owns the `Fill` (no lifetime parameter), built by consuming `output.fills` in the single settlement pass, with all downstream access routed through `settlement.fill`. Refreshes the canbench baseline for the resulting sub-0.03% instruction drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…se_scale) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move `Fill` and `FillSettlement` into a new `canister/src/order/fill.rs`, rewrite `fill_settlement` as the `FillSettlement::new` constructor, and turn `push_balance_operations` and `accrue_fill` into methods on `FillSettlement`. Condense the single-write-per-order comment at the settlement call site. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r-level-scalars Integrate the DEFI-2853 Fill-or-Kill matching/settlement work from main with this PR's order-level realized scalars: the settlement loop builds one FillSettlement per fill (feeding both balance operations and per-order scalar deltas) and also unreserves the placement reservation of each killed FOK order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… builder Add a `RemovedOrderSettlement` sibling to `FillSettlement` and a unified `settle()` builder so `record_matching_event` no longer pushes balance operations or builds the order-update map inline. `settle()` does a single pass producing both the balance operations and the per-order update map, gating the per-fill `accrue_fill` work under the write path so `Skip`/replay no longer does the per-fill accrue work (the update map is discarded under `Skip` anyway). `cancel_limit_order` now also routes its refund through `RemovedOrderSettlement`, letting the free `refund_for` helper be deleted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the entire settlement of a matching event -- the `settle()` call, the status merges, the `order_history` apply, and the `pending_settling_events` push -- inside the `StableMemoryOptions::Write` branch of `record_matching_event`. Only `process_pending_orders` (the in-memory order-book rebuild) stays outside the gate, since replay needs it. Because `settle` now only ever runs under Write, drop its `write` parameter and make `accrue_fill` unconditional inside it. Behavior is identical: under Skip (post-upgrade replay) the settlement outputs were either discarded -- `replay_events` clears the whole `pending_settling_events` queue at the end -- or no-op'd (`record_settling_event` returns early under Skip), and the `order_history` apply was already gated; under Write nothing changes. The net effect is skipping the wasted balance-operation building and per-fill u256 arithmetic during replay, which also subsumes the prior `accrue_fill`-under-Skip concern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Route the ckUSDT worked-example pair through SupportedTokens::CKUSDT instead of a duplicate ckusdt_metadata fixture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… store Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…to gdemay/DEFI-2901-fill-store # Conflicts: # canister/src/order/mod.rs # canister/src/state/mod.rs
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review thread 3504335473: rename the lean per-fill settling record and its arbitrary test fixture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review thread 3504319662: the returned candid Settling event dropped its per-fill payload. Add a candid FillEvent DTO carrying the same information as the internal record (fill/taker/maker seqs, quantity, and the maker/taker fee rates in basis points) and surface it on the candid SettlingEvent. Regenerate the candid interface accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-store # Conflicts: # canister/canbench_results.yml # canister/src/history/tests.rs # canister/src/order/mod.rs # canister/src/order/trades/tests.rs # canister/src/state/mod.rs # canister/src/test_fixtures/mod.rs
The function resolves each referenced OrderSeq into its owner, side, and price, not only the user. Rename to reflect that it resolves orders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The alias for CursorNotFound is not referenced anywhere; keep the plain CursorNotFound export. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses Copilot review thread 3504739601: FillEvent::trade_legs no longer reconstructs a Fill with taker_price set to maker_price. The shared fees() helper now takes the maker price, quantity, side and fee rates directly, so both the matching phase and the settling phase pass the values they actually have. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses Copilot review thread 3504739531: the settling loop now resolves each fill's taker/maker order seq via get().expect() with a self-documenting BUG message instead of a bare map index that panics with a generic "no entry found". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review thread 3504804236: collapse per-field TradeRecord assertions in the trade-leg reconstruction proptest and the sell-taker side-swap test to a single equality check against a constructed expected record. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… test Addresses review thread 3504855794: the test name settling_event_under_skip_writes_no_fills_and_no_balances is self-describing; remove the multi-line doc comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends review thread 3504804236 to the settle_fills persistence tests: collapse the field-by-field TradeRecord checks in buy_taker_sweeping_two_levels_persists_per_fill_records and order_filling_both_ways_records_a_taker_and_a_maker_fill to a single equality against the full expected records. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add three regression guards for the per-fill persistence landed in the fill-store PR: a stable-memory bytes-per-fill/order accounting test, a fee/notional parity proptest correlating persisted trade legs to their settlement balance operations, and a test pinning the persisted fee to the match-time fee-rate snapshot across a between-phase rate change. Test-only: the touched production files gain only cfg(test) helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
# Conflicts: # canister/src/state/tests.rs
This was referenced Jul 1, 2026
Move the settlement pipeline (FillSettlement, FillEvent, RemovedOrderSettlement, and the fee helpers) out of order::fill into a new top-level crate::settlement module, and replace the free settle() plus the inline resting/filled/expired status merge with a named MatchSettlement built from a MatchingOutput. Pure refactor: no behavior or CBOR-wire change (schema-stability golden test unchanged). Fill and the id types stay in order, since MatchingOutput holds them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The producer returns a FillEvent; align the method name with the type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds regression guards around fill persistence invariants (fee/notional parity, match-time fee-rate snapshots, and per-fill stable footprint), while also refactoring settlement-related types/logic into a dedicated settlement module.
Changes:
- Adds new proptests/unit tests to ensure persisted
TradeRecordlegs match the balance ops emitted from the same settlement and remain pinned to match-time fee rates. - Pins the encoded stable-memory byte footprint per persisted fill via a deterministic encoded-size assertion.
- Refactors settlement computation and
FillEventtype location into a newcanister::settlementmodule and updates call sites/imports.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| canister/src/test_fixtures/mod.rs | Updates arbitrary fixture imports to use settlement::FillEvent. |
| canister/src/test_fixtures/event.rs | Updates fixture helpers to use settlement::FillEvent. |
| canister/src/state/tests.rs | Moves/extends invariant tests; adds new persistence invariants and fee-rate snapshot test (currently contains a compile-breaking duplicate import). |
| canister/src/state/snapshot/tests.rs | Updates snapshot tests to reference settlement::FillEvent. |
| canister/src/state/mod.rs | Refactors matching settlement construction to use MatchSettlement; adds cfg(test) fee-rate setter for tests. |
| canister/src/state/event/mod.rs | Updates event model to use settlement::FillEvent. |
| canister/src/settlement/tests.rs | New focused tests for fill settlement leg reconstruction + settlement shape proptest (moved from order/tests.rs and state/tests.rs). |
| canister/src/settlement/mod.rs | New settlement module containing MatchSettlement, FillSettlement, FillEvent, and RemovedOrderSettlement. |
| canister/src/order/trades/tests.rs | Adds encoded-size “bytes per fill” invariant test. |
| canister/src/order/trades/mod.rs | Adds cfg(test) helper to expose encoded-size accounting for a leg. |
| canister/src/order/tests.rs | Removes settled_fill tests (moved to settlement/tests.rs). |
| canister/src/order/mod.rs | Stops re-exporting fill/settlement types from order::fill. |
| canister/src/order/fill.rs | Removes settlement-related types from order::fill, leaving core fill identity/types. |
| canister/src/order/book.rs | Adds cfg(test) set_fee_rates to support fee-rate snapshot testing. |
| canister/src/main.rs | Updates event mapping to accept settlement::FillEvent. |
| canister/src/lib.rs | Exposes new settlement module from the canister library crate. |
| canister/src/history/mod.rs | Adds cfg(test) helper to compute encoded size of a primary + by-user index entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2546
to
+2550
| use crate::order::{FeeRates, Quantity}; | ||
| use crate::state::event::{BalanceOperation, SettlingEvent}; | ||
| use crate::test_fixtures::arbitrary::{arb_fee_rates, arb_side}; | ||
| use proptest::prelude::Strategy; | ||
| use proptest::{prop_assert_eq, proptest}; |
| pub mod metrics; | ||
| pub mod order; | ||
| pub mod runtime; | ||
| pub mod settlement; |
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
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.
Follow-up work on the merged per-fill persistence (#179), kept separate so #179 stayed lean and approved. Two parts:
Settlement module extraction (pure refactor — no behavior or Candid change).
The settlement pipeline that was scattered across the
orderandstatemodules is pulled into a dedicated top-levelsettlementmodule, and the free settle step plus the order-status merge that was inlined in the matching path are replaced by a single type constructed directly from the matcher's output. The intent is that the flow reads off the types: match result → settled consequences (balance operations, order-record updates, lean persisted fill records) → replayed trades. Behavior and the on-wire (stable-memory) schema are unchanged — the schema-stability golden test is untouched.Fill-persistence regression guards (the non-blocking suggestions from #179's review):
stable_memory_increasereads 0 because the memory manager pre-allocates buckets, so this direct encoded-size assertion pins the growth those buckets absorb).The guards are test-only; the refactor is behavior-preserving.
Spec: docs/src/development/specs/DEFI-2901-persist-fills.md
📚 Follow-up on #179 (merged to
main).