Skip to content

fix(polymarket): harden execution recovery accounting - #28

Merged
proerror77 merged 1 commit into
mainfrom
codex/fix-polymarket-review-findings
Jul 15, 2026
Merged

proerror77 merged 1 commit into
mainfrom
codex/fix-polymarket-review-findings

Conversation

@proerror77

@proerror77 proerror77 commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Summary

  • enforce independent signed quantity/notional ceilings and venue-derived Polymarket V2 fees
  • make private Fill/Fee delivery atomic, replayable, generation-acknowledged, and fail-closed across reconnects
  • close generic ExecutionWorker gaps for per-client stream completion, intra-batch disconnects, and operator pause recovery
  • persist an ordered bounded accounting replay journal and reject unsafe oversized legacy recovery state

Verification

  • Engine unit/integration/doc tests: pass
  • Polymarket execution adapter: 55 passed
  • Portfolio core: 10 passed
  • Runtime: 52 passed
  • Binance execution adapter: 42 passed
  • Bybit execution adapter: 33 passed
  • hft-live deployment artifacts/envelope: 17 passed
  • strict Clippy on affected packages with warnings denied: pass
  • three-agent spec/standards/concurrency fixed-point review: no actionable findings

No live orders, cloud mutations, or shadow activation were performed.

Summary by CodeRabbit

  • New Features
    • Added configurable maximum order-quantity limits, with validation and clear rejection reasons.
    • Improved execution-event reliability through batching, acknowledgements, recovery handling, and reconciliation safeguards.
    • Added Polymarket fee-schedule support for accurate, idempotent taker-fee processing.
    • Preserved accounting consistency during replay and recovery by preventing duplicate fills and fees.
  • Bug Fixes
    • Improved trade identification to distinguish markets and fee schedules.
    • Balance synchronization now reports portfolio import failures instead of continuing silently.
  • Documentation
    • Clarified integration, recovery, fee, and synchronization behavior.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds signed order-quantity limits, bounded accounting-event replay deduplication, applied execution-stream acknowledgements, and a reliable Polymarket execution outbox. Polymarket reconciliation now uses cached V2 fee schedules, pending-trade latches, grouped fill/fee handling, and stricter recovery gating.

Changes

Execution reliability and risk controls

Layer / File(s) Summary
Signed quantity limits and deployment propagation
rust_hft/market-core/ports/*, rust_hft/market-core/runtime/*, rust_hft/market-core/engine/src/lib.rs, rust_hft/apps/live/*
Order intents, runtime configuration, strategy activation, engine limits, rejection statistics, and tests now support max_order_quantity.
Accounting deduplication and state restoration
rust_hft/market-core/engine/src/lib.rs, rust_hft/risk-control/portfolio-core/src/lib.rs, rust_hft/market-core/ports/src/traits.rs, rust_hft/apps/live/src/helpers/balance_sync.rs
Fill and fee events use bounded replay deduplication persisted in portfolio state; portfolio imports now return and handle errors.
Applied-stream acknowledgements and intake recovery
rust_hft/market-core/engine/src/execution_queues.rs, rust_hft/market-core/engine/src/execution_worker.rs
Execution workers track stream barriers, applied acknowledgements, recovery retries, bounded draining, and operator intake state.
Polymarket reliable outbox and submission flow
rust_hft/execution-gateway/adapters/adapter-polymarket/src/lib.rs, rust_hft/market-core/core/src/error.rs
Polymarket events use reserved, batched, replayable outbox delivery with staged submission outcomes and epoch-based readiness.
Polymarket fees and reconciliation
rust_hft/execution-gateway/adapters/adapter-polymarket/src/lib.rs, rust_hft/data-pipelines/adapters/adapter-polymarket/src/lib.rs, docs/architecture/POLYMARKET_TRADING_INTEGRATION.md
V2 fee schedules, pending private trades, grouped fill/fee deduplication, strict REST catch-up, and fee-aware trade IDs are implemented and documented.
Adapter failure contracts and audit updates
rust_hft/execution-gateway/adapters/adapter-binance/src/lib.rs, rust_hft/execution-gateway/adapters/adapter-bybit/src/lib.rs, rust_hft/docs/reports/UNBOUNDED_CHANNEL_AUDIT.md
Lagging private streams now report restart-required reconciliation reasons, and applied-stream acknowledgements are documented in the channel audit.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Strategy
  participant ExecutionWorker
  participant PolymarketExecutionClient
  participant PolymarketREST
  participant Engine
  Strategy->>ExecutionWorker: submit bounded order intent
  ExecutionWorker->>PolymarketExecutionClient: process intent
  PolymarketExecutionClient->>PolymarketREST: submit or reconcile order
  PolymarketExecutionClient-->>ExecutionWorker: reliable execution batch
  ExecutionWorker->>Engine: apply execution events
  Engine-->>ExecutionWorker: acknowledge synchronized stream
  ExecutionWorker->>PolymarketExecutionClient: restore or retain intake
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.17% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and matches the main change: hardening Polymarket execution recovery accounting.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-polymarket-review-findings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proerror77
proerror77 merged commit af2eb7c into main Jul 15, 2026
17 of 18 checks passed
@proerror77
proerror77 deleted the codex/fix-polymarket-review-findings branch July 15, 2026 02:59

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
rust_hft/execution-gateway/adapters/adapter-polymarket/src/lib.rs (1)

62-86: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift

Scope the reservation lock to each Polymarket client. The shared static mutex serializes reservations across independent clients even though each PolymarketExecutionClient owns its own event_tx. The builder already supports multiple Polymarket execution clients/accounts, so make this an instance field (for example Arc<StdMutex<()>>) and thread it through the reservation helpers instead of using a process-wide lock.

🤖 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/execution-gateway/adapters/adapter-polymarket/src/lib.rs` around
lines 62 - 86, Replace the process-wide EVENT_OUTBOX_RESERVATION_LOCK with an
Arc<StdMutex<()>> field on PolymarketExecutionClient, initialize it per client
in the builder, and thread that instance lock through the event-outbox
reservation helpers. Preserve reservation serialization within each client while
allowing independent clients and accounts to reserve concurrently.
🤖 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 `@docs/architecture/POLYMARKET_TRADING_INTEGRATION.md`:
- Around line 77-80: Update the FeeCharged fee formula documentation to include
traded quantity as a multiplier: quantity × rate × (price × (1 -
price))^exponent. Preserve the existing V2 fd schedule, zero-fee, validation,
and maker-fill semantics.

In `@rust_hft/market-core/engine/src/execution_worker.rs`:
- Around line 365-369: Update the intake loops around process_order_intents and
the corresponding paths at the other referenced locations to track a monotonic
recovery generation for each dequeued intents batch. Before submitting each
envelope, reject the remaining local batch when recovery was pending at dequeue
time or when the generation changes during processing, including recovery
completed by drain_private_events_before_submission; add a regression covering
recovery between the first and second submissions.
- Around line 903-915: Update the ConnectionStatus handling in the execution
worker, including the related logic around the alternate location, so a bare
connected: true event cannot restore client_connected while
stream_recovery_pending is set. Require the
ExecutionStreamBarrier/ExecutionStreamSynchronized flow and its engine-applied
acknowledgement to clear recovery and restore connectivity, while preserving
immediate disconnect handling and requiring this sequence after every
private-stream fault.

In `@rust_hft/market-core/engine/src/lib.rs`:
- Around line 132-151: Update the replay-journal validation around
recent_accounting_event_ids to reject duplicate (order_id, event_id) pairs
before inserting them into deduper. Track the number of unique restored keys and
use that count, rather than the raw recent_accounting_event_ids length, in the
legacy_event_count versus capacity completeness check; return an
HftError::Execution describing any duplicate key.

In `@rust_hft/market-core/ports/src/traits.rs`:
- Around line 774-776: Update PortfolioState deserialization for the
recent_accounting_event_ids field to supply an empty Vec when the field is
absent, preserving compatibility with legacy persisted records. Ensure
AccountingEventDeduper::from_portfolio_state can still receive legacy states and
execute its existing recovery branch.

In `@rust_hft/risk-control/portfolio-core/src/lib.rs`:
- Around line 167-173: Update record_accounting_event and the related
persisted-state export/import logic so evicting the oldest
recent_accounting_event_ids entry also removes its corresponding key from
processed_fill_ids (including the associated fill/fee deduplication state). Keep
processed state bounded by ACCOUNTING_EVENT_REPLAY_CAPACITY, while preserving
the existing legacy-state migration behavior.

---

Nitpick comments:
In `@rust_hft/execution-gateway/adapters/adapter-polymarket/src/lib.rs`:
- Around line 62-86: Replace the process-wide EVENT_OUTBOX_RESERVATION_LOCK with
an Arc<StdMutex<()>> field on PolymarketExecutionClient, initialize it per
client in the builder, and thread that instance lock through the event-outbox
reservation helpers. Preserve reservation serialization within each client while
allowing independent clients and accounts to reserve concurrently.
🪄 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

Run ID: 4a17c324-e440-47f9-b594-be661c766b71

📥 Commits

Reviewing files that changed from the base of the PR and between 102b4cc and fd559ce.

📒 Files selected for processing (22)
  • docs/architecture/POLYMARKET_TRADING_INTEGRATION.md
  • rust_hft/apps/live/src/deployment_envelope.rs
  • rust_hft/apps/live/src/helpers/balance_sync.rs
  • rust_hft/apps/live/tests/deployment_artifacts.rs
  • rust_hft/apps/live/tests/deployment_envelope.rs
  • rust_hft/data-pipelines/adapters/adapter-polymarket/src/lib.rs
  • rust_hft/docs/reports/UNBOUNDED_CHANNEL_AUDIT.md
  • rust_hft/execution-gateway/adapters/adapter-binance/src/lib.rs
  • rust_hft/execution-gateway/adapters/adapter-bybit/src/lib.rs
  • rust_hft/execution-gateway/adapters/adapter-polymarket/src/lib.rs
  • rust_hft/market-core/core/src/error.rs
  • rust_hft/market-core/engine/src/execution_control.rs
  • rust_hft/market-core/engine/src/execution_queues.rs
  • rust_hft/market-core/engine/src/execution_worker.rs
  • rust_hft/market-core/engine/src/lib.rs
  • rust_hft/market-core/ports/src/events.rs
  • rust_hft/market-core/ports/src/traits.rs
  • rust_hft/market-core/runtime/src/lib.rs
  • rust_hft/market-core/runtime/src/system_builder.rs
  • rust_hft/market-core/runtime/src/system_builder/config_loader.rs
  • rust_hft/market-core/runtime/src/system_builder/config_types.rs
  • rust_hft/risk-control/portfolio-core/src/lib.rs

Comment on lines +77 to +80
- Confirmed taker fills emit an idempotent `FeeCharged` event from the V2 market `fd` schedule using
`rate × (price × (1 - price))^exponent`. An absent `fd` follows official V2 semantics and is
treated as zero fee. Metadata request/identity errors, invalid negative rates, and positive
non-taker-only schedules fail closed; maker fills remain fee-free. The fee is reflected in Monday

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the referenced section with line numbers.
sed -n '70,90p' docs/architecture/POLYMARKET_TRADING_INTEGRATION.md | cat -n

# Look for other mentions of the fee formula or quantity/shares in the doc.
rg -n "fee|quantity|shares|taker|maker|fd|price ×|price \\* \\(1 - price\\)" docs/architecture/POLYMARKET_TRADING_INTEGRATION.md

Repository: proerror77/monday

Length of output: 3592


🌐 Web query:

Polymarket trading fees quantity shares fee formula rate price (1 - price)^exponent fee amount shares

💡 Result:

Polymarket calculates trading fees for "taker" orders using a specific formula that is not a flat percentage of the total trade value [1][2]. Instead, the fee depends on the number of shares traded, the category-specific fee rate, and the current price of the shares [1][3]. The official fee formula is [1][3]: fee = C × feeRate × p × (1 - p) In this formula: - C is the number of shares traded [1][2]. - feeRate is the category-specific taker fee rate [1][2]. - p is the price of the shares (expressed as a decimal between 0 and 1) [2][4]. Because the formula includes the p × (1 - p) term, the fee is symmetric around a 50% probability ($0.50 price) [1][3]. This means a trade at $0.30 and a trade at $0.70 will incur the same dollar-denominated fee [1][5]. The fee reaches its maximum at a price of $0.50 and decreases toward zero as the price approaches $0.00 or $1.00 [5][2]. Key details regarding this fee structure: - Makers (those placing limit orders that rest on the order book) pay zero fees [1][3][4]. - Taker fees are applied automatically by the protocol at match time; you do not need to include fee information in your orders [1][3]. - Fees are calculated in USDC [1][4]. For buy orders, the fee is typically deducted from the total shares received; for sell orders, the fee is deducted from the USDC proceeds [4]. - Fees are rounded to 5 decimal places, and the minimum fee charged is 0.00001 USDC [1][3]. Extremely small trades, especially those at prices near the extremes ($0.00 or $1.00), may result in a fee of zero due to rounding [1][3]. While your query mentioned an "exponent," the standard Polymarket documentation for its CLOB (Central Limit Order Book) does not include an explicit variable exponent in its primary fee formula [1][3]. The formula relies on the product of the probability and its complement (p * (1-p)) [5][2]. You can verify the specific parameters for any given market (including the fee rate) using the getClobMarketInfo(conditionID) function in the Polymarket SDK [1][4].

Citations:


Include traded quantity in the fee formula.
The current expression only shows the per-share curve; the fee amount should be quantity × rate × (price × (1 - price))^exponent so multi-share fills aren’t understated.

🤖 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 `@docs/architecture/POLYMARKET_TRADING_INTEGRATION.md` around lines 77 - 80,
Update the FeeCharged fee formula documentation to include traded quantity as a
multiplier: quantity × rate × (price × (1 - price))^exponent. Preserve the
existing V2 fd schedule, zero-fee, validation, and maker-fill semantics.

Comment on lines +365 to +369
let mut intents = std::mem::take(&mut self.intents_buf);
intents.clear();
self.queues.receive_envelopes_into(&mut intents);
if !intents.is_empty() {
self.process_order_intents(&mut intents).await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift

Do not reopen intake over an already-dequeued stale batch.

reconcile_open_orders() drains only the SPSC queue. If barrier processing and engine acknowledgement complete inside drain_private_events_before_submission(), recovery can reopen while remaining pre-watermark envelopes are still held in the local intents vector, causing the next envelope to be submitted.

Track a monotonic recovery generation for each dequeued batch and reject all remaining local envelopes if recovery was pending or its generation changes during processing. Add a regression where recovery completes between the first and second submissions.

Also applies to: 520-535, 718-728, 1758-1770

🤖 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/market-core/engine/src/execution_worker.rs` around lines 365 - 369,
Update the intake loops around process_order_intents and the corresponding paths
at the other referenced locations to track a monotonic recovery generation for
each dequeued intents batch. Before submitting each envelope, reject the
remaining local batch when recovery was pending at dequeue time or when the
generation changes during processing, including recovery completed by
drain_private_events_before_submission; add a regression covering recovery
between the first and second submissions.

Comment on lines 903 to +915
ExecutionEvent::ConnectionStatus { connected, .. } => {
if let Some(state) = self.client_connected.get_mut(client_idx) {
*state = *connected;
let barrier_pending = self
.client_stream_barriers
.get(client_idx)
.is_some_and(Option::is_some);
if !connected || !barrier_pending {
if let Some(state) = self.client_connected.get_mut(client_idx) {
*state = *connected;
}
}
if !connected {
self.accepting_intents = false;
self.stream_recovery_pending = true;
self.latch_stream_recovery();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win

Require a new stream generation after every private-stream fault.

After a disconnect, a bare ConnectionStatus { connected: true } sets client_connected and triggers reconciliation when no barrier is pending. A matching order snapshot can therefore reopen intake without any ExecutionStreamBarrier/ExecutionStreamSynchronized marker or engine-applied watermark.

While stream_recovery_pending is set, ignore connected-only recovery; only the synchronized marker plus its applied acknowledgement should restore connectivity.

Also applies to: 989-1003

🤖 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/market-core/engine/src/execution_worker.rs` around lines 903 - 915,
Update the ConnectionStatus handling in the execution worker, including the
related logic around the alternate location, so a bare connected: true event
cannot restore client_connected while stream_recovery_pending is set. Require
the ExecutionStreamBarrier/ExecutionStreamSynchronized flow and its
engine-applied acknowledgement to clear recovery and restore connectivity, while
preserving immediate disconnect handling and requiring this sequence after every
private-stream fault.

Comment on lines +132 to +151
if !state.recent_accounting_event_ids.is_empty() {
if legacy_event_count > capacity && state.recent_accounting_event_ids.len() < capacity {
return Err(HftError::Execution(format!(
"portfolio accounting replay journal is incomplete: {} processed IDs but only {} ordered recent keys",
legacy_event_count,
state.recent_accounting_event_ids.len()
)));
}
for (order_id, event_id) in &state.recent_accounting_event_ids {
let valid = event_id
.strip_prefix("fill:")
.or_else(|| event_id.strip_prefix("fee:"))
.is_some_and(|fill_id| !fill_id.is_empty());
if !valid {
return Err(HftError::Execution(format!(
"portfolio accounting replay journal contains invalid key {event_id:?}"
)));
}
deduper.insert((order_id.clone(), event_id.clone()));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject duplicate keys in the persisted replay journal.

Line 133 validates the journal using its raw length, but Line 150 silently collapses duplicate (order_id, event_id) entries. A capacity-sized journal containing duplicates can therefore pass as complete while restoring a shorter dedupe horizon, allowing omitted fills or fees to be applied again.

Validate uniqueness and use the restored unique-key count for the completeness check.

Proposed fix
+            let mut journal_keys = HashSet::with_capacity(
+                state.recent_accounting_event_ids.len(),
+            );
             for (order_id, event_id) in &state.recent_accounting_event_ids {
                 let valid = event_id
                     .strip_prefix("fill:")
                     .or_else(|| event_id.strip_prefix("fee:"))
                     .is_some_and(|fill_id| !fill_id.is_empty());
                 if !valid {
                     return Err(HftError::Execution(format!(
                         "portfolio accounting replay journal contains invalid key {event_id:?}"
                     )));
                 }
-                deduper.insert((order_id.clone(), event_id.clone()));
+                let key = (order_id.clone(), event_id.clone());
+                if !journal_keys.insert(key.clone()) {
+                    return Err(HftError::Execution(format!(
+                        "portfolio accounting replay journal contains duplicate key {key:?}"
+                    )));
+                }
+                deduper.insert(key);
             }
+            if legacy_event_count > capacity && deduper.ids.len() < capacity {
+                return Err(HftError::Execution(
+                    "portfolio accounting replay journal has an incomplete unique horizon"
+                        .to_string(),
+                ));
+            }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if !state.recent_accounting_event_ids.is_empty() {
if legacy_event_count > capacity && state.recent_accounting_event_ids.len() < capacity {
return Err(HftError::Execution(format!(
"portfolio accounting replay journal is incomplete: {} processed IDs but only {} ordered recent keys",
legacy_event_count,
state.recent_accounting_event_ids.len()
)));
}
for (order_id, event_id) in &state.recent_accounting_event_ids {
let valid = event_id
.strip_prefix("fill:")
.or_else(|| event_id.strip_prefix("fee:"))
.is_some_and(|fill_id| !fill_id.is_empty());
if !valid {
return Err(HftError::Execution(format!(
"portfolio accounting replay journal contains invalid key {event_id:?}"
)));
}
deduper.insert((order_id.clone(), event_id.clone()));
}
if !state.recent_accounting_event_ids.is_empty() {
if legacy_event_count > capacity && state.recent_accounting_event_ids.len() < capacity {
return Err(HftError::Execution(format!(
"portfolio accounting replay journal is incomplete: {} processed IDs but only {} ordered recent keys",
legacy_event_count,
state.recent_accounting_event_ids.len()
)));
}
let mut journal_keys = HashSet::with_capacity(
state.recent_accounting_event_ids.len(),
);
for (order_id, event_id) in &state.recent_accounting_event_ids {
let valid = event_id
.strip_prefix("fill:")
.or_else(|| event_id.strip_prefix("fee:"))
.is_some_and(|fill_id| !fill_id.is_empty());
if !valid {
return Err(HftError::Execution(format!(
"portfolio accounting replay journal contains invalid key {event_id:?}"
)));
}
let key = (order_id.clone(), event_id.clone());
if !journal_keys.insert(key.clone()) {
return Err(HftError::Execution(format!(
"portfolio accounting replay journal contains duplicate key {key:?}"
)));
}
deduper.insert(key);
}
if legacy_event_count > capacity && deduper.ids.len() < capacity {
return Err(HftError::Execution(
"portfolio accounting replay journal has an incomplete unique horizon"
.to_string(),
));
}
🤖 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/market-core/engine/src/lib.rs` around lines 132 - 151, Update the
replay-journal validation around recent_accounting_event_ids to reject duplicate
(order_id, event_id) pairs before inserting them into deduper. Track the number
of unique restored keys and use that count, rather than the raw
recent_accounting_event_ids length, in the legacy_event_count versus capacity
completeness check; return an HftError::Execution describing any duplicate key.

Comment on lines +774 to +776
/// Engine accounting replay horizon, ordered oldest to newest. Hash-based portfolio fill sets
/// cannot reconstruct recency once the bounded engine deduper reaches capacity.
pub recent_accounting_event_ids: Vec<(OrderId, String)>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Default the new replay-journal field during deserialization.

Legacy persisted PortfolioState records do not contain this field, so deserialization can fail before AccountingEventDeduper::from_portfolio_state reaches its legacy recovery branch.

Proposed fix
     /// Engine accounting replay horizon, ordered oldest to newest. Hash-based portfolio fill sets
     /// cannot reconstruct recency once the bounded engine deduper reaches capacity.
+    #[serde(default)]
     pub recent_accounting_event_ids: Vec<(OrderId, String)>,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Engine accounting replay horizon, ordered oldest to newest. Hash-based portfolio fill sets
/// cannot reconstruct recency once the bounded engine deduper reaches capacity.
pub recent_accounting_event_ids: Vec<(OrderId, String)>,
/// Engine accounting replay horizon, ordered oldest to newest. Hash-based portfolio fill sets
/// cannot reconstruct recency once the bounded engine deduper reaches capacity.
#[serde(default)]
pub recent_accounting_event_ids: Vec<(OrderId, String)>,
🤖 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/market-core/ports/src/traits.rs` around lines 774 - 776, Update
PortfolioState deserialization for the recent_accounting_event_ids field to
supply an empty Vec when the field is absent, preserving compatibility with
legacy persisted records. Ensure AccountingEventDeduper::from_portfolio_state
can still receive legacy states and execute its existing recovery branch.

Comment on lines +167 to +173
fn record_accounting_event(&mut self, order_id: OrderId, event_id: String) {
if self.recent_accounting_event_ids.len() >= ACCOUNTING_EVENT_REPLAY_CAPACITY {
self.recent_accounting_event_ids.pop_front();
}
self.recent_accounting_event_ids
.push_back((order_id, event_id));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Bound the persisted deduplication state, not only the chronological journal.

Eviction removes an entry from recent_accounting_event_ids, but the same key remains indefinitely in processed_fill_ids; export and import therefore retain unbounded memory and snapshot growth. Evict the corresponding fill/fee key—or rebuild the processed sets from the bounded journal—while preserving the documented legacy-state migration.

Also applies to: 287-318

🤖 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/risk-control/portfolio-core/src/lib.rs` around lines 167 - 173,
Update record_accounting_event and the related persisted-state export/import
logic so evicting the oldest recent_accounting_event_ids entry also removes its
corresponding key from processed_fill_ids (including the associated fill/fee
deduplication state). Keep processed state bounded by
ACCOUNTING_EVENT_REPLAY_CAPACITY, while preserving the existing legacy-state
migration behavior.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd559ceb8a

ℹ️ 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".

}

async fn cancel_order(&mut self, order_id: &OrderId) -> HftResult<()> {
// Cancellation is risk-reducing and remains available through authenticated REST while
// private/account recovery keeps all new exposure disabled.
let client = self.ensure_cancel_ready()?;
let venue_id = self.resolve_venue_id(order_id).await;
let event_permit = reserve_event_batch(&self.event_tx)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Make cancels independent of outbox backpressure

When the reliable event outbox is full (for example because the worker/engine is stalled or the pre-subscription backlog has reached PRE_SUBSCRIPTION_EVENT_CAPACITY), this ? returns before the REST cancel_order call is made. That makes emergency, ack-timeout, or operator cancellations fail solely due to local report buffering, leaving live Polymarket exposure open even though the comment says cancellation remains risk-reducing and available through REST; the cancel path should wait for/report capacity or otherwise avoid blocking the venue cancel on this local permit.

Useful? React with 👍 / 👎.

params: runtime::StrategyParams::Formula {
ast: ast.clone(),
max_order_notional: order_notional,
max_order_notional: total_notional,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep max_order_size as the per-order notional cap

For activations where max_order_size is lower than the deployment notional/symbol ceilings, this now sizes the formula strategy from total_notional instead of the previous per-order cap. The new intent_max_order_quantity is a unit/share limit, so it does not preserve the USD/order-size ceiling for non-Polymarket venues (e.g. a BTC strategy can now emit up to the 500 notional ceiling when the envelope's max_order_size was 100), widening the deployment risk limit.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant