Record complete Polymarket trade sets - #153
Conversation
|
Warning Review limit reached
Next review available in: 12 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 Run ID: 📒 Files selected for processing (2)
✨ 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: 1870d8880e
ℹ️ 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".
| self.config.trade_finalization_stable_polls, | ||
| ) | ||
| { | ||
| updates.push(trade_completion_update( |
There was a problem hiding this comment.
When a previously tracked market is absent from discovery and its authoritative detail fetch is deferred until after the hour-seeding cycle, market_metadata_update at lines 2294-2303 returns None because the unchanged metadata hash was persisted from an earlier hour. This path can still append the completion here after its stable polls, but the resulting hourly tape contains no metadata identity for that market; the uploader then unconditionally rejects it at polymarket_upload.rs:1176-1185 as contradicting market metadata, permanently blocking archival of that closed segment. Ensure the market has metadata in the current tape before emitting its completion proof.
Useful? React with 👍 / 👎.
| self.config.trade_finalization_stable_polls, | ||
| ) | ||
| { | ||
| updates.push(trade_completion_update( |
There was a problem hiding this comment.
If Gamma temporarily returns no valid settlement during an otherwise successful trade poll that discovers a new trade, the settlement_available guard prevents advance_trade_finalization from running, so its new_trade_count > 0 reset never happens and the prior stable-poll count survives. On the next poll where settlement is valid and no trade is new, this block can emit a completion proof after only one stable poll following the newly discovered trade instead of the configured number, allowing late trades to be missed. Reset the counter for every destabilizing trade result even when settlement is unavailable.
Useful? React with 👍 / 👎.
| if state.markets.len() > max_markets | ||
| || state.trade_seen.len() > max_markets |
There was a problem hiding this comment.
max_markets is explicitly a per-lane limit (DEFAULT_MAX_MARKETS_PER_LANE), and GammaDiscovery::append_page maintains independent open and closed counters, so a valid discovery can retain up to twice this many distinct markets and condition maps. Comparing the combined state.markets and state.trade_seen sizes to only one lane's limit causes the collector to reject otherwise valid cycles or existing state whenever the two lanes together exceed max_markets, even though neither lane exceeded its configured cap. Use a checked combined-lane bound here.
Useful? React with 👍 / 👎.
Change contract: The Polymarket reference collector emits an event-local trade completion proof only after authoritative settlement, exhausted pagination, stable post-settlement polls, and retention of the exact canonical v2 trade ID set until proof emission.
Out of scope: Downstream multi-hour research import, evidence sealing, strategy evaluation, and live execution.
Dependency or merge order: None. Merge this PR before the stacked reference-segment-set and evidence-binding PRs.
Focused validation: cargo test --manifest-path rust_hft/Cargo.toml -p hft-collector --lib (167 passed, 2 ignored); cargo clippy --manifest-path rust_hft/Cargo.toml -p hft-collector --all-targets --features collector-binance -- -D warnings; cargo fmt --manifest-path rust_hft/Cargo.toml -- --check; git diff --check. Independent correctness and standards reviews at fixed head 1870d88 reported no findings.
Rollout/rollback impact: Adds a raw collector marker and state-version migration; legacy completed markets reopen collection fail-closed. Roll back by reverting this PR; downstream consumers must not merge before this base. Live execution remains disabled.