refactor(ids)!: shared id/seq machinery and composite ids (1/4)#192
Merged
Conversation
Introduce FillSeq and mint a per-order-book fill sequence for each match, persisting the book's fill counter in its snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the standalone OrderBookId newtype with Seq<OrderBookMarker>, so it encodes as a bare u64. Update the worst-case event sizes and the snapshot golden encoding to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the id/seq machinery out of order:: to a crate-level ids module and model OrderId as a CompositeId, replacing the bespoke OrderId newtype and its OrderIdParseError with the shared ParseFixedWithIdError. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add arbitrary strategies for Seq and CompositeId test types and cover the minicbor, fixed-size byte, and hex encode/decode paths with proptests. Introduce the FillId and TradeId composite-id aliases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared identifier layer (ids) for fixed-width sequence IDs and composite IDs, migrates order IDs onto it, and extends order-book matching with a persisted per-book fill sequence (FillSeq) to support future fill/trade persistence.
Changes:
- Add
canister::idswith genericSeq<M>andCompositeId<A, B>plus fixed-width byte + hex conversions and property tests. - Migrate
OrderBookId,OrderSeq, andOrderIdto the sharedidsmachinery (withOrderIdas aCompositeId). - Add
FillSeq/fill_seqtoFill, mint it during matching, and persistnext_fillinOrderBookSnapshot(with related test updates and updated golden encodings / size expectations).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| canister/src/test_fixtures/mod.rs | Updates Fill fixtures to include fill_seq. |
| canister/src/test_fixtures/event.rs | Updates worst-case serialized size expectations after encoding/schema changes. |
| canister/src/state/snapshot/tests.rs | Extends snapshot fixture/schema stability coverage for the new fill sequence state and updates golden hex. |
| canister/src/order/tests.rs | Updates order ID parse error expectations and adjusts fill assertions to include fill_seq. |
| canister/src/order/mod.rs | Migrates OrderBookId/OrderSeq/OrderId to the new ids module abstractions. |
| canister/src/order/fill.rs | Introduces FillSeq and adds fill_seq to Fill; adds FillId/TradeId aliases. |
| canister/src/order/book.rs | Mints FillSeq during matching and persists next_fill in OrderBookSnapshot. |
| canister/src/lib.rs | Wires in ids module and updates order-id parsing/formatting paths. |
| canister/src/ids/tests.rs | Adds proptest coverage for encoding/decoding and hex/byte round-trips for the new id types. |
| canister/src/ids/mod.rs | Implements Seq, CompositeId, fixed-width serialization, and parsing error type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gregorydemay
changed the title
This was referenced Jun 29, 2026
Per-match fill_seq minting shifts instruction counts; regenerate canbench_results.yml via canbench --persist. Also fix "fix number of bytes" -> "fixed number of bytes" in FixedWidthId doc comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gregorydemay
changed the title
|
✅ No security or compliance issues detected. Reviewed everything up to cafb9ce. Security Overview
Detected Code Changes
|
Replace the standalone UserId newtype with Seq<UserIdMarker>, mirroring how OrderSeq and FillSeq are declared. Seq gains a Storable impl producing the same 8-byte big-endian fixed layout the registry map relies on, plus Hash, so all UserId call sites keep working unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mbjorkqvist
approved these changes
Jun 30, 2026
mbjorkqvist
left a comment
mbjorkqvist
left a comment
Contributor
There was a problem hiding this comment.
Thanks @gregorydemay, just a couple of testing-related nits.
On the per-book FillSeq (resolved downstream - no change needed here): This PR makes FillSeq per-order-book (minted in OrderBook::mint_fill_seq, persisted per book via OrderBookSnapshot.next_fill), whereas the DEFI-2901 spec as of this PR still describes a single canister-global fill sequence. I checked how the account-wide feed (R4 ByAccount / R8) actually orders across books, since a per-book sequence can't by itself form a globally-consistent newest-first cursor: #179 introduces a separate canister-global insertion sequence (TradeGlobalSeq, len()-derived) that keys the by_user index, while this per-book FillSeq is used only for the per-order identity (TradeId = (OrderId, FillSeq)) and the per-order prefix scan. #179 also updates the spec to describe both sequences. So the design is sound and the spec/implementation agree once the whole stack is considered — the per-book vs. global split is intentional, just spread across #192 (per-book FillSeq) and #179 (global global_seq + spec edit). Noting it here only as a cross-reference, not as a concern.
Tighten the from_hex guard to canonical lowercase hex only, rejecting uppercase, sign characters and non-ASCII so it strictly inverts write_hex, and add negative parsing tests for Seq and CompositeId. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses Copilot id 3497750038: fold the duplicated length/charset check in Seq::from_hex and CompositeId::from_hex into a single is_canonical_hex helper. Addresses Copilot id 3497750001: document OrderId as a lowercase hex string to match the strict canonical-lowercase parser. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Introduces a reusable id layer and migrates the order types onto it.
idsmodule with a genericSeq<M>sequence type (marker-typed so distinct id families can't be mixed) and aCompositeId<A, B>for opaque hex composite ids.OrderBookId,OrderSeq, andOrderIdonto this machinery, withOrderIdmodeled as aCompositeId.FillSeqand thefill_seqfield onFill, minted during matching and persisted in the order-book snapshot, plus theFillId/TradeIdaliases for upcoming trade persistence.TradeId) shape.Breaking change
BREAKING CHANGE: the persisted stable-memory encoding changes. Order ids (
OrderBookId/OrderSeq) now encode as a bare CBORu64instead of a 1-element array, andOrderBookSnapshotgains a non-Optionnext_fillfield. A canister upgraded with state (event log or order-book snapshot) persisted before this change would fail to decode it. This is acceptable because the canister is pre-launch — there is no deployed state to migrate. The Candid interface is unchanged (oisy_trade.didis byte-identical tomain), so this breaks the state format, not the public API.Performance impact
A pure type/encoding refactor: no new stable-memory writes (the per-fill
fill_seqis an in-memory counter increment), and migratingOrderBookId/OrderSeqto a bare-u64Seqencoding shaves ~1 byte per id in the CBOR event log and snapshot. Net effect is neutral-to-positive — no benchmark regresses by more than 1.55%, while the event-log and upgrade paths get cheaper. Measured with canbench (top-level total instructions).upgrade_1000_no_fillsread_eventsupgrade_full_depthwrite_eventsprocess_pending_orders_1000_no_fillsAll other benchmarks move under ±0.5%, including the full-depth fill sweeps (
gtc/fok_fill_full_bid_side, ~813M instructions) at +0.20%.📚 PR stack
(#171 — order-level scalars — already merged to
main.)🤖 Generated with Claude Code