fix(risk): enforce batch exposure and tokenized policy - #61
Conversation
|
Warning Review limit reached
Next review available in: 25 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 (12)
✨ 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 |
78ca7a8 to
a0994d7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d92891fe4b
ℹ️ 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 !incoming.is_zero() { | ||
| let key = ExposureKey { | ||
| venue: intent.target_venue, |
There was a problem hiding this comment.
When target_venue is None, risk runs before the execution router chooses a venue (engine/src/lib.rs:1592-1598, then execution_worker.rs:1231-1232), but this key makes every unresolved-venue order share the same venue bucket. A same-symbol buy and sell with no target venue therefore net to zero for symbol_gross_quantity and can both pass the position cap, even though RoundRobin/StrategyMap can later send them to different venues. Treat unresolved venues as non-nettable or route before this projection.
Useful? React with 👍 / 👎.
| if projected.symbol_gross_quantity > self.max_position_per_symbol | ||
| || projected.gross_notional > self.max_global_notional | ||
| { |
There was a problem hiding this comment.
When the account is already above a position/notional cap, this pre-filter rejects every intent whose projected gross exposure is still above the cap. Because ExposureProjector seeds existing exposure and adds closing order notional instead of recognizing a signed reduction, a sell that reduces a long (or buy reducing a short) is dropped here before DefaultRiskManager's explicit reduce-order bypass can run; live systems use this wrapper through SystemBuilder, so breached accounts can be unable to submit exits.
Useful? React with 👍 / 👎.
| pub ws_reconnect_count: u32, | ||
| /// 數據間隙次數 | ||
| pub data_gap_count: u32, | ||
| pub data_gap_count: u64, |
There was a problem hiding this comment.
Removing ws_reconnect_count leaves an existing SystemStats literal in rust_hft/apps/live/src/helpers/sentinel.rs:98-109 with ws_reconnect_count: 0. That crate will fail to compile as soon as the live app is checked, so the caller needs to be updated with this API change (or the field retained).
Useful? React with 👍 / 👎.
| let portfolio_names = self.manager.portfolios_for_strategy(&intent.strategy_id); | ||
| if portfolio_names.is_empty() { | ||
| approved.push(intent); |
There was a problem hiding this comment.
For normal multi-symbol strategies, the factory generates runtime ids like <config.name>:<symbol> (system_builder/strategy_factory.rs:119), while portfolio definitions are validated against StrategyConfig.name without the symbol. If a portfolio lists the documented/configured strategy name, this lookup returns empty for every generated intent and the new budget wrapper approves them without applying max_notional or max_position; normalize both names or expand portfolio memberships to the actual instance ids.
Useful? React with 👍 / 👎.
| fn update_config(&mut self, update: ports::RiskConfigUpdate) -> Result<(), hft_core::HftError> { | ||
| self.base_risk_manager.update_config(update) |
There was a problem hiding this comment.
This wrapper caches max_global_notional at construction but update_config only forwards changes to the inner manager. When an operator raises the global notional limit through RiskConfigUpdate::max_position_usd, get_config_snapshot() reflects the new inner limit while this pre-filter still rejects anything above the old cap before the base manager can approve it, so runtime limit increases do not actually take effect.
Useful? React with 👍 / 👎.
| let max_position = system_risk_config | ||
| .enhanced | ||
| .as_ref() | ||
| .map(|config| config.max_position_per_symbol) | ||
| .filter(|limit| *limit > rust_decimal::Decimal::ZERO) | ||
| .unwrap_or(system_risk_config.global_position_limit); |
There was a problem hiding this comment.
In risk_type = "Enhanced" with no explicit enhanced section, create_risk_manager builds an EnhancedRiskManager from EnhancedRiskConfig::default() (which has its own position cap), but this wrapper falls back to system_risk_config.global_position_limit. Since the runtime RiskConfig default for that field is zero and the enhanced manager otherwise ignores it, the SystemBuilder path can reject every positive order before the enhanced risk manager sees it.
Useful? React with 👍 / 👎.
| venue_check, | ||
| rate_check, | ||
| self.check_position_limits(&intent, account), | ||
| self.check_position_limits(&intent, &projected_account), |
There was a problem hiding this comment.
For callers that use DefaultRiskManager directly, passing a projected account here still lets same-symbol batches revalue already-approved exposure at the next order's price because check_position_limits applies the current intent.price to the entire projected position. A batch such as 100 @ 1000 followed by 100 @ 1 can pass a global notional cap just above 100,000 even though the aggregate order notional exceeds it; use an order-notional projector or carry weighted/accumulated notional instead of the last price.
Useful? React with 👍 / 👎.
| for (portfolio_name, projector) in projections { | ||
| projectors.insert(portfolio_name, projector); | ||
| } | ||
| approved.push(intent); |
There was a problem hiding this comment.
The portfolio projector is advanced before the inner risk manager has accepted the order. In a single batch, an earlier same-portfolio intent that this layer accepts but the base manager later rejects (for example a tokenized-security intent rejected by the new tokenized wrapper, or an intent with no matching VenueSpec) can consume the temporary portfolio budget and cause a later valid intent to be dropped even though the first order will never be sent.
Useful? React with 👍 / 👎.
| self.last_data_gap_count = stats.data_gap_count; | ||
| SentinelAction::Stop |
There was a problem hiding this comment.
This branch records the new last_data_gap_count before returning Stop; on the next sentinel tick with the same cumulative count, data_action becomes Continue, and update_state(Continue) moves a non-Recovering sentinel back to Normal. A single market-data gap therefore clears the sentinel state on the following clean tick instead of requiring operator-controlled recovery as the new comment states.
Useful? React with 👍 / 👎.
…risk # Conflicts: # rust_hft/apps/live/src/helpers/sentinel.rs
Change contract
Reject aggregate cross-venue exposure and tokenized-security intents unless venue-scoped, fresh, authoritative evidence and portfolio budgets are satisfied.
Out of scope
No account snapshot/reconciliation implementation, readiness endpoint semantics, backtest behavior, or promotion lifecycle changes.
Dependencies and merge order
Depends on #60 because batch projection consumes the account-truth and per-position PnL contract. #60 and #62 are now merged; this PR targets
main.Focused validation
cargo test -p hft-runtime --libcargo test -p hft-riskcargo test -p hft-engine --test tokenized_security_execution_e2ecargo test -p hft-infra-metricscargo test -p hft-live runtime_attribution::tests:: --libcargo check -p hft-live --features clickhouse,redis,grpc --lockedcargo --config build.rustc-wrapper="" build --release --locked -p hft-live --features clickhouse,redis,grpcRollout and rollback
New or unproven exposure fails closed. Revert this PR to remove these additional policy gates.
Scope assessment
908 non-generated added lines across 12 files exceed the 750-line assessment threshold. They form one fail-closed behavior contract: projected exposure, portfolio budgets, venue evidence, tokenized-policy enforcement, and the sole live
SystemStatsconstructor must land together or an execution path can bypass the same limit or fail to compile. Splitting them would not create an independently safe rollback unit.Scope exception
Atomic exception approved for review by
proerror77; reviewer should verify the shared batch-limit invariant rather than treating the touched modules as separate features.