feat(data): expose verified Binance replay timeline - #131
Conversation
📝 WalkthroughWalkthroughReplay verification now records ordered per-symbol replay and checkpoint events, rejects backward receive-time movement across segments, and exposes receive-time bounds, event counts, and typed event lists through the verified tape surfaces. ChangesReplay event sequencing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant verify_binance_market_tape
participant ReplaySequenceValidator
participant ReplayedBinanceBook
verify_binance_market_tape->>ReplaySequenceValidator: Observe diff and checkpoint rows
ReplaySequenceValidator-->>verify_binance_market_tape: Return replay events
verify_binance_market_tape->>ReplayedBinanceBook: Store ordered replay and checkpoint events
verify_binance_market_tape->>verify_binance_market_tape: Reject backward received_at_ns
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d0c1ecdf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if last_received_at_ns.is_some_and(|last| received_at_ns < last) { | ||
| bail!("market-tape receive time moved backwards"); | ||
| } | ||
| last_received_at_ns = Some(received_at_ns); |
There was a problem hiding this comment.
When a later segment manifest declares start_received_at_ns earlier than the previous segment's declared end but its first row is still >= the last row seen, this row-level check passes and identity() now exposes overlapping segment bounds as verified. That lets downstream callers of segments() see a timeline that moves backward even though verification succeeded; compare declared segment bounds across the ordered set or derive the exposed bounds from the observed rows before returning them.
Useful? React with 👍 / 👎.
| if last_received_at_ns.is_some_and(|last| received_at_ns < last) { | ||
| bail!("market-tape receive time moved backwards"); | ||
| } | ||
| last_received_at_ns = Some(received_at_ns); |
There was a problem hiding this comment.
This check runs across every archived row, but the newly exposed timeline is per ReplayedBinanceBook and excludes agg_trade/session_start rows. In the inspected collector, depth and aggregate-trade messages are produced by separate websocket tasks, timestamped before enqueueing, and then written in receiver order, so a valid agg_trade or another symbol's row with a larger receive time can be written before this book's next diff with a smaller receive time even though the book's own replay remains ordered; track the last receive time per replayed symbol or only validate the emitted book events.
Useful? React with 👍 / 👎.
| replayed | ||
| .get_mut(symbol) | ||
| .context("market-tape replay event symbol is undeclared")? | ||
| .extend(events.into_iter().map(ReplayedBinanceBookEvent::Replay)); |
There was a problem hiding this comment.
When an earlier snapshot or diff contains a non-positive price/quantity that is later corrected before the final checkpoint, the existing final-book check can still pass, but this new events() surface now returns that invalid level as verified replay data. Validate the levels in each ReplaySequenceEvent before extending the public timeline, or reject the artifact before returning VerifiedBinanceMarketTape.
Useful? React with 👍 / 👎.
Change contract
Expose a chronological, read-only replay timeline from
VerifiedBinanceMarketTapeand fail closed whenreceived_at_nsmoves backwards across the ordered sealed segment set.Out of scope
Dependency / merge order
None. Depends only on verifier contracts already merged in #128, #129, and #130. The PIT materializer adapter will be a separate dependent PR after this one merges.
Focused validation
receive_time_cannot_move_backwards_across_segmentsfailed because the old verifier returned a Verified handle after a cross-segment receive-time rollback.7d0c1ecdf1680c4a93779781c56f2008ed915da1.Rollout / rollback impact
Data-contract-only, fail-closed tightening. No runtime service change. Reverting this PR removes the timeline projection and returns to accepting cross-segment receive-time rollback; dependent PIT materialization must not roll out without it.
Summary by CodeRabbit
New Features
Bug Fixes