feat(order): expose the account-wide get_my_trades ByAccount filter (5/5)

[gregorydemay]

Final PR of the persist-fills stack: the thin account-wide trades feed.

The per-order feed PR (#186) left the get_my_trades { ByAccount } arm as an empty-page stub, and the account-wide storage (the by-user index, the account-wide reader, and the settlement wiring that populates it) now lives in the fill-store PR (#179). This PR is the thin API layer that wires the ByAccount arm to that storage.

What this delivers:

• A state-level reader that resolves the caller's user identity and returns their trades across all their orders, newest-first, via the fill store's account-wide reader.
• The get_my_trades { ByAccount { after, length } } arm: owner-scoped, malformed TradeId cursor -> Err(RequestError(...)), unknown cursor -> Ok([]), length clamped to the max, each returned Trade carrying its own id.
• Unit tests for the ByAccount arm (cross-order newest-first, paging without overlap or gap, owner scoping, unknown/empty cursor, malformed cursor, clamping, unregistered caller) and an end-to-end PocketIC test over three orders.

The Candid surface is unchanged: the ByAccount variant and TradeId cursor already existed in the type from #186; this PR only wires behavior. The check_candid_interface_compatibility test passes. No storage, index, settlement, or canbench changes — those land in #179.

Spec: docs/src/development/specs/DEFI-2901-persist-fills.md.

📚 PR stack

• ✅ refactor(ids)!: shared id/seq machinery and composite ids (1/4) #192 — shared id/seq machinery — merged to main.
• ✅ feat(order): add the trade store and its read API (2/5) #193 — trade store + read API — merged to main.
• ✅ feat(order): persist per-fill records in stable memory (3/5) #179 — persist per-fill records on settlement — merged to main.
• ✅ feat(order): expose get_my_trades ByOrder feed (4/5) #186 — per-order `get_my_trades { ByOrder }` feed — merged to main.
• feat(order): expose the account-wide get_my_trades ByAccount filter (5/5) #180 — account-wide `get_my_trades { ByAccount }` filter (this PR) — base main.
• refactor(settlement): extract a settlement module and harden fill-persistence tests #195 — fill-persistence hardening tests — base main (sibling test-only PR).

🤖 Generated with Claude Code

[gregorydemay]

🧐 Review — PR 3/3 (Account-wide ByAccount filter), R4.

Substance: clean. No blockers, no mediums, no nits. The incremental work faithfully mirrors the established OrderHistory by_user / orders_after shape and correctly reuses PR 2's primitives.

Verified against the in-scope acceptance criterion (R4) and its sub-points:

• Write-gate / replay safety (R8): the two by_user index inserts happen inside FillStore::append, called only within the existing if write block in record_matching_event (state/mod.rs ~L438). The Skip replay path writes nothing — no double-write. ✓
• Index↔record consistency: append writes both side-projected records and both index entries atomically (2 + 2); no path writes one without the other. trades_after resolves index→record and guards a dangling reference with a BUG: expect. ✓
• Ownership: the account scan is keyed by the caller's resolved UserId (get_user_trades → user_registry.lookup); an unregistered caller gets Ok([]). A cursor seq belonging to another user falls outside this user's (user, seq) prefix → CursorNotFound → Ok([]). No way to read another principal's fills. ✓
• Cross-order, newest-first: trades_after reverse-scans the account-wide index, genuinely spanning distinct orders (unit + state + integration tests assert order ids across multiple orders, descending). ✓
• R5 / R10 dispositions: the ByAccount arm mirrors ByOrder exactly — malformed after → Err(InvalidCursor), unknown cursor → Ok([]), length clamped to MAX_FILLS_PER_RESPONSE. ✓
• Cursor reuse: ByAccount parses the same FillSeq cursor established in PR 2; each Trade carries its own cursor and order_id. ✓
• order_owner_user's two BUG: expects are unreachable on the Write path (placement records order_history + registers the user under the same gate before any fill can settle) and match the codebase's existing BUG: convention — consistent invariant handling, not a silent fallback.

Tests: thorough at every layer (unit trades_after cross-order/owner-scope/paging/unknown-cursor/clamp/unregistered; state-level multi-order + Skip-replay; PocketIC end-to-end over three orders with pagination no-overlap/gap, owner-scoping, unknown-cursor empty, malformed-cursor RequestError, and account↔per-order agreement). The PR 2 empty-page stub test was correctly replaced. All 425 canister unit tests pass locally. .did is unchanged (candid-backward-compat CI job passed).

Blocking reason — CI not green. gh pr checks 180 shows lint, unit-tests, reproducible-build, and benchmark still pending. Pending CI is a hard block on approval independent of review substance. Re-run once CI completes green and this flips to ready.

Tally: 0 🔴 / 0 🟠 / 0 🔵 — CI pending.

(Note: GitHub blocks --request-changes on one's own PR, so this verdict is posted as a conversation comment.)

VERDICT: CHANGES_REQUESTED

[github-actions]

canbench 🏋 (dir: canister) 5af2c1c 2026-07-02 10:15:40 UTC

✅ canister/canbench_results.yml is up to date
📦 canbench_results_benchmark.csv available in artifacts

---------------------------------------------------

Summary:
  instructions:
    status:   No significant changes 👍
    counts:   [total 14 | regressed 0 | improved 0 | new 0 | unchanged 14]
    change:   [max +489 | p75 0 | median 0 | p25 -138.01K | min -204.49K]
    change %: [max 0.00% | p75 0.00% | median 0.00% | p25 -0.01% | min -0.02%]

  heap_increase:
    status:   No significant changes 👍
    counts:   [total 14 | regressed 0 | improved 0 | new 0 | unchanged 14]
    change:   [max 0 | p75 0 | median 0 | p25 0 | min 0]
    change %: [max 0.00% | p75 0.00% | median 0.00% | p25 0.00% | min 0.00%]

  stable_memory_increase:
    status:   No significant changes 👍
    counts:   [total 14 | regressed 0 | improved 0 | new 0 | unchanged 14]
    change:   [max 0 | p75 0 | median 0 | p25 0 | min 0]
    change %: [max 0.00% | p75 0.00% | median 0.00% | p25 0.00% | min 0.00%]

---------------------------------------------------
CSV results saved to canbench_results.csv

[gregorydemay]

🧐 Review passed — no blockers/mediums/nits, CI green. Ready for human approval.

Substance was clean in the prior pass (PR 3/3, account-wide ByAccount filter, R4). CI is now fully green at unchanged head 83ecec1 — all checks pass (lint, unit-tests, integration-tests, reproducible-build, reproducibility-verify, benchmark, candid-backward-compat, and PR-meta checks). This completes the DEFI-2901 three-PR stack.

VERDICT: READY

[gregorydemay]

🤖 Ready for your review — reviewer verdict is READY, CI 11/11 green, MERGEABLE.

PR 3 of 3 (final) for DEFI-2901 — account-wide get_my_trades { ByAccount }, stacked on #179.

Adds the FILLS_BY_USER_MEMORY_ID (MemoryId 8) (UserId, FillSeq) → FillKey secondary index and trades_after, mirroring OrderHistory's by_user / orders_after. The ByAccount arm (previously a PR-2 empty-page stub) now returns the caller's fills across all their orders, newest-first, paginated by the same Trade.cursor → after convention as ByOrder. Settlement writes the two by-user index entries alongside the side-projected fill records (2 + 2 inserts/fill) inside the existing Write gate, so post-upgrade replay (Skip) writes nothing. Owner-scoped — another principal's fills never appear. .did unchanged (the arm already existed from PR 2), candid backward-compat passes.

With this, the spec's full requirements set (R1–R12) is delivered across #171 / #179 / #180.

Left as a draft — marking ready, approval, and merge are yours.

[gregorydemay]

🧐 Re-review after the stacked cascade merge (7a0ef75, DEFI-2853 FOK absorbed via base). Read-only; verifying the merge is correct and behavior-preserving for PR-3's R4 work — not re-reviewing PR-1/PR-2.

Merge integration — correct. I diffed the merge head against both parents (83ecec1 = PR-3's prior tip, 7e09676 = the FOK-bearing base). The conflict in record_matching_event (canister/src/state/mod.rs) was resolved correctly: under the single if write gate ALL three coexist, none dropped —

• per-fill FillSettlement → balance ops + per-order scalar deltas (PR-1);
• FillStore::append(taker_leg, taker_user, maker_leg, maker_user) writing 2 records + 2 (UserId, FillSeq)→FillKey by-user index entries, with owners resolved via order_owner_user from the correct taker_leg/maker_leg.order_id (PR-3's new 4-arg signature kept over the base's 2-arg one);
• the FOK expired_orders kill-refund loop + OrderStatus::Expired + the + output.expired_orders.len() capacity bump (from main/base).

The refund loop sits outside the if write block by design — it only pushes Unreserve ops into balance_operations, applied later in record_settling_event; balance ops are not write-gated. No leftover conflict markers anywhere in the touched files.

FOK × fills — no fill written for a killed order. A killed FOK takes the MatchResult::Killed path into output.expired_orders and never enters output.fills (book.rs process_pending_orders). Since append only runs in for fill in &output.fills, a killed/expired FOK writes no fill record and no by-user index entry. R8 replay-safety holds: append is Write-gated; replay_under_skip_writes_no_fills asserts both feeds stay empty under Skip.

R4 acceptance — holds. Account-wide newest-first across orders via trades_after (reverse range over by_user, O(length), resolving each entry back through fills); owner-scoped (get_user_trades keys on the caller's UserId, unknown caller → Ok([])); cursor→after pagination; R5/R10 dispositions in the ByAccount arm of get_my_trades (malformed cursor → Err(InvalidCursor), unknown cursor → Ok([]), length clamped to MAX_FILLS_PER_RESPONSE). MemoryIds distinct (7 fills / 8 by_user / 9 seq); init and post_upgrade both wire the three regions. Taker and maker legs share one FillSeq per insert, so a cursor is valid across both ByOrder and ByAccount feeds.

Tests / CI. 431 canister unit tests pass on the merge head; the state-level account_trades_span_a_users_orders_newest_first and the end-to-end should_expose_account_wide_fills_via_get_my_trades_by_account (3 orders, owner-scoping, pagination, unknown/malformed cursor, account↔per-order agreement) both pass. Owner-resolution coverage is real: the owner-scoping assertions would fail if taker_user/maker_user were swapped in append. gh pr checks 180 all green, including candid-backward-compat (the .did is byte-identical to the base — ByAccount already existed in the type).

Maintainability sweep.

• duplication: none found — FillByUserKey/by_user deliberately mirror FillKey/OrderHistory's by_user, no copy-paste blocks ≥10 lines.
• unused derives: none — FillByUserKey derives only what's used (Ord/PartialOrd for the range scan, Copy/Clone/Eq for key+lookup, Debug for panics); correctly omits Hash.
• primitive-obsession params: none — append/trades_after/get_user_trades take domain newtypes (UserId, FillSeq, FillRecord), not bare u64.
• divergent invariant handling: cleared — order_owner_user traps (BUG:) on a missing settled order, get_user_trades returns Ok([]) for an unregistered caller; these are different situations (invariant breach vs. expected-absent input), so the divergence is justified and matches get_user_orders.
• silent fallbacks: none on a failure path — the unwrap_or_default() in the query arm sits behind CursorNotFound/Ok([]) (expected empty), and the index→record resolution traps (BUG:) on an invariant breach rather than swallowing it.

No blockers, no mediums, no nits. Merge is behavior-preserving for PR-3 and CI is green.

VERDICT: READY

[gregorydemay]

🤖 Updated after the stack cascade (merge 7a0ef75): merged the updated #179 down. FillStore::append (4-arg, writing the two fill records + the two by-user index entries) reconciled with the FOK refund path, all Write-gated; killed orders write neither fill nor index entry. Reviewer re-ran: READY. CI 11/11 green, MERGEABLE. Still a draft.

[Copilot]

Pull request overview

Wires the previously-stubbed get_my_trades { ByAccount { after, length } } query arm to the fill store’s account-wide (per-user) trade reader, enabling an owner-scoped, newest-first trade feed across all of a caller’s orders with cursor pagination and request-error handling for malformed cursors.

Changes:

• Implement TradesFilter::ByAccount in get_my_trades, including TradeId cursor parsing, max-length clamping, and TradeId-carrying results.
• Add a state-level reader State::get_user_trades that resolves caller → user id and reads account-wide trades.
• Add unit + integration coverage for ordering, pagination, owner scoping, unknown/malformed cursor behavior, clamping, and unregistered callers.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
integration_tests/tests/tests.rs	Adds PocketIC E2E test validating the ByAccount feed across multiple orders and paging semantics.
canister/src/tests.rs	Adds unit tests and helpers covering ByAccount behavior (ordering, paging, scoping, cursor handling, clamping, unregistered caller).
canister/src/state/tests.rs	Adds state-level test ensuring unregistered callers get empty pages (with/without cursor).
canister/src/state/mod.rs	Introduces State::get_user_trades to read account-wide trades via the trade history’s per-user index.
canister/src/lib.rs	Implements the get_my_trades ByAccount arm: parse TradeId cursor, clamp length, read, and return public trades with ids.
canister/canbench_results.yml	Updates bench numbers reflecting the small instruction-count delta from the new code paths.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.