feat(research): define evidence-bound CEX snapshot V2 - #781
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (1)
📝 WalkthroughWalkthroughThe manifest package adds CEX replay schema V2 types, evidence metadata, validation, and digest-bound dataset manifests. V1 types remain available for historical decoding. Worktree metadata and dependencies now target the research manifest package. ChangesCEX replay manifest contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 033d21ca8a
ℹ️ 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".
…ntract-v2 # Conflicts: # agent-worktree.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5f335e918
ℹ️ 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".
|
@codex review exact head c06145724a9d8d5b4e409e5e565824daf9510aa8 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e85ad31634
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (5)
rust_hft/research-core/manifest/src/lib.rs (5)
559-561: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMake
equal_decimalsfail closed on unparseable input.
left.parse().ok() == right.parse().ok()returnstruewhen both strings fail to parse, becauseNone == None. The only caller at line 286 is protected, becauseordered_nonnegative_decimalsat line 279 already rejects unparseable values and the||chain returns first. The helper is therefore safe today. Make the helper self-contained so a future caller cannot inherit the trap.♻️ Proposed fix
fn equal_decimals(left: &str, right: &str) -> bool { - left.parse::<Decimal>().ok() == right.parse::<Decimal>().ok() + matches!( + (left.parse::<Decimal>(), right.parse::<Decimal>()), + (Ok(left), Ok(right)) if left == right + ) }🤖 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/research-core/manifest/src/lib.rs` around lines 559 - 561, Update equal_decimals to return true only when both inputs parse successfully and their Decimal values are equal; ensure any unparseable input returns false instead of allowing None == None.
326-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a parameter struct for
validate_snapshot_core.The function takes 15 positional parameters, and six of them are
&str. A caller can transposevenue,instrument_type,symbol, orreplay_clockwithout a compile error. Both current call sites are correct, so this is a future-proofing suggestion. Group the shared fields into aSnapshotCoreView<'_>borrow struct and pass one argument. The#[allow(clippy::too_many_arguments)]attribute then becomes unnecessary.🤖 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/research-core/manifest/src/lib.rs` around lines 326 - 344, Introduce a borrowed `SnapshotCoreView<'_>` struct containing the shared validation fields currently passed to `validate_snapshot_core`, update both call sites to construct and pass that view, and change `validate_snapshot_core` to accept it alongside the remaining distinct arguments. Remove the now-unnecessary `#[allow(clippy::too_many_arguments)]` attribute while preserving validation behavior and field mappings.
1026-1036: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tests for the
spotderivatives branch.The fixture is
usdmonly, so the match arms at lines 256-269 are partly unexercised. Three cases have no test:
spotwithderivatives_reference: Nonemust validate. This is the accepting arm at line 267.spotwithderivatives_reference: Some(..)must be rejected.usdmwithderivatives_reference: Nonemust be rejected.The same gap applies to the required-modality set at lines 203-210, because no test proves that a
spotsnapshot must omitfundingandopen_interest. The instrument-dependent behaviour is the central new rule in this contract, so it needs direct coverage.Do you want me to generate these tests?
🤖 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/research-core/manifest/src/lib.rs` around lines 1026 - 1036, Add direct validation tests for the instrument-dependent derivatives rules in the existing manifest test module: cover spot snapshots with derivatives_reference None as valid, spot with Some(..) as invalid, and usdm with None as invalid. Also assert the required-modality behavior for spot by verifying it rejects snapshots containing funding or open_interest, reusing the existing snapshot fixtures and ManifestError expectations.
114-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDocument the canonical decimal string requirement.
The decimal fields are
Stringand enter the snapshot digest verbatim.equal_decimalstreats"0.10"and"0.1"as equal, butsha256()produces two differentmanifest_idvalues for them. Writers must therefore emit a canonical decimal form to keep dataset identity stable. Add a doc comment on these fields that states the canonicalization requirement.Also applies to: 153-153, 168-170
🤖 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/research-core/manifest/src/lib.rs` around lines 114 - 123, Add a Rust doc comment to the decimal string fields in CexInstrumentRulesV2 and the corresponding fields at the referenced locations, stating that writers must emit canonical decimal representations because values are included verbatim in snapshot digests. Ensure the guidance covers tick_size, step_size, min_notional, and any matching decimal fields without changing their types or behavior.
237-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the fused validation conditions to report the failing field.
Lines 237-255 fuse 14 predicates into one error,
"PIT rules or fee evidence is invalid". Lines 270-292 fuse 11 predicates into"measured latency cost evidence is invalid". A rejected snapshot therefore does not identify which bound failed. For a fail-closed contract, an operator must inspect the code to diagnose a rejection.Extract per-group helpers that return distinct error strings, for example
"instrument rules are invalid","fee account binding is invalid","fee validity window is too short", and"latency percentiles are nonmonotonic". The existing tests already assert on the coarse strings, so update them together with the split.🤖 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/research-core/manifest/src/lib.rs` around lines 237 - 292, Split the fused validation in the snapshot validation method into focused per-group checks or helpers that return field-specific error messages, covering instrument rules, fee account binding and validity bounds, derivatives reference, and latency evidence (including percentile monotonicity). Preserve fail-closed behavior and update existing tests to assert the new distinct error strings instead of the coarse messages.
🤖 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.
Nitpick comments:
In `@rust_hft/research-core/manifest/src/lib.rs`:
- Around line 559-561: Update equal_decimals to return true only when both
inputs parse successfully and their Decimal values are equal; ensure any
unparseable input returns false instead of allowing None == None.
- Around line 326-344: Introduce a borrowed `SnapshotCoreView<'_>` struct
containing the shared validation fields currently passed to
`validate_snapshot_core`, update both call sites to construct and pass that
view, and change `validate_snapshot_core` to accept it alongside the remaining
distinct arguments. Remove the now-unnecessary
`#[allow(clippy::too_many_arguments)]` attribute while preserving validation
behavior and field mappings.
- Around line 1026-1036: Add direct validation tests for the
instrument-dependent derivatives rules in the existing manifest test module:
cover spot snapshots with derivatives_reference None as valid, spot with
Some(..) as invalid, and usdm with None as invalid. Also assert the
required-modality behavior for spot by verifying it rejects snapshots containing
funding or open_interest, reusing the existing snapshot fixtures and
ManifestError expectations.
- Around line 114-123: Add a Rust doc comment to the decimal string fields in
CexInstrumentRulesV2 and the corresponding fields at the referenced locations,
stating that writers must emit canonical decimal representations because values
are included verbatim in snapshot digests. Ensure the guidance covers tick_size,
step_size, min_notional, and any matching decimal fields without changing their
types or behavior.
- Around line 237-292: Split the fused validation in the snapshot validation
method into focused per-group checks or helpers that return field-specific error
messages, covering instrument rules, fee account binding and validity bounds,
derivatives reference, and latency evidence (including percentile monotonicity).
Preserve fail-closed behavior and update existing tests to assert the new
distinct error strings instead of the coarse messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0227dcde-9343-4d1a-ba65-044a9d2811ce
⛔ Files ignored due to path filters (1)
rust_hft/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
agent-worktree.ymlrust_hft/research-core/manifest/Cargo.tomlrust_hft/research-core/manifest/src/lib.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6f5600d2
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4421bd21b
ℹ️ 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".
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct CexFeeScheduleV2 { | ||
| pub account_fingerprint: String, |
There was a problem hiding this comment.
When one account has fee artifacts for multiple symbols or both spot and USDM, this contract accepts the wrong artifact because CexFeeScheduleV2 retains only the account fingerprint and validation never compares the other identities. The canonical artifact records venue, market, and symbol in tools/collector/src/binance_fee_artifact.rs:27-32; fresh evidence at exact head is that latency evidence now binds all three identities while fee evidence remains unscoped. Preserve these identities in the fee schedule and require them to match the snapshot before admission.
AGENTS.md reference: AGENTS.md:L67-L68
Useful? React with 👍 / 👎.
| if first_event_ns < source_segments[0].start_received_at_ns | ||
| || last_event_ns > final_segment_end_ns |
There was a problem hiding this comment.
When two authenticated segments cover [0,1] and [10,20], a V2 snapshot can claim first_event_time=5 and last_event_time=6 and still pass because these checks compare only against the outermost start and end. Both purported feature events are then inside an unauthenticated gap, so the snapshot fabricates source coverage; require each event boundary to be contained in an actual segment rather than merely inside the aggregate envelope.
AGENTS.md reference: AGENTS.md:L65-L66
Useful? React with 👍 / 👎.
| && evidence | ||
| .iter() | ||
| .enumerate() | ||
| .all(|(index, item)| !evidence[..index].contains(item)) |
There was a problem hiding this comment.
When a funding or open-interest snapshot spans a normal long research window, observations == evidence.len() forces thousands of triplets, while scanning every preceding prefix makes validation quadratic. For example, covering 30 days under the 90-second maximum gap requires about 28,800 entries and roughly 415 million equality checks per series; insert identities into a BTreeSet while iterating so duplicate rejection remains practical.
Useful? React with 👍 / 👎.
| self.snapshot.validate()?; | ||
| if self.dataset_kind != CEX_REPLAY_DATASET_KIND | ||
| || self.schema_version != CEX_REPLAY_DATASET_SCHEMA_V2 | ||
| || self.feature_manifest_id.trim().is_empty() |
There was a problem hiding this comment.
When feature_manifest_id is mistyped or points to another feature manifest, V2 accepts it as long as it is nonblank even though the canonical feature writer derives the ID as dataset-{artifact_sha256} in tools/collector/src/feature_matrix.rs:135-137. Two otherwise identical admissions can therefore carry different feature lineage while receiving the same snapshot-derived manifest_id, causing an immutable registry identity collision; require this field to equal the ID derived from snapshot.feature_artifact_sha256.
AGENTS.md reference: AGENTS.md:L67-L68
Useful? React with 👍 / 👎.
| worktree: /Users/proerror/Documents/monday/.worktrees/codex/binance-fee-snapshot-v2 | ||
| branch: codex/binance-fee-snapshot-v2 | ||
| base_sha: 32e01918a88355f0708bbacd9ae6d4d5eb32aa13 | ||
| contract: cex-research-contract-v2 |
There was a problem hiding this comment.
This change contains 772 non-generated changed lines (682 additions and 90 deletions), exceeding the repository's 750-line split threshold, while the commit declares no atomic exception or named reviewer approval. Split the contract into independently testable rollout units or record the required approved exception before merging.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
Change contract
Define the CEX ResearchSnapshot V2 contract with independently timed funding/OI evidence, authenticated data/manifest/_SUCCESS digests, point-in-time rule and fee validity, and verified order-lifecycle latency cost. Retain V1 only for immutable read-only decoding and reserve a new outer materialization schema.
Issue relationship
Refs #773
Out of scope
Materialization, evaluation consumption, production jobs, and live trading activation are separate rollout units.
Dependencies and merge order
None
Focused validation
cargo test -p hft-research-manifest --locked(10 passed)Rollout and rollback
Contract-only; no runtime mutation. Revert this PR before adding V2 producers if review rejects the schema.
Scope exception
None
Summary by CodeRabbit
New Features
Bug Fixes