Skip to content

docs(specs): persist individual fills and expose per-order / per-account trade feeds#170

Draft
gregorydemay wants to merge 5 commits into
mainfrom
dex_DEFI-2901_persist-individual-fills-and-expose-a-per-order
Draft

docs(specs): persist individual fills and expose per-order / per-account trade feeds#170
gregorydemay wants to merge 5 commits into
mainfrom
dex_DEFI-2901_persist-individual-fills-and-expose-a-per-order

Conversation

@gregorydemay

@gregorydemay gregorydemay commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Adds the DEFI-2901 spec for persisting individual fills and exposing them through a query endpoint.

The spec separates two distinct deliverables that should not be conflated:

  • Order-level scalars on OrderRecord — cumulative realized filled_quote (always quote-denominated) and filled_fee (denominated in the order's receive token), folded into the existing single per-order write. VWAP is derived client-side rather than stored, since an integer price cannot represent a fractional average exactly.
  • Per-fill records in their own stable-memory regions — a side-projected, counterparty-free feed behind one new owner-scoped endpoint, get_my_trades, filterable either by order (ByOrder) or account-wide (ByAccount).

Key decisions captured in the spec: store realized amounts and never fee rates (robust to future rate changes); denormalized side-projected records over a normalized canonical-plus-pointers layout (fewer settlement hot-path inserts, no read indirection); compute per-fill values in settlement where the base-token scale is in scope; and a canbench measurement of the settlement path as an acceptance gate on the persistence PR. Includes a worked ICP/ckUSDT example and a comparison of the proposed surface against Binance, Coinbase Advanced, and Kraken.

Delivered as three stacked PRs: order-level scalars, fill store + per-order filter, then the account-wide filter.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 22, 2026 20:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new development spec (DEFI-2901) documenting the planned design for persisting individual fills in stable memory, extending OrderRecord with cumulative execution scalars, and exposing owner-scoped per-order and per-account trade feeds.

Changes:

  • Introduces the DEFI-2901 spec with requirements (R1–R12), non-goals, and design decisions for order-level scalars plus per-fill persistence.
  • Details the proposed storage layout (stable-memory regions + indices), new query endpoints (get_order_fills, get_my_trades), and cursor/pagination behavior.
  • Defines a test/benchmark plan including a canbench-based hot-path cost acceptance gate.

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

Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown

canbench 🏋 (dir: canister) e77369e 2026-06-23 07:14:55 UTC

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

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

Summary:
  instructions:
    status:   No significant changes 👍
    counts:   [total 12 | regressed 0 | improved 0 | new 0 | unchanged 12]
    change:   [max +794.90K | p75 +63.93K | median +154 | p25 -220.35K | min -1.82M]
    change %: [max +0.47% | p75 +0.15% | median +0.03% | p25 -0.11% | min -1.02%]

  heap_increase:
    status:   No significant changes 👍
    counts:   [total 12 | regressed 0 | improved 0 | new 0 | unchanged 12]
    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 12 | regressed 0 | improved 0 | new 0 | unchanged 12]
    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%]

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

Only significant changes:
| status | name                               | calls |    ins |  ins Δ% | HI |  HI Δ% | SMI |  SMI Δ% |
|--------|------------------------------------|-------|--------|---------|----|--------|-----|---------|
|   -    | bench_write_events::AddTradingPair |     1 | 16.95K |  -2.04% |  0 |  0.00% |   0 |   0.00% |
|   -    | bench_write_events::Deposit        |     1 | 10.30K |  -2.15% |  0 |  0.00% |   0 |   0.00% |
|   -    | bench_write_events::Init           |     1 | 23.13K |  -2.71% |  0 |  0.00% |   0 |   0.00% |

ins = instructions, HI = heap_increase, SMI = stable_memory_increase, Δ% = percent change

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

Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
Comment thread docs/src/development/specs/DEFI-2901-persist-fills.md Outdated
gregorydemay and others added 2 commits June 23, 2026 05:48
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 23, 2026 07:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment on lines +96 to +98
- **R10 — Bounded pages.** The feed caps `length` at `MAX_FILLS_PER_RESPONSE`; an absent /
larger value is clamped. An unknown `after` cursor yields an empty page (malformed cursors are
rejected per R5).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Fixed in b3fc8f1. You're right — length is mandatory (nat32) in the filter records, so there is no "absent" case. R10 now reads: "length is mandatory in the filter; a value above MAX_FILLS_PER_RESPONSE is clamped down to it." (Only after is optional.)

Comment on lines +39 to +40
2. **Per-fill records in their own stable-memory regions** — the granular feed, behind one new
`get_my_trades` endpoint that filters either by order or account-wide.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Good catch — the PR description was stale. Updated it to describe the single get_my_trades endpoint with the ByOrder | ByAccount filter (the spec was collapsed to one endpoint in an earlier revision); there is no longer a get_order_fills.

micro-benchmark, with vs. without fill persistence, to characterize the per-fill insert cost
against the timer chunk's instruction budget.

## Worked example

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format the worked example as a table

  1. 1 row per fill
  2. 1 column per side (maker/taker)

in each cell bullet list, one bullet per parameter.
Change the examples to have 2 fills, use a bit more realistic example (ICP/ckUSDT, use 8/6 decimals, can use price of 10 ckUSDT per ICP to keep it easy)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Reformatted in 0b2aaa8. The worked example is now a table — one row per fill, a Taker-leg and a Maker-leg column, each cell a bullet list of the Trade parameters. Switched to ICP/ckUSDT (8/6 decimals, prices in ckUSDT/ICP) and to two fills: a taker buy of 5 ICP @ limit 12 sweeping Maker A (2 ICP @ 10) and Maker B (3 ICP @ 11). Added a setup note showing the smallest-unit scaling (price = ckUSDT/ICP·10⁶, quantity = ICP·10⁸, notional = price·quantity/10⁸) worked once for Fill 1, plus order-level rollups (taker: filled_quote 53, VWAP 10.6, fee 0.005 ICP, 7 ckUSDT released; makers: 0.01 / 0.0165 ckUSDT fee). Updated the state/tests.rs plan to assert these numbers.

gregorydemay and others added 2 commits June 23, 2026 07:31
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 23, 2026 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants