feat(order): persist per-fill records in stable memory (3/5)#179
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>
|
|
🧐 Reviewer verdict — DEFI-2901 PR 2 of 3 ("Fill store + per-order feed") Reviewed only PR 2's incremental commit (53d475c) on top of PR #171; did not re-review PR 1's order-level scalars. What is solid (no changes needed):
🔴 Blocker — end-to-end pagination is non-functional (the design tension, resolved against the PR). 🔵 Nit — CI: lint, unit-tests, benchmark, candid-backward-compat, reproducible-build all green; Severity tally: 1 blocker, 0 medium, 1 nit. Plus pending CI. VERDICT: CHANGES_REQUESTED |
|
🤖 Fixed the blocker via resolution (a) in commit c30ed99. The cursor is now surfaced on Pagination now round-trips end to end:
Docs fixed to match: the Nit on Verified locally: |
|
🧐 Reviewer re-review — DEFI-2901 PR 2 of 3 ("Fill store + per-order feed"), at c30ed99. The single blocker from my prior review — non-functional pagination (no cursor on the public Verified:
No unresolved review threads or inline comments outstanding. The review substance is clean — no blockers, no mediums, no new nits. The ONLY thing holding back a READY verdict is CI: Severity tally: 0 blockers, 0 mediums, 0 new nits. Blocked solely on pending CI. VERDICT: CHANGES_REQUESTED |
|
🧐 Final reviewer verdict — DEFI-2901 PR 2 of 3 ("Fill store + per-order feed"), at c30ed99. The substance was already certified clean in my prior re-review pass (no blockers, no mediums, no nits) — the only thing holding back a READY was three pending CI checks. Both conditions I set are now satisfied:
Ready for your approval. VERDICT: READY |
|
🤖 Ready for your review — reviewer verdict is READY, CI 11/11 green, MERGEABLE. PR 2 of 3 (fill store + per-order This round resolved the one reviewer blocker (commit c30ed99): each Scope held to PR 2: Left as a draft — marking ready, approval, and merge are yours. |
…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>
gregorydemay
left a comment
gregorydemay
left a comment
There was a problem hiding this comment.
🧐 VERDICT: READY (substance) — 0 blockers, 0 mediums, 0 nits; CI green except integration-tests still pending.
Review details
Focused re-review of the changes since the prior READY: the public candid FillEvent exposure, the SettledFill→FillEvent rename, the main merge (generic History refactor), the resolve_op_users→resolve_op_orders rename, and the TradeCursorNotFound cleanup.
1. Public candid change. The map_fill_event mapping (canister/src/main.rs) is total and correct: fill_seq/taker_order_seq/maker_order_seq via .get() → nat64, quantity.into() → nat, fee_rates.maker.get()/fee_rates.taker.get() → maker_fee_bps/taker_fee_bps (nat16). No misorder, no data loss. SettlingEvent/FillEvent appear only in the get_events return path (never as an input), so the addition is a backward-compatible Candid evolution — candid-backward-compat and check_candid_interface_compatibility both pass, so the .did matches the code-derived interface. The maker/taker swap is pinned by the integration test's distinct 10/23 bps values, so a transposed mapping would fail.
2. FillEvent rename. Complete — no stray SettledFill anywhere in the branch. The internal FillEvent is the lean event-log record (FillSeq/OrderSeq/Quantity/FeeRates, minicbor #[n(..)]), distinct from the candid DTO; both legs are persisted via TradeHistory::append.
3. main merge / generic-History refactor. Taken cleanly: OrderHistory and TradeHistory are both newtype wrappers over the shared History<M,K,V> core (the sibling-unification pattern). arb_trade_record is de-duplicated to a single definition in test_fixtures/mod.rs and fuzzes all 8 fields. trade_history is written in production (state/mod.rs settling loop) with no #[allow(dead_code)].
4. Renames/cleanup. resolve_op_orders returns ResolvedOrder { user, side, price }; TradeCursorNotFound fully removed (only the shared CursorNotFound remains). FillId/TradeId are now genuinely used, so their prior dead-code allows are correctly gone.
5. Earlier invariants still hold. Settling persistence is Skip-gated (early return) — replay-safe; owners resolved via resolve_op_orders; taker side and maker price recovered from the order records; notional/fees recomputed from the pinned fee_rates snapshot through the shared fees() helper (R11 single source). The settling_event_under_skip_writes_no_fills_and_no_balances test asserts both no-fills AND no-balance-movement under Skip vs. exactly-once under Write; the audit harness captures the real SettlingEvent (with fills) for event-log replay; the snapshot test roundtrips pending_settling_events with FillEvent.
Maintainability accounting.
- Duplication (within diff): none found —
fees()is the single shared source;map_fill_eventfollows the existingmap_*DTO-boundary pattern inmain.rs. - Structural duplication (repo-wide): none — the merge unifies
OrderHistory/TradeHistoryonto the genericHistorycore rather than mirroring. - Unused derives: none found on
FillEvent(Clone/PartialEq/Eq/Debug used in tests + Encode/Decode for the event log). - Primitive-obsession params: none — internal
FillEventuses domain newtypes; the candid DTO's bareu64/u16is the expected wire boundary. - Divergent invariant handling: none — the
BUG:panics for missing order/owner inresolve_op_ordersmatch theSkip-gate/expect convention used elsewhere in settling. - Silent fallbacks: none found.
- Test-only code in production modules: none —
arb_fill/arb_fee_rates/arb_trade_recordlive intest_fixtures.
Tests. rebuilds_legs_identical_to_the_matching_computation is not a parallel-oracle liability: it pins R11's central invariant that the split matching-phase and settling-phase computations stay consistent (the exact risk the refactor introduces), deriving expectations from the real balance ops. Buy/Sell, taker/maker, and both roles-in-one-order are all exercised end-to-end. cargo test -p oisy_trade_canister — 422 passed, 0 failed; check_candid_interface_compatibility passes.
Only outstanding gate: the integration-tests CI job is still pending (it covers the candid FillEvent mapping end-to-end). Substantively the PR is clean; flip to READY once that job goes green.
|
🧐 VERDICT: READY (substance) — 0 blockers, 0 mediums, 0 nits; CI green except Review detailsFocused re-review of the changes since the prior READY: the public candid 1. Public candid change. The 2. 3. 4. Renames/cleanup. 5. Earlier invariants still hold. Settling persistence is Maintainability accounting.
Tests. Only outstanding gate: the |
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>
|
✅ No security or compliance issues detected. Reviewed everything up to 324cdb5. Security Overview
Detected Code Changes
|
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @gregorydemay!
I was confused by that fact that even though we're here persisting records in stable memory, the canbench report shows stable_memory_increase: 0 for every fill/settling bench, including bench_gtc_fill_full_bid_side::fills::append (697 appends, 206M instructions). IIUC, the fills are persisted, but canbench's SMI counts only stable_grow page allocations inside the measured closure, and MemoryManager allocates in large multi-MiB buckets (128 pages = 8 MiB by default) whose first bucket is claimed at StableBTreeMap::init time in new_state(), i.e. during setup, before bench_fn. Each bench then writes at most a few hundred KB of trade legs (gtc: ~697 fills x 2 legs x ~140 B ~= 150 KB), which fits inside that pre-allocated bucket, so no stable_grow fires in the measured window -> SMI = 0 (same reason heap_increase is 0 there). One way to track the stable memory usage could be to add a byte-accounting unit test rather than leaning on canbench SMI: Append N fills to a fresh TradeHistory and assert the backing regions' page delta (trades_memory().size() before/after) or the summed minicbor-encoded size of a representative SeqRecord<TradeRecord> + TradeId key + by-user entry, pinning bytes-per-fill (and, mirrored, bytes-per-order) as a tracked, regression-guarded number. A canbench sized to cross bucket boundaries would also surface SMI, but it's 8-MiB-granular and noisier than a direct byte assertion.
|
🤖 @mbjorkqvist Good call on the SMI/bucket explanation — canbench reports |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Persists every execution as a side-projected per-fill record in stable memory — the granular layer beneath the merged order-level scalars. Builds on the trade store landed in #193: this PR wires the producer that writes fills into that store on settlement. To keep the matching→settling event boundary small, the settling event now carries a lean, normalized per-fill record (fill identity, quantity, fee-rate snapshot) instead of the full settlement; the settling phase recovers each fill's side and execution price from the order records and recomputes its notional and fees before persisting the two trade legs. Persistence runs under the existing
Writegate so replay does not double-write. No retrieval endpoint ships here; theget_my_tradesfeed is #186.Spec: docs/src/development/specs/DEFI-2901-persist-fills.md
Requirements coverage
priceis the maker's price,notionalthe executed notional,feethe side's realized amount,is_makerthe per-fill role.Write-gated and replay-safe; records are durable across upgrade.Performance impact
Measured against
mainvia the full-Binance-snapshot sweep benches. Splitting the fill benches by phase shows the cost is concentrated entirely in settling — matching stays flat, confirming it never touches stable memory:gtc_fill_full_bid_sideprocess_pending_orders_1000process_pending_orders_1000_with_fees(
fok_fill_full_bid_sideis identical togtc_fill_full_bid_side— same fill path.)SettledFillon the heap; it never persists.write_events22.3M → 28.9M (+29%),read_events14.9M → 18.8M (+26%) — the cost of the lean record onSettlingEvent(vs. re-serializing the full settlement, which would cost ~3× more). Worst-caseSettlingEventis 208,030 B.notionalis computed once per fill (the sharedfees()returns it), avoiding a redundant per-fill bignum recompute.Writegate, so post-upgrade replay pays none of it — the no-fills and upgrade benches are unchanged (<0.5%).📚 PR stack
main.main.main.🤖 Generated with Claude Code