feat(polymarket): publish event candidate evidence - #338
Conversation
|
Warning Review limit reached
Next review available in: 52 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)
📝 WalkthroughWalkthroughThe collector adds candidate Polymarket evidence normalization and immutable triplet publication for non-ready events, with candidate-specific validation, manifests, qualifications, surface counts, and replay/no-clobber tests. ChangesPolymarket candidate evidence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Collector
participant CandidateNormalizer
participant CandidatePublisher
participant Qualification
Collector->>CandidateNormalizer: normalize candidate event
CandidateNormalizer->>CandidatePublisher: provide candidate data and manifest inputs
CandidatePublisher->>Qualification: bind candidate digests and verifier contract
Qualification-->>Collector: return candidate artifact and qualification
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: edca5290e7
ℹ️ 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".
| } | ||
| let mut event_config = config.evidence.clone(); | ||
| event_config.market_ids = vec![market_id.clone()]; | ||
| match normalize_candidate(&event_config).and_then(|evidence| { |
There was a problem hiding this comment.
When a source-closed producer declares an event Ready but the actual tape is missing a surface, execution bypasses this candidate branch, complete normalization fails, and the later deterministic-error arm publishes only a rejected qualification. Thus an untrusted readiness claim can suppress the sparse candidate triplet containing the evidence rows that do exist; run candidate normalization after deterministic complete-verification failures as well.
Useful? React with 👍 / 👎.
| .filter(|_| contracts.len() == 1) | ||
| .ok_or_else(|| anyhow!("candidate evidence requires exactly one market"))?; | ||
| let (trades, settlements) = reference_records(reference_path, contracts)?; | ||
| let completions = aggregated_trade_completions(inputs)?; |
There was a problem hiding this comment.
When two accepted reference segments contain a completion proof for the same sibling market, this call fails before producing the selected event's candidate because aggregated_trade_completions aggregates every market and rejects every duplicate. Same-hour/sparse multi-segment inputs are otherwise admitted and sibling trades and settlements are explicitly deduplicated, so an unrelated repeated proof can suppress a valid event-local candidate; filter completion identities to the selected market before checking duplicates.
Useful? React with 👍 / 👎.
| validate_candidate_dataset(&evidence)?; | ||
| ensure_canonical_directory(output_root)?; | ||
| let digest = &evidence.report.content_sha256; | ||
| let directory = output_root.join(format!("sha256={digest}")); |
There was a problem hiding this comment.
When the same event rows are normalized from a different valid source triplet or reference set containing only changed sibling data, the NDJSON digest remains unchanged while the manifest changes because it embeds validated_inputs. Since the directory and manifest filename here are keyed only by the NDJSON digest, the second publication encounters the first manifest with different bytes and fails no-clobber instead of publishing the otherwise valid candidate; incorporate the manifest/provenance digest into the immutable identity or make the manifest a function of the content bytes alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust_hft/tools/collector/src/polymarket_research_select.rs (1)
508-538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo positional
boolcontrol inputs are swap-prone.event_localandrequire_quote_coverageare both plain booleans passed positionally at three call sites. They also overlap: whenrequire_quote_coverage == true,event_localis ignored entirely. A future caller (or an accidental argument swap, e.g. candidate's(true, false)becoming(false, true)) would silently switch the segment-validation pipeline and quote-coverage enforcement without a compile error. Consider collapsing these into a single explicit policy enum (e.g.Strict/EventLocal/Candidate) so each flow selects exactly one variant.🤖 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/polymarket_research_select.rs` around lines 508 - 538, The with_selected_research_contracts_policy API uses two overlapping positional booleans that can be swapped without compiler errors. Replace event_local and require_quote_coverage with a single explicit policy enum representing the strict, event-local, and candidate flows, then update the three call sites and the dispatch logic in with_selected_research_contracts_policy so each variant selects exactly one validation pipeline and preserves quote-coverage enforcement for the strict path.
🤖 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/tools/collector/src/polymarket_research_select.rs`:
- Around line 508-538: The with_selected_research_contracts_policy API uses two
overlapping positional booleans that can be swapped without compiler errors.
Replace event_local and require_quote_coverage with a single explicit policy
enum representing the strict, event-local, and candidate flows, then update the
three call sites and the dispatch logic in
with_selected_research_contracts_policy so each variant selects exactly one
validation pipeline and preserves quote-coverage enforcement for the strict
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d3afbaa-4da9-454e-8487-8256a5b30315
📒 Files selected for processing (4)
rust_hft/tools/collector/src/polymarket_evidence_artifact.rsrust_hft/tools/collector/src/polymarket_research_import.rsrust_hft/tools/collector/src/polymarket_research_normalize.rsrust_hft/tools/collector/src/polymarket_research_select.rs
edca529 to
e35428c
Compare
Change contract
For each source-closed explicit Polymarket event, publish one event-local, content-addressed, no-clobber candidate triplet containing only the evidence rows that actually exist, while keeping the complete evidence path strict and treating every candidate as an untrusted producer carrier that requires independent verification.
Closes #336.
Out of scope
Independent Ready/Partial/Rejected verification, catalog receipts and queries, OSS acquisition/downloading, cohort or snapshot construction, evaluator/MCTS behavior, deployment, and collector cutover.
Dependency / merge order
Depends on #318 / PR #330 (merged). Merge this PR before #337, then #319.
Focused validation
cargo test -p hft-collector --locked— pass (216 lib tests passed, 2 ignored; all binary/integration groups passed)cargo test -p hft-collector --locked candidate_normalization_preserves_zero_missing_surfaces_without_rows— pass through the real segment-validation and public candidate-normalization seamcargo test -p hft-collector --locked event_local_trade_union_allows_sparse_reference_hours— pass; complete trust boundary remains strictcargo check -p hft-collector --tests --locked --target x86_64-unknown-linux-gnu— pass; Linux-only no-clobber/publication counterexamples compilerustfmt --checkon all four touched Rust files — passgit diff --check— passedca5290e787e6c45cdd0bcab38e53a167d64425Safety counterexamples
Partial + independent_verification_required.Unchanged; conflicting bytes at the immutable path fail closed.Rollout / rollback impact
No runtime rollout in this PR. Rollback removes candidate publication and restores qualification-only handling for non-Ready events; the existing complete v2/v3 evidence path is unchanged.
Summary by CodeRabbit
New Features
Bug Fixes