feat(order): add realized quote and fee onto OrderRecord#171
Merged
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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the order persistence and API surface so an order can report realized (executed) economics, not just limit price and filled base quantity. It adds cumulative per-order scalars for realized quote notional and realized fees, computed once per fill during settlement and folded into the existing per-order read/modify/write update path.
Changes:
- Add
filled_quote(realized quote notional) andfilled_fee(realized fee in receive token) toOrderRecordacross internal types, public types, and the hand-written.did. - Refactor matching settlement to compute per-fill realized values once (
fill_settlements) and reuse them for both balance operations and per-order scalar accumulation. - Update fixtures and tests (including new worked-example tests) to validate rollups, overflow trapping, and replay behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/types/src/lib.rs | Extends public OrderRecord with filled_quote/filled_fee fields and docs. |
| integration_tests/tests/tests.rs | Updates integration expectations for canceled partially-filled orders to include realized scalars. |
| canister/src/tests.rs | Updates canister unit tests’ expected OrderRecord values with new fields. |
| canister/src/test_fixtures/mod.rs | Extends arbitrary OrderRecord generator with populated filled_quote/filled_fee. |
| canister/src/state/tests.rs | Updates existing settlement tests and adds worked-example tests for realized scalars. |
| canister/src/state/mod.rs | Introduces FillSettlement, computes settlements once, accrues per-order deltas, and builds balance ops from settlements. |
| canister/src/order/history/tests.rs | Adds/updates tests to cover accumulation and overflow trapping for new scalars. |
| canister/src/order/history/mod.rs | Adds filled_quote/filled_fee to the stable-minicbor OrderRecord and extends OrderUpdate to carry new deltas. |
| canister/oisy_trade.did | Evolves the Candid OrderRecord type to include the new fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
gregorydemay
commented
Jun 23, 2026
Contributor
Author
|
🤖 Automated review — PR 1 of the DEFI-2901 delivery (order-level scalars), against the spec and the team review rules. (Posted as a comment because GitHub won't let me file a formal request-changes review on a PR owned by the same account.) VerdictThe code is substantively sound and scoped correctly. The only thing gating READY is CI: Severity tally: 🔴 0 · 🟠 0 · 🔵 1 (inline, comment/fixture naming). What I verified locally (all green)
Acceptance criteria
Maintainability sweep
ScopeCorrectly limited to order-level scalars — no VERDICT: CHANGES_REQUESTED |
…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>
gregorydemay
commented
Jun 23, 2026
gregorydemay
left a comment
gregorydemay
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Automated re-review — PR 1 of the DEFI-2901 delivery (order-level scalars). Follow-up to my prior CHANGES_REQUESTED (gated on pending CI + one nit).
Verdict
All prior gates cleared. CI is green and every open thread is resolved with new commits. Ready for your review.
Severity tally: 🔴 0 · 🟠 0 · 🔵 0.
Re-verification of what changed since last pass
- Single-pass settlement (R11).
record_matching_eventnow iteratesoutput.fillsonce: each fill's realized values are computed exactly once viafill_settlement(...), feedingpush_balance_operations(...)(always) and the per-orderaccrue_fill(...)deltas (only underStableMemoryOptions::Write). Thewritegate is intact — both theaccrue_fillcalls and theupdates-map application sit insideif write. The single-source guarantee holds: there is oneFillSettlementper fill and both consumers read it. Thecompute_balance_operationsshape proptest was updated to drive the samefill_settlement+push_balance_operationspath, so the op-shape coverage is preserved. arb_order_record(Copilot #2).filled_quoteis now derived as the engine does —price.checked_mul_quantity_scaled(&filled_quantity, base_scale=10^8)(cf.Fill::quote_amount) — no longer inflated bybase_scale. Both new minicbor fields (#[n(9)]/#[n(10)]) are fuzzed (not hard-coded sentinels) and exercised byseq_order_record_roundtrips_through_storable.- Worked-example test (my nit + Copilot #3).
realized_scalarsis now a literal ICP/ckUSDT (8/6) instance viaicp_ckusdt_trading_pair()/ckusdt_metadata(); the misleading "decimals irrelevant" doc is gone and the ckUSDT labels are correct. Stored smallest-unit assertions are unchanged (base stays ICP,base_scale = 10^8). - CBOR
#[cbor(default)](Copilot #1). Correctly declined: consistent with the module's documented pre-launch schema policy and the sibling non-Optionfieldsfilled_quantity(n(6)) /time_in_force(n(8)); backfilling pre-existing orders is a spec non-goal.
Local verification (all green)
cargo test -p oisy_trade_canister— 353 passed;check_candid_interface_compatibilitypasses (two trailingnatfields on a returned record is a backward-compatible Candid evolution).gh pr checks 171— all green; canbench reportscanbench_results.ymlup to date.- Coverage by mutation (reverted): mapping
quote_delta'snotional → quantity, and swappingtaker_fee ↔ maker_feeinaccrue_fill, each fail the worked-example state tests. The assertions pin the realized notional and which fee accrues to which leg.
Canbench cost assessment
The baseline refresh is appropriate for PR-1 and the implementer's characterization is accurate. fill_settlement computes the same notional/quote_fee/base_fee the old compute_balance_operations did — moved, not duplicated. The residual qty-scope growth (+28-35%) is the genuinely-new per-fill u256 quote_delta/fee_delta rollup that R1/R2 require — inherent to the feature, not avoidable without dropping it. R12's dedicated with/without-persistence benchmark lands in PR-2.
Acceptance criteria
- R1/R6 (order-level) —
filled_quote = Σ maker_price × quantity / base_scale; buy-taker surplus stays theUnreserveop, excluded. Worked example: 53 ckUSDT, VWAP 10.6, 7-ckUSDT surplus released. ✅ - R2 —
filled_feeis the realized withheld amount in the receive token (base buy / quote sell), summed, never rate-derived; per-leg rates pinned by the cross-then-rest test. ✅ - R7 —
quote_delta/fee_deltafold into the same singleOrderUpdateread-modify-write; no-op still writes nothing. ✅ - R9 — both scalars monotonic via
checked_add+ always-on.expect("BUG: …"); dedicated#[should_panic]trap tests (notdebug_assert!). ✅ - R11 — one
FillSettlementper fill feeds both balance ops and order deltas. ✅
Maintainability sweep
- Duplication: none — the refactor de-duplicates fee/notional into
fill_settlement. - Unused derives: none —
OrderUpdate'sDefault/PartialEq/Eqall exercised; new fields add no capability. - Primitive-obsession:
accrue_fill(update, quantity, notional, fee)takes threeQuantitypositional args; all are the domain newtype, internal helper with a clear doc — below the nit threshold. Cleared. - Divergent invariant handling: consistent —
filled_delta/quote_delta/fee_deltaall usechecked_add+BUG:trap identically. - Silent fallbacks: none introduced — no
unwrap_or_default/ok()/NaN/let _ =; thesurpluspath traps with aBUG:message.
Scope
Correctly limited to order-level scalars — no FillStore/Trade/get_my_trades/new MemoryIds/canbench micro-bench pulled in from PRs 2-3.
VERDICT: READY
Contributor
Author
|
🤖 This PR is ready for your review. The spec-driven build loop has completed for DEFI-2901 PR 1 (order-level scalars):
Acceptance criteria R1, R2, R6 (order-level), R7, R9, R11 are met. Scope is limited to PR 1 — the fill store, Note for your review: the order-level rollup adds ~+10–14% on the Left as a draft — marking ready, approval, and merge are yours. |
gregorydemay
commented
Jun 23, 2026
gregorydemay
left a comment
gregorydemay
left a comment
Contributor
Author
There was a problem hiding this comment.
Preliminary Review
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>
gregorydemay
commented
Jun 23, 2026
gregorydemay
left a comment
gregorydemay
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Automated re-review — PR 1 of the DEFI-2901 delivery (order-level scalars). Follow-up to the prior READY verdict, re-verifying the seven addressed comments and the origin/main merge.
Verdict
All prior gates remain cleared, CI is green, and every one of the changes since the last pass holds up under independent verification. Ready for your review.
Severity tally: 🔴 0 · 🟠 0 · 🔵 0.
Re-verification of the changes since last pass
- Candid docs simplified —
oisy_trade.didfilled_quote/filled_feenow describe smallest-unit semantics + VWAP in prose, nobase_scaleformula, no reservation-surplus sentence. (Thelibs/typesRust doc keeps the R1 formula — that is the public Rust surface, not the candid, so the simplification request is satisfied.) - Integration test exercises fees —
should_cancel_partially_filled_buy_and_refund_residualnow registers 10/23 bps maker/taker, asserts non-zerofilled_fee = 1_000, consistentfilled_quote, and the withheld base fee reflected in the post-cancel free balance (999_000). - PR description — Performance-impact and Requirements-coverage sections present.
R#tags removed — code comments carry no requirement tags;expect("BUG: …")strings retained.- Single
if write+ scope rename — confirmed sound. The per-fillfill_settlement+accrue_fillloop runs once overoutput.fills; theupdatesmap andbalance_operationsare built unconditionally, butapply_update(the only stable-memory write) stays insideif write. The Skip-path cost is just threechecked_adds per fill into a discarded map —balance_operationswas already built unconditionally before this PR (oldcompute_balance_operationssat outside the gate), so the only new replay-path work is negligible and cannot double-count. Acceptable as-is; re-gating would buy nothing measurable. Thestatus→apply_order_updatesbench scope rename is reflected incanbench_results.yml. FillSettlementborrowsFill—fill: &'a Fillplus the four derived fields (notional,taker_fee,maker_fee,surplus); no field duplication. All four are read bypush_balance_operations/accrue_fill. R11 single-source holds: oneFillSettlementper fill feeds both the balance ops and the per-order deltas.realized_scalarsmodule removed — coverage folded intofees::*: the parameterizedfill_deducts_fees_on_both_sidesnow also asserts each side'sfilled_quote/filled_fee(buyer base-fee, seller quote-fee, never swapped), plus the two kept dedicated tests (multi-level sweep worked example; both-ways single batch).
The origin/main merge is clean; the effective diff vs main is exactly the PR-1 work (10 files) — no FillStore/Trade/get_my_trades/new MemoryIds pulled in.
Coverage by evidence (mutation, reverted)
accrue_fillnotional → quantity: caught by 4fees::*tests (worked-examplefilled_quote53M vs the quantity sum).accrue_filltaker_fee ↔ maker_fee swap: caught by the same 4 tests.
Both mutation guarantees the task called out are real.
Local verification
cargo test -p oisy_trade_canister— 386 passed, 0 failed;check_candid_interface_compatibilitypasses (two trailingnaton a returned record is a backward-compatible candid evolution).gh pr checks 171— all green (incl.candid-backward-compat,benchmark,integration-tests).
Acceptance criteria
- R1 / R6 (order-level) —
filled_quote = Σ maker_price × quantity / base_scale; buy-taker surplus stays theUnreserveop, excluded. Worked example: 53 ckUSDT, VWAP 10.6, 7-ckUSDT surplus released. ✅ - R2 —
filled_feerealized withheld amount in receive token (base buy / quote sell), summed, never rate-derived. ✅ - R7 —
quote_delta/fee_deltafold into the same singleOrderUpdate::applyread-modify-write; no-op writes nothing. ✅ - R9 — both scalars monotonic via
checked_add+ always-onBUG:trap; dedicated#[should_panic]tests, notdebug_assert!. ✅ - R11 — one
FillSettlementper fill feeds balance ops and order deltas. ✅
Maintainability sweep
- Duplication: none —
fill_settlementde-duplicates the fee/notional computation; the proptest drives the same path. - Unused derives: none —
OrderUpdate'sDefault/Copy/PartialEq/Eqall exercised;FillSettlementhas no derives, every field read. - Primitive obsession:
accrue_fill(update, quantity, notional, fee)takes threeQuantitynewtypes (the domain primitive), internal helper with a clear doc — below the nit threshold. Cleared. - Divergent invariant handling: consistent —
filled_delta/quote_delta/fee_deltaall usechecked_add+BUG:trap identically. - Silent fallbacks: none introduced — no
unwrap_or_default/ok()/NaN/let _ =; the surplus path traps with aBUG:message.
Note (non-blocking)
The 11 review threads (Copilot + author) each carry a substantive reply tied to a commit, but several are not yet marked "Resolved" in the GitHub UI. The content is fully addressed; flagging only so the threads can be closed out before merge.
VERDICT: READY
Contributor
Author
|
🤖 Ready for your review again — your 7 review comments are all addressed and CI is green.
One small thing for your judgment (flagged on the relevant thread): to satisfy "single Left as a draft — marking ready, approval, and merge are yours. |
…r-level-scalars # Conflicts: # canister/src/state/mod.rs
…play Move the refund settlement of a canceled limit order -- the `RemovedOrderSettlement` balance-operation production and the `pending_settling_events` push -- inside the `StableMemoryOptions::Write` branch of `record_cancel_limit_order`, alongside the already write-gated `order_history` status apply. Only the in-memory `book.remove_order(seq)` mutation stays outside the gate, since the state rebuild on replay needs it. This mirrors the matching-side fix in c2e8354. Behavior is identical: under Skip (post-upgrade replay) the SettlingEvent was either no-op'd (`record_settling_event` returns early under Skip) or discarded (`replay_events` clears the whole `pending_settling_events` queue at the end); under Write nothing changes. The net effect is skipping wasted refund computation and queue churn on replay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gregorydemay
commented
Jun 25, 2026
gregorydemay
left a comment
gregorydemay
left a comment
Contributor
Author
There was a problem hiding this comment.
🧐 Re-review at head 772edb2 — Review passed, no blockers/mediums, CI green. Ready for human approval.
Re-verified the five commits landed since the prior READY; acceptance (R1, R2, R6 order-level, R7, R9, R11) still holds.
f676083(test-only): completed the second worked-example table row ("taker on entry then maker within one batch") so all three orders now carryExpects. Arithmetic checks out per the per-fill role: SELLER maker fee 10_000, BUYER 200_000 (taker leg) + 150_000 (maker leg), MAKER_B taker-leg quote fee 30_000. Strengthens coverage.d1d23b4(test-only): three identical localrecord_ofcopies collapsed into one generictest_fixtures::record_of; 15 call sites routed to it; one definition remains. Same lookup, pure dedup, no coverage change.f0e21a0(test-only): three fill-result sites now useassert_eq_ignoring_timestamp(&record, &OrderRecord{..})— a whole-record compare that pins MORE fields than the prior per-field asserts (owner/side/price/quantity/time_in_force) and normalizes only the two timestamp fields, so thecreated_at/last_updated_atvalues pulled fromactualare not tautological. The replay-under-Skip site keeps its separatelast_updated_at == Noneassertion (the one field the helper ignores).933e131(test-only): worked-example loop swapped tomock_runtime_for_timer(); correct, since timer-driven matching readstime()but nomsg_caller()— a stronger guard (mockall would panic on an unexpected caller read).772edb2(only production change):record_cancel_limit_ordernow gates theRemovedOrderSettlementrefund +pending_settling_eventspush underWrite, mirroring the matching-side fix inc2e8354; onlybook.remove_order(seq)stays unconditional.base_scale_for_bookis a pure read computed before the gate in both old and new code, so behavior on the Write path is identical and replay just skips wasted work. Verified by mutation: dropping the moved push fails 9 tests on BOTH the Write path (balance-refundassert_balance) and the replay path (assert_replay_matches/should_replay_cancel_*); reverted the mutation.
Maintainability sweep on the changed lines — duplication: none introduced (commits net-remove it); unused derives: none (no new types); primitive-obsession params: none (no new signatures, record_of is typed); divergent invariant handling: none — the cancel gate now CONVERGES with the matching-side gate rather than diverging; silent fallbacks: none — the moved code keeps only always-on expect("BUG: ...") traps.
Evidence: cargo test -p oisy_trade_canister --lib 395/395 pass (incl. cancel_limit_order::*, should_replay_cancel_*, replay_under_skip_does_not_write_history, order::history R7/R9 apply+overflow traps, the worked-example table). No .did / libs/types / canbench_results.yml changes (Write path identical) — consistent with green candid-backward-compat and benchmark. gh pr checks 171: all green, none pending.
VERDICT: READY
Contributor
Author
|
🤖 Ready for your review again — reviewer READY, CI 12/12 green, MERGEABLE, all threads answered. Since the last pass: addressed your 5 test nits ( Left as a draft — approval/merge are yours. |
gregorydemay
commented
Jun 26, 2026
|
✅ No security or compliance issues detected. Reviewed everything up to 0b2fb0b. Security Overview
Detected Code Changes
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gregorydemay
commented
Jun 26, 2026
gregorydemay
left a comment
gregorydemay
left a comment
Contributor
Author
There was a problem hiding this comment.
🧐 Re-review at head e28d073 — prior READY verdict holds.
Two commits landed since the last READY, both strictly TEST/FIXTURE-only (no production code):
3fc7aed—ckbtc_token_id()/ckusdt_token_id()/icp_token_id()incanister/src/test_fixtures/mod.rsnow delegate toSupportedTokens::{CKBTC,CKUSDT,ICP}.token_id().into(). Behavior-identical:From<oisy_trade_types::TokenId>is a pure field copy (Self(value.ledger_id)), andSupportedTokenscarries the same principals (mxzaz-…,cngnf-…,ryjl3-…). No coverage lost; removes the duplicated principal literals (one source of truth).e28d073— drops a 3-line redundant comment atopshould_cancel_partially_filled_buy_and_refund_residual; code unchanged.
These two commits touch only 4 lines in test_fixtures/mod.rs and delete 3 comment lines in integration_tests/tests/tests.rs. No production code changed since the previously-reviewed head; in-scope criteria (R1, R2, R6 order-level, R7, R9, R11) untouched. PRs 2–3 (fill store / get_my_trades) remain out of scope.
Maintainability sweep on the delta: duplication — improved (principal literals deduplicated), none introduced; unused derives — none (no new types); primitive-obsession params — none (no signatures changed); divergent invariant handling — none; silent fallbacks — none; test-only code in production modules — none (changes are in test_fixtures / tests).
CI: all green at e28d073 (12/12 reported). Unit tests pass locally (353/353). All review threads answered with a reply + commit; the few not-yet-resolved threads await only the human resolve action, not implementer work.
No blockers, no mediums, no new nits. Ready for human approval.
VERDICT: READY
Contributor
Author
|
🤖 Ready for your review again — reviewer READY, CI 12/12 green, MERGEABLE, all threads answered. This round was test/fixture-only: the token-id fixtures now delegate to |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mbjorkqvist
approved these changes
Jun 29, 2026
mbjorkqvist
left a comment
mbjorkqvist
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thanks @gregorydemay! I couldn't find anything that wasn't already covered in one of the earlier review rounds.
gregorydemay
added a commit
that referenced
this pull request
Jun 29, 2026
Bring the fill-store branch current after #171 squash-merged to main: integrate the realized quote + fee OrderRecord scalars (now in their final reviewed form on main) with the fill-store additions, keep the redesigned TradeHistory / FillId spec, and regenerate the canbench baseline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Purpose
get_my_ordersexposes the original limit price and cumulative basefilled_quantity, but nothing about the price(s) actually traded — so the realized notional, the volume-weighted average price (VWAP), and the fees paid cannot be recovered from an order. This adds the first, order-level layer of that missing data: two cumulative scalars folded into the write that already updatesfilled_quantity.Requirements coverage
filled_quoteaccumulates the realized quote notional per fill at the maker price.filled_feeaccumulates the realized fee in the order's receive token (the amount withheld).OrderRecordthroughget_my_orders/get_my_order.filled_quantityand only under the write gate, so replay does not double-count.Performance impact
Measured with canbench (
just bench-check); deltas are versus the pre-feature baseline.matchingscope: ~+10–14% from the inherent per-fill u256 notional and fee rollup.qtyrollup: ~+28–35%, dominated by the u256Quantityarithmetic the realized notional/fee require (this is intrinsic to the feature, not allocation overhead).order_history::apply_update: ~+3–6% for writing the two extra scalar fields.Vec<FillSettlement>is materialized — so the residual cost is the per-fill u256 rollup itself, not allocation.canbench_results.ymlis unchanged by that gating (just bench-checkreports no change versus the committed baseline).statusbench scope is renamedapply_order_updates(it wraps the whole per-order apply-update), so its baseline key/value changed accordingly.📚 PR stack
main.get_my_trades { ByOrder }feed — base feat(order): persist per-fill records in stable memory (3/5) #179.ByAccountfilter — base feat(order): expose get_my_trades ByOrder feed (4/5) #186.🤖 Generated with Claude Code