fix(polymarket): admit trailing completion proof inputs - #298
Conversation
📝 WalkthroughWalkthroughValidation now uses explicit market selection to gate quote-collection failures and completion-only references. Reference ordering, UTC-hour constraints, event composition, test helpers, and completion-only scenarios were updated. ChangesExplicit reference validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rust_hft/prediction-markets/crates/ploy-market-data/src/polymarket_evidence/artifact.rs (2)
695-753: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompletion-only sequencing logic verified correct.
Traced this against all new/existing tests (later-only OK, sparse/non-explicit rejected, early/interleaved rejected, reversed rejected, same-hour overlap rejected): the all-references ordering check (701-711) plus the
data_references-only consecutive-hour check (735-743) plus thehas_prior_metadata/data_references.is_empty()guard (716-719) and the "must follow all evidence data" guard (726-733) together correctly implement "ordered & non-overlapping for all segments, consecutive UTC hours for data-bearing segments only, trailing-only completion proofs." No correctness issue found.One coverage gap: no test exercises a chain of multiple consecutive completion-only references after the data segments (the logic appears to permit this, consistent with "trailing reference segments" being plural in the PR summary), so a regression here wouldn't be caught.
🤖 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/prediction-markets/crates/ploy-market-data/src/polymarket_evidence/artifact.rs` around lines 695 - 753, Add a regression test covering multiple consecutive completion-only references after the data-bearing segments in the validation flow around the ordering and sequencing checks. Ensure the test confirms the trailing completion-only chain is accepted while preserving the existing requirements that completion-only references follow evidence data and remain trailing.
1419-1549: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage of new sequencing rules; consider adding a chained completion-only test.
These four tests correctly pin down: later completion-only accepted, sparse completion-only rejected when the manifest isn't explicit, early/interleaved completion-only rejected with distinct messages, and malformed completion counts still rejected. Consider adding a case with two (or more) consecutive completion-only references trailing the data segments, since the implementation in
validate_inputs(695-753) appears to permit that but it's currently untested.🤖 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/prediction-markets/crates/ploy-market-data/src/polymarket_evidence/artifact.rs` around lines 1419 - 1549, Add a test alongside the existing completion-only sequencing tests that appends at least two consecutive valid completion-only references after the final evidence-data reference. Use the existing input builders and sealing flow, preserve chronological ordering and valid completion metadata, and assert that validate_inputs via seal_polymarket_evidence_triplet succeeds.
🤖 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/prediction-markets/crates/ploy-market-data/src/polymarket_evidence/artifact.rs`:
- Around line 695-753: Add a regression test covering multiple consecutive
completion-only references after the data-bearing segments in the validation
flow around the ordering and sequencing checks. Ensure the test confirms the
trailing completion-only chain is accepted while preserving the existing
requirements that completion-only references follow evidence data and remain
trailing.
- Around line 1419-1549: Add a test alongside the existing completion-only
sequencing tests that appends at least two consecutive valid completion-only
references after the final evidence-data reference. Use the existing input
builders and sealing flow, preserve chronological ordering and valid completion
metadata, and assert that validate_inputs via seal_polymarket_evidence_triplet
succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72137ead-9e44-413e-8319-116916d64605
📒 Files selected for processing (1)
rust_hft/prediction-markets/crates/ploy-market-data/src/polymarket_evidence/artifact.rs
dd03fca to
95ea575
Compare
Change contract
For explicit-market Polymarket evidence only, admit authenticated trailing reference segments that contain redundant metadata plus exact completion proofs but no trade or settlement history, while keeping every segment ordered/non-overlapping and every data-bearing segment consecutive by UTC hour.
Out of scope
Collector or deployment changes; evidence byte or digest formulas; snapshot sampling or factor logic; Binance inputs; evaluator/MCTS; Paper or Live.
Dependency or merge order
Base:
main@23af4258bf452a94c8c630ad1f8d8d5479f6b954. Depends on the event-local sparse-reference producer contract merged in #278 and verifier fixes through #294. The unrelated frontend audit blocker was split into #299/#300 and is merged. Merge this PR before retrying #232 with an exact-main research image.Focused validation
validated reference inputs must be consecutive UTC hours.cargo test -p ploy-market-data- 60 passed.cargo test -p ploy-research --test mission_search_policies -- polymarket_missions_pin_current_search_policy_snapshot- passed for BTC 5m and SOL 5m governed missions.cargo fmt -p ploy-market-data -- --check- passed.cargo clippy -p ploy-market-data --all-targets --no-deps -- -D warnings- passed.git diff --check- passed.Rollout or rollback impact
Rollout publishes a new immutable exact-main research image, then reruns the unchanged #232 evidence in ACK. Rollback is a one-commit revert; no existing artifact or collector state is mutated.
Root cause
PR #278 intentionally allowed explicit event-local evidence normalization to consume nondecreasing sparse reference hours, but the downstream typed verifier retained PR #154's earlier consecutive-hour rule. This producer/verifier contract drift rejected a valid later completion-proof segment.
Why no PRD
#297 defines one already-approved verifier predicate and its counterexamples, so Matt TDD was the narrowest workflow.
Tracks #297. Cloud acceptance remains post-merge and will be recorded on the issue before it is closed.