feat(collector): qualify published Polymarket event evidence - #330
Conversation
📝 WalkthroughWalkthroughPolymarket evidence publication now requires producer qualification input, classifies each selected market, publishes immutable qualification records, verifies ready events locally, and returns batched evidence and qualification results. The CLI reads qualification data from a required JSON file. ChangesPolymarket qualification publication
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI as polymarket-raw-ops
participant Publisher as publish_polymarket_evidence
participant Verifier as verified_event_input
participant Storage as immutable publication
CLI->>Publisher: qualification JSON and selected market IDs
Publisher->>Verifier: normalized event evidence
Verifier-->>Publisher: verified or rejected result
Publisher->>Storage: publish evidence triplet
Publisher->>Storage: publish qualification record
Storage-->>Publisher: immutable creation or existing payload
🚥 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 |
fadc9d7 to
73b2ceb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fadc9d778d
ℹ️ 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 !["BTCUSDT", "SOLUSDT"].contains(&input.symbol.as_str()) | ||
| || (event_end - event_start).num_seconds() != 300 |
There was a problem hiding this comment.
A complete SOL event passes this check and can be published as Ready, but the governing contract explicitly limits the continuously-ready path to BTC five-minute episodes and states that SOL remains outside it until a separate verified contract exists (docs/architecture/PREDICTION_MARKETS.md:64-69,169-174). This admits an unsupported product family into downstream research qualification instead of returning UnsupportedProductContract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to #319. This #318 record carries the producer-declared product identity for event-local evidence qualification and is not the authoritative supported-product admission catalog. The current verifier still requires normalized evidence to match that declaration; #319 will own the BTC/SOL support policy and promotion authority.
| pub state: PolymarketEventQualificationState, | ||
| pub reasons: Vec<PolymarketEventQualificationReason>, | ||
| pub retry: bool, |
There was a problem hiding this comment.
The record exposes a single unscoped Ready state but contains no supported-task or coverage identity, so a downstream cohort cannot distinguish settlement qualification from up_execution or down_execution qualification. Those tasks have different evidence requirements, and the repository contract requires readiness to apply only to verified supported tasks and the verifier receipt to bind the exact task and coverage (docs/architecture/PREDICTION_MARKETS.md:89-93,103-120); without that binding, this receipt can incorrectly admit evidence to an evaluator it was never verified for.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| verified.symbol = contract["symbol"] | ||
| .as_str() | ||
| .context("verified contract symbol is missing")? | ||
| .to_owned(); | ||
| verified.event_start = contract["event_start"] |
There was a problem hiding this comment.
When the normalized contract has the same market_id and token pair but a different symbol or event window, these assignments silently replace the producer declaration rather than rejecting the identity mismatch. If the producer clocks are broad enough to encompass both windows, its request outcomes and sequence claims are then attached to the verifier's different episode and the result can still become Ready; compare the declared symbol/start/end to the normalized contract before carrying producer-local evidence into the receipt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in final head 6514421. verified_event_input now rejects a declared market_id, symbol, event_start, or event_end mismatch instead of overwriting declared identity. The Linux-gated integration test includes four independent mutators and asserts each mismatch fails.
| #[derive(Debug, Clone, PartialEq, Eq, Serialize)] | ||
| pub struct PolymarketEventQualificationRecord { | ||
| pub schema: &'static str, | ||
| pub verifier_contract: &'static str, |
There was a problem hiding this comment.
The receipt records only a static schema name for the verifier while all concrete source, image, and configuration hashes belong to the producer. When the verifier binary, configuration, or policy changes, downstream readers therefore cannot tell which verifier authorized a Ready classification or reproduce that decision. The governing contract explicitly requires the verifier to record its own binary, configuration, and policy identities (docs/architecture/PREDICTION_MARKETS.md:82-87,103-106), so these identities need to be bound into every qualification receipt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| if input.sequence.gaps != 0 || input.sequence.end < input.sequence.start { | ||
| reasons.push(PolymarketEventQualificationReason::SequenceGap); | ||
| permanent_failure = true; |
There was a problem hiding this comment.
This only checks that the producer-supplied sequence range is internally ordered and claims zero gaps; it is never reconciled with the sequences in the validated input manifests or normalized event rows. A producer can declare start=1,end=7,gaps=0 for evidence whose authenticated source range is entirely different and still receive Ready, leaving the immutable receipt with false sequence-completion evidence even though sequence proof is a required readiness condition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| let path = directory_path.join(format!( | ||
| "polymarket-event-qualification.{}.{}.json", | ||
| record.market_id, identity | ||
| )); |
There was a problem hiding this comment.
The qualification filename is determined only by market_id and evidence content digest, so two legitimate receipts for byte-identical evidence but different producer revisions, provenance, clocks, or verifier policy resolve to the same path. Because install_no_clobber rejects different bytes at an existing path, independently verifying a new producer revision then fails instead of appending a second immutable receipt as required by the revision model (docs/architecture/PREDICTION_MARKETS.md:92-93); include a digest of the full receipt or its producer/verifier identities in the object key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| if declared_record.state != PolymarketEventQualificationState::Ready { | ||
| qualifications.push(publish_qualification_record( | ||
| &config.output_root, | ||
| declared_record, | ||
| )?); |
There was a problem hiding this comment.
Every producer-declared non-Ready event takes this branch without invoking normalization or any staged-artifact verifier. A qualification file can therefore name a nonexistent market, supply syntactically valid hashes and clocks, mark one surface incomplete, and obtain an immutable Partial receipt even when no authentic evidence for that episode exists. The state contract defines partial as authentic but incomplete evidence (docs/architecture/PREDICTION_MARKETS.md:89-93), so the event identity and available producer artifacts must still be authenticated before this classification is published.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| DateTime::parse_from_rfc3339(&outcome.completed_at) | ||
| .map(|completed_at| { | ||
| let completed_at = completed_at.with_timezone(&Utc); | ||
| completed_at < opened_at || completed_at > closed_at | ||
| }) |
There was a problem hiding this comment.
Request timestamps are accepted whenever they fall between the producer's broad opened_at and closed_at clocks, with no surface-specific causality check. A Settlement outcome marked Succeeded and completed before event_end—even before the event starts—therefore passes and can contribute to Ready, despite official settlement being unavailable until the episode closes. Require the settlement completion clock to be at or after event_end so the receipt cannot assert impossible or future-leaking label availability.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in final head 6514421. A Settlement request marked Succeeded is now terminal InvalidSourceClocks when completed_at is before event_end, with a focused 05:34:59Z versus 05:35:00Z counterexample.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tools/collector/src/polymarket_evidence_artifact.rs`:
- Around line 1083-1089: The Ok branch currently publishes normalized evidence
before qualification is finalized. In the branch handling Ok((evidence, mut
verified)), call classify_polymarket_event before publish_normalized, and only
publish evidence and qualification records when the resulting state is Ready;
preserve the existing digest assignment and published_evidence updates for Ready
records while excluding Rejected/Partial results.
- Around line 1409-1423: The test fixture used by
batch_qualifies_siblings_independently_and_returns_exact_triplet_digests lacks a
valid contract row, causing market-1 to be rejected instead of ready. Replace
one empty NDJSON object in the evidence fixture with a single valid contract
record containing surface, source_token_ids, source_outcomes, market_id, symbol,
event_start, and event_end, while preserving the expected qualification
assertions.
🪄 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: 52e5833f-c336-4d12-a361-36089282769b
📒 Files selected for processing (2)
rust_hft/tools/collector/src/bin/polymarket-raw-ops.rsrust_hft/tools/collector/src/polymarket_evidence_artifact.rs
73b2ceb to
2019bec
Compare
2019bec to
6514421
Compare
Change contract
Event-local source-closed Polymarket evidence plus explicit producer provenance becomes one immutable
Ready/Partial/Rejectedqualification record, while preserving monotonic qualification state and hardened no-clobber publication.Out of scope
Dependency / merge order
Issues #315 and #327 are already merged into
main. This PR is not stacked and has no remaining merge-order dependency.Focused validation
cargo test -p hft-collector --locked polymarket_evidence_artifactcargo test -p hft-collector --locked(215 library tests passed, 2 ignored; all binary tests passed)cargo clippy -p hft-collector --all-targets --features collector-binance --no-deps --locked -- -D warningsrustfmt --edition 2021 --check tools/collector/src/polymarket_evidence_artifact.rsgit diff --check.github/scripts/agent-worktree-preflight.sh checkThe Linux-only
O_TMPFILEand integrated publication tests are CI-pending and are not claimed as locally proven on macOS.Rollout / rollback impact
Research producer publication and its CLI contract only;
polymarket-raw-ops publishnow requires a qualification JSON input. This PR performs no deployment or collector cutover. Roll back by reverting this PR; already-published immutable artifacts remain intact and are never overwritten.Scope guardrail
Two authorized files changed: 732 additions and 17 deletions, for exactly 749 total changed lines. This is below the repository review threshold and remains inside the reviewed issue boundary.
PRD note
Issue #318 is the scoped implementation contract under the #315 architecture, so a separate PRD was unnecessary.
Closes #318
Summary by CodeRabbit