Disable legacy examples and align with minimal API #10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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
Comment on lines
+126
to
148
| /// Validate an order against simple position size limits and the emergency stop flag. | ||
| pub fn validate_order( | ||
| &self, | ||
| order: &OrderRequest, | ||
| _positions: &HashMap<String, Position>, | ||
| ) -> Result<()> { | ||
| if self.emergency_stop { | ||
| return Err(RiskError::General("Emergency stop is active".to_string())); | ||
| } | ||
|
|
||
| // Check daily loss limit | ||
| if self.daily_tracker.is_daily_loss_limit_reached(self.config.max_daily_loss_pct) { | ||
| let daily_loss_pct = self.daily_tracker.daily_loss_pct(); | ||
| return Err(RiskError::DailyLossLimitReached { | ||
| current_loss_pct: daily_loss_pct, | ||
| max_loss_pct: self.config.max_daily_loss_pct, | ||
| }); | ||
| } | ||
|
|
||
| // Check drawdown limit | ||
| if self.portfolio_metrics.max_drawdown > self.config.max_drawdown_pct { | ||
| return Err(RiskError::DrawdownLimitExceeded { | ||
| current_drawdown_pct: self.portfolio_metrics.max_drawdown * 100.0, | ||
| max_drawdown_pct: self.config.max_drawdown_pct * 100.0, | ||
| }); | ||
| } | ||
|
|
||
| // Calculate order value | ||
| let order_value = match order.price { | ||
| Some(price) => order.quantity * price, | ||
| None => { | ||
| // For market orders, we need to estimate the price | ||
| // In a real implementation, we would use the current market price | ||
| // For simplicity, we'll use the current position price if available | ||
| if let Some(position) = current_positions.get(&order.symbol) { | ||
| order.quantity * position.current_price | ||
| } else { | ||
| // If no position exists, we can't validate the order properly | ||
| return Err(RiskError::General( | ||
| "Cannot validate market order without price information".to_string() | ||
| )); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Check position size limit | ||
| let max_position_value = self.portfolio_value * self.config.max_position_size_pct; | ||
|
|
||
| // Calculate the total position value after this order | ||
| let mut new_position_value = order_value; | ||
| if let Some(position) = current_positions.get(&order.symbol) { | ||
| // Add existing position value | ||
| new_position_value = match order.side { | ||
| OrderSide::Buy => position.size.abs() * position.current_price + order_value, | ||
| OrderSide::Sell => { | ||
| if order.quantity <= position.size { | ||
| // Reducing position | ||
| (position.size - order.quantity).abs() * position.current_price | ||
| } else { | ||
| // Flipping position | ||
| (order.quantity - position.size).abs() * position.current_price | ||
| } | ||
| } | ||
| }; | ||
| return Err(RiskError::TradingHalted); | ||
| } | ||
|
|
||
| // Apply volatility-based position sizing if volatility data is available | ||
| if let Some(volatility_data) = self.volatility_data.get(&order.symbol) { | ||
| let volatility_adjusted_max_size = self.calculate_volatility_adjusted_position_size( | ||
| &order.symbol, | ||
| max_position_value | ||
| ); | ||
|
|
||
| if new_position_value > volatility_adjusted_max_size { | ||
|
|
||
| if let Some(price) = order.price { | ||
| let notional = price * order.quantity.abs(); | ||
| let max_notional = self.config.max_position_size_pct * self.portfolio_value; | ||
| if max_notional > 0.0 && notional > max_notional { | ||
| return Err(RiskError::PositionSizeExceeded { | ||
| message: format!( | ||
| "Position value ${:.2} exceeds volatility-adjusted limit ${:.2}", | ||
| new_position_value, volatility_adjusted_max_size | ||
| "order notional {:.2} exceeds {:.2} ({:.2}% of portfolio)", | ||
| notional, | ||
| max_notional, | ||
| self.config.max_position_size_pct * 100.0, | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
[P1] Enforce size limit for market orders
The only position-size check is in RiskManager::validate_order, but the code bails out unless order.price is Some. Market orders created by OrderRequest::market always have price = None, so any market order—regardless of quantity—will pass validation and can blow past max_position_size_pct and the configured portfolio value. Either derive a notional from current market data or reject market orders that do not provide a price so that the risk limit cannot be bypassed.
Useful? React with 👍 / 👎.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
RiskConfigvia the prelude so downstream callers and examples can construct aRiskManagerTesting
CARGO_NET_OFFLINE=true cargo testhttps://chatgpt.com/codex/tasks/task_e_68d558510d94832bb4c2ea4087b29145