feat(data): introduce binance.market_tape.v2 tape schema - #544
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesMarket-tape v2
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust_hft/tools/collector/src/lob_archiver.rs (1)
619-702: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
trade_representationcontract-aligned for v2 recovered segments.
market_tape_schemamatchesMARKET_TAPE_SCHEMA_V2, which acceptsraw_traderows.recover_partsonly builds summaries fromagg_traderows, sofinalize_segmentcan emit emptytrade_summarieswith metadata saying aggregate trades are present and the price surface derives from aggregate trades. Make the recovered metadata content/schema-aware, or prevent raw-trade-only v2 recoveries from reaching this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/lob_archiver.rs` around lines 619 - 702, Update finalize_segment’s market-tape metadata construction to reflect the recovered event content: do not advertise aggregate-trade representation, aggregate-trade price derivation, or aggregate trade summaries when the segment contains only raw_trade rows. Use the existing schema/content signals from recover_parts and preserve the current aggregate-trade metadata only when corresponding agg_trade data was actually recovered, or reject raw-trade-only MARKET_TAPE_SCHEMA_V2 segments before finalize_segment.
🧹 Nitpick comments (4)
rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs (1)
649-655: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the
expectonstream_typeswith a fail-closed error.
validate_manifest_identityguarantees that a v2 manifest declaresstream_types, so the invariant holds for sealed triplets today. Theexpectstill converts any future path that skips identity validation into a panic instead of a verification failure.Return an error to keep the verifier fail-closed.
♻️ Proposed change
- let declared_types = segment - .manifest - .stream_types - .as_ref() - .expect("v2 manifest declares stream types"); + let declared_types = segment + .manifest + .stream_types + .as_ref() + .context("v2 market-tape manifest declares no stream types")?;The same pattern exists at Lines 1063-1066 in
validate_stream_coverage_row.As per coding guidelines: "Missing capability must fail closed until a governed Rust implementation exists."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs` around lines 649 - 655, Replace the stream_types expect calls in the v2 branches of the manifest verification logic and validate_stream_coverage_row with fallible error returns. Preserve the existing validation behavior when stream_types is present, but propagate a verification error when it is missing instead of panicking.Source: Coding guidelines
rust_hft/data-pipelines/core/src/binance_market_tape.rs (2)
850-865: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider making
event_time_msoptional for spot frames.For spot frames the code stores the receive clock in
event_time_ms. A consumer cannot distinguish a venue source clock from a receive-derived value. Only the doc comment records the difference.An
Option<u64>field would make the absence explicit and prevent latency math on a receive-derived value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/data-pipelines/core/src/binance_market_tape.rs` around lines 850 - 865, The BookTicker event_time_ms field conflates spot receive time with venue event time. Change event_time_ms to Option<u64>, return None for spot frames lacking a source clock, and preserve the existing source timestamp for USD-M frames; update constructors and consumers to handle the optional value explicitly.
815-848: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
RawTradeSequenceValidatorduplicatesAggregateTradeSequenceValidator.The state shape
HashMap<String, (u64, u64, u64)>and the gap/rollback logic matchAggregateTradeSequenceValidatorat Lines 722-758. Only the id field and the error text differ.A shared generic validator, parameterized by the id accessor and the family name, would remove the duplicate logic. This is optional for this PR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/data-pipelines/core/src/binance_market_tape.rs` around lines 815 - 848, Optionally refactor RawTradeSequenceValidator and AggregateTradeSequenceValidator into one shared generic sequence validator, parameterized by the trade ID accessor and family name. Preserve the existing per-symbol consecutive-ID and source-time rollback checks, overflow handling, and family-specific error text while making both validators reuse the common implementation.rust_hft/tools/collector/src/lob_archiver.rs (1)
1770-1901: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding an explicit v2 case to
recovery_preserves_legacy_and_current_schema_identity/recovery_quarantines_mixed_or_schema_incompatible_parts.These tests exercise
LEGACY_LOB_TAPE_SCHEMAandRAW_SCHEMA(v1) recovery paths, but there is no case usingMARKET_TAPE_SCHEMA_V2with araw_trade,book_ticker, orforce_orderrow. Sincemarket_tape_schemaandevent_type_allowedwere generalized specifically to cover v2, a dedicated recovery test would confirm the new schema branch behaves as expected end to end (manifest fields, quarantine behavior).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/lob_archiver.rs` around lines 1770 - 1901, Add explicit MARKET_TAPE_SCHEMA_V2 recovery cases to recovery_preserves_legacy_and_current_schema_identity and recovery_quarantines_mixed_or_schema_incompatible_parts, using representative raw_trade, book_ticker, or force_order rows. Assert valid v2 parts recover with the expected manifest schema and metadata, and invalid or incompatible v2 rows are quarantined with no manifest produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust_hft/data-pipelines/core/src/binance_market_tape.rs`:
- Around line 875-909: Update from_frame’s market detection to reject frames
that lack the USD-M event identity but contain either source-clock field E or T,
instead of treating them as spot payloads. Add this guard before the spot path
proceeds, while preserving valid spot frames without source clocks and existing
USD-M validation.
---
Outside diff comments:
In `@rust_hft/tools/collector/src/lob_archiver.rs`:
- Around line 619-702: Update finalize_segment’s market-tape metadata
construction to reflect the recovered event content: do not advertise
aggregate-trade representation, aggregate-trade price derivation, or aggregate
trade summaries when the segment contains only raw_trade rows. Use the existing
schema/content signals from recover_parts and preserve the current
aggregate-trade metadata only when corresponding agg_trade data was actually
recovered, or reject raw-trade-only MARKET_TAPE_SCHEMA_V2 segments before
finalize_segment.
---
Nitpick comments:
In `@rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs`:
- Around line 649-655: Replace the stream_types expect calls in the v2 branches
of the manifest verification logic and validate_stream_coverage_row with
fallible error returns. Preserve the existing validation behavior when
stream_types is present, but propagate a verification error when it is missing
instead of panicking.
In `@rust_hft/data-pipelines/core/src/binance_market_tape.rs`:
- Around line 850-865: The BookTicker event_time_ms field conflates spot receive
time with venue event time. Change event_time_ms to Option<u64>, return None for
spot frames lacking a source clock, and preserve the existing source timestamp
for USD-M frames; update constructors and consumers to handle the optional value
explicitly.
- Around line 815-848: Optionally refactor RawTradeSequenceValidator and
AggregateTradeSequenceValidator into one shared generic sequence validator,
parameterized by the trade ID accessor and family name. Preserve the existing
per-symbol consecutive-ID and source-time rollback checks, overflow handling,
and family-specific error text while making both validators reuse the common
implementation.
In `@rust_hft/tools/collector/src/lob_archiver.rs`:
- Around line 1770-1901: Add explicit MARKET_TAPE_SCHEMA_V2 recovery cases to
recovery_preserves_legacy_and_current_schema_identity and
recovery_quarantines_mixed_or_schema_incompatible_parts, using representative
raw_trade, book_ticker, or force_order rows. Assert valid v2 parts recover with
the expected manifest schema and metadata, and invalid or incompatible v2 rows
are quarantined with no manifest produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f6905a7-179d-460f-979f-98686fdcb338
📒 Files selected for processing (4)
rust_hft/data-pipelines/core/src/binance_lob_replay.rsrust_hft/data-pipelines/core/src/binance_market_tape.rsrust_hft/data-pipelines/core/src/binance_market_tape_artifact.rsrust_hft/tools/collector/src/lob_archiver.rs
Add the v2 market-tape schema with three new validated event families: raw_trade (per-symbol trade-id continuity), book_ticker and force_order (clock-bounds only, no sequence guarantee; force_order is USD-M only). v2 declares its per-symbol stream-type list in the manifest and the session_start row; websocket_streams and stream_coverage evidence are checked against that declaration instead of the fixed v1 two-stream model. kline is reserved as an event type name only. v1 tapes keep byte-identical verification: v1 manifests must not carry stream_types, v1 rows reject the new event types, and the legacy stream count rule is unchanged. A BinanceRawTradeContinuityVerifier parallels the aggregate-trade cross-segment verifier, and the collector archiver finalization now matches the market-tape schema version set. Refs #535
Binance spot bookTicker payloads carry no e/E/T fields (those are USD-M only), so the strict identity and source-clock checks rejected every real spot frame. Decide the market from the frame shape: USD-M frames keep the strict e/E/T validation, spot frames derive the symbol from s or the combined-stream name and fall back to the receive clock. Both shapes still fail closed on malformed payloads. Refs #535 Refs #536
1adcaad to
3b3328e
Compare
1 participant
Change contract
Introduce
binance.market_tape.v2indata-pipelines/core: new event familiesraw_trade(per-symbol trade-id continuity),book_ticker(spot + fstream frame shapes),force_order(USD-M only), version-aware structural checks (v1 = 2 streams/symbol, v2 = declaredstream_typeslist), explicit continuity-summary arms, and byte-identical v1 verification.klinereserved as name only.Out of scope
Dependency / merge order
Merge after nothing; base of the stack
tape-v2-schema ← archiver-v2-streams ← tape-v2-deploy-gates. Old binaries remain fail-closed on v2 tapes (verified); new code accepts v1 (regression suite unchanged and green).Focused validation
cargo test -p hft-collector --lockedgreen; clippy cleane/E/T) now parse with receive-clock-only bounds; sealed v2 spot tape verifies end-to-endRollout / rollback impact
None until #536 lands — the archiver still writes v1. Verifier-only change; rollback = revert.
Issue relationship
Closes #535