Skip to content

fix(data): require trades for every tape symbol - #128

Merged
proerror77 merged 4 commits into
mainfrom
codex/binance-verifier-aggtrade
Jul 18, 2026
Merged

proerror77 merged 4 commits into
mainfrom
codex/binance-verifier-aggtrade

Conversation

@proerror77

@proerror77 proerror77 commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Change contract

Reject a Binance market-tape Verified handle unless every symbol declared by the immutable manifest has at least one validated aggregate trade across the sealed segment set.

Out of scope

  • Snapshot/replay seed validation
  • Final book numeric validation
  • Timeline projections or PIT materialization
  • Collector, uploader, deployment, or evaluator changes

Dependency / merge order

Based on current main after merged PR #127. Merge this PR before stacked PR #129 and the typed timeline/materializer PRs.

Focused validation

  • Review budget: 1 file, below the 25-file / 750-line split threshold.
  • Red counterexample: GitHub Linux run 29637715115 failed only at every_declared_symbol_requires_an_aggregate_trade; the log showed an incorrect Ok(VerifiedBinanceMarketTape) with BTC/SOL books but only a BTC trade.
  • Green implementation: compare the validated aggregate-trade symbol set with the manifest symbol set after all sealed segments are processed.
  • Positive boundary: aggregate_trade_coverage_can_span_segments proves coverage is across the sealed set and is not incorrectly required in every segment.
  • Exact-head targeted Linux run 29638230099 passed rustfmt and all 7 verifier tests at ccab9ae0899dae06f2b96e6747fc8ce5ef699b50.
  • Exact-head standard GitHub CI: all applicable checks passed.
  • Static review findings about multi-symbol fixture fidelity and cross-segment positive coverage were addressed; no production-scope finding remains.

Rollout / rollback impact

Verifier-only fail-closed tightening. No collector or production runtime change. Rollback is reverting this PR; artifacts missing per-symbol trades remain rejected while active.

Summary by CodeRabbit

  • Bug Fixes

    • Market tape verification now rejects manifests when any declared symbol lacks aggregate trade coverage.
    • Verification correctly recognizes symbol coverage distributed across multiple signed segments.
  • Tests

    • Added coverage for missing symbol trades.
    • Added multi-symbol and multi-segment validation scenarios.

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8ca68071-296e-4448-a7bb-c60fc0ca5bdc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

verify_binance_market_tape now requires aggregate-trade coverage for every manifest symbol. Test helpers support multiple symbols, and tests cover missing-symbol rejection and coverage distributed across signed segments.

Changes

Market tape validation

Layer / File(s) Summary
Enforce declared-symbol aggregate coverage
rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs
Verification rejects tapes when any manifest-declared symbol lacks an aggregate trade.
Generalize fixtures and validate coverage
rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs
Triplet helpers derive manifest symbol metadata from input rows and test missing and cross-segment aggregate-trade coverage.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the core change: tighter Binance tape verification requiring trades for each declared symbol.
Description check ✅ Passed The description covers the required sections with clear details; only the optional scope-exception note is not explicit.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/binance-verifier-aggtrade

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proerror77
proerror77 marked this pull request as ready for review July 18, 2026 08:59
@proerror77

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@proerror77
proerror77 merged commit f15dc2d into main Jul 18, 2026
17 checks passed
@proerror77
proerror77 deleted the codex/binance-verifier-aggtrade branch July 19, 2026 04:27
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.

1 participant