From b555f6e00440b386daba61b40c4698a071f2af3e Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 28 Feb 2026 17:23:58 -0400 Subject: [PATCH] Fix high-severity issues identified by Codex 1. Paper equity calculation (High): - get_balance() now correctly includes position value (size * current_price) - Previously only added unrealized_pnl, missing position principal 2. Buy affordability checks (High): - place_order() now uses execution price including slippage for market orders - Uses limit price for limit orders instead of current price - Prevents orders that would drive cash negative 3. ExchangeInterface price stubs (High): - get_trade_executed_price() now handles order_price=0.0 (market orders) - Falls back to current price or entry price when order_price is zero 4. Determinism hash strengthening (Medium): - Hash now includes full trade sequence with individual trade details - Added test for different trade sequences with same totals 5. Live mode consistency (Medium): - Factory now falls back to paper trading with warning instead of raising - Consistent behavior between factory.py and Strategies.py Added test: test_paper_broker_equity_calculation to verify fix #1 Co-Authored-By: Claude Opus 4.5 --- src/ExchangeInterface.py | 17 +++++- src/backtest_result.py | 21 ++++++- src/brokers/factory.py | 14 ++++- src/brokers/paper_broker.py | 19 ++++-- tests/test_backtest_determinism.py | 96 +++++++++++++++++++++--------- tests/test_brokers.py | 40 +++++++++++-- 6 files changed, 163 insertions(+), 44 deletions(-) diff --git a/src/ExchangeInterface.py b/src/ExchangeInterface.py index 9b55637..effb035 100644 --- a/src/ExchangeInterface.py +++ b/src/ExchangeInterface.py @@ -352,13 +352,24 @@ class ExchangeInterface: """ Get the executed price of a trade order. - For paper/backtest modes, returns the order price. + For paper/backtest modes, returns the order price or current market price. :param trade: The trade object. :return: Executed price. """ - # For paper trading / backtesting, use order price - return trade.order_price + # For paper trading / backtesting, use order price if set + if trade.order_price and trade.order_price > 0: + return trade.order_price + + # For market orders (order_price=0), get current price + try: + return self.get_price(trade.symbol) + except Exception: + # Fallback: if we can't get price, return entry price if available + if hasattr(trade, 'entry_price') and trade.entry_price > 0: + return trade.entry_price + # Last resort: return 0 and let caller handle it + return 0.0 def get_user_balance(self, user_id: int) -> float: """ diff --git a/src/backtest_result.py b/src/backtest_result.py index a08d381..6d7bfb4 100644 --- a/src/backtest_result.py +++ b/src/backtest_result.py @@ -117,21 +117,36 @@ class BacktestResult: Generate a hash of the deterministic parts of the result. Excludes non-deterministic fields like run_datetime and run_duration. + Includes full trade sequence to detect different trade distributions. """ import hashlib - # Include only deterministic fields + # Normalize trade data for comparison (include sequence information) + normalized_trades = [] + for i, t in enumerate(self.trades): + normalized_trades.append({ + 'seq': i, # Preserve order + 'pnl': round(t.get('pnl', 0), 6), + 'pnlcomm': round(t.get('pnlcomm', t.get('pnl', 0)), 6), + 'size': round(t.get('size', 0), 6), + 'open_price': round(t.get('open_price', 0), 6), + 'close_price': round(t.get('close_price', 0), 6) if t.get('close_price') else None, + }) + + # Include only deterministic fields with full trade sequence deterministic_data = { 'strategy_id': self.strategy_id, 'initial_capital': self.initial_capital, 'final_portfolio_value': round(self.final_portfolio_value, 6), 'equity_curve': [round(e, 6) for e in self.equity_curve], - 'trades_count': len(self.trades), - 'trades_pnl': sum(t.get('pnl', 0) for t in self.trades), + 'trades': normalized_trades, # Full trade sequence 'metrics': { 'total_return': round(self.metrics.total_return, 6), 'number_of_trades': self.metrics.number_of_trades, + 'sharpe_ratio': round(self.metrics.sharpe_ratio, 6), + 'max_drawdown': round(self.metrics.max_drawdown, 6), 'win_rate': round(self.metrics.win_rate, 6), + 'profit_factor': round(self.metrics.profit_factor, 6), } } diff --git a/src/brokers/factory.py b/src/brokers/factory.py index 94ac5f1..481ebe6 100644 --- a/src/brokers/factory.py +++ b/src/brokers/factory.py @@ -71,10 +71,18 @@ def create_broker( ) elif mode == TradingMode.LIVE: - # Live broker will be implemented in Phase 5 - raise NotImplementedError( + # Live broker not yet implemented - fall back to paper trading with warning + logger.warning( "Live trading broker not yet implemented. " - "Use paper trading for testing with live prices." + "Falling back to paper trading for safety." + ) + return PaperBroker( + initial_balance=initial_balance, + commission=commission, + slippage=slippage if slippage > 0 else 0.0005, + price_provider=price_provider, + data_cache=data_cache, + **kwargs ) else: diff --git a/src/brokers/paper_broker.py b/src/brokers/paper_broker.py index 126c6f8..cccc16e 100644 --- a/src/brokers/paper_broker.py +++ b/src/brokers/paper_broker.py @@ -144,8 +144,18 @@ class PaperBroker(BaseBroker): message=f"Cannot get current price for {symbol}" ) - # Calculate required margin/cost - order_value = size * current_price + # Calculate required margin/cost using the actual execution price + # For market orders: include slippage; for limit orders: use limit price + if order_type == OrderType.MARKET: + if side == OrderSide.BUY: + execution_price = current_price * (1 + self.slippage) + else: + execution_price = current_price * (1 - self.slippage) + else: + # For limit orders, use the limit price (or current price if not specified) + execution_price = price if price else current_price + + order_value = size * execution_price required_funds = order_value * (1 + self.commission) if side == OrderSide.BUY and required_funds > self._cash: @@ -306,10 +316,11 @@ class PaperBroker(BaseBroker): return open_orders def get_balance(self, asset: Optional[str] = None) -> float: - """Get total balance including unrealized P&L.""" + """Get total equity (cash + locked + position values).""" total = self._cash + self._locked_balance for position in self._positions.values(): - total += position.unrealized_pnl + # Include full position value (size * current_price), not just unrealized PnL + total += position.size * position.current_price return total def get_available_balance(self, asset: Optional[str] = None) -> float: diff --git a/tests/test_backtest_determinism.py b/tests/test_backtest_determinism.py index 18d43f4..69c2416 100644 --- a/tests/test_backtest_determinism.py +++ b/tests/test_backtest_determinism.py @@ -120,6 +120,20 @@ class TestBacktestDeterminism: def test_same_inputs_same_hash(self): """Test that identical inputs produce the same hash.""" + # Use identical trade data with all required fields + trades = [ + {'ref': 1, 'pnl': 500, 'pnlcomm': 499, 'size': 0.1, 'open_price': 50000, 'close_price': 55000, 'side': 'buy'}, + {'ref': 2, 'pnl': 500, 'pnlcomm': 499, 'size': 0.1, 'open_price': 55000, 'close_price': 60000, 'side': 'sell'}, + ] + stats = { + 'total_return': 10.0, + 'number_of_trades': 2, + 'win_rate': 100.0, + 'sharpe_ratio': 1.5, + 'max_drawdown': -2.0, + 'profit_factor': 2.0, + } + result1 = create_backtest_result( strategy_id='strategy-abc', strategy_name='Test Strategy', @@ -128,15 +142,8 @@ class TestBacktestDeterminism: initial_capital=10000.0, final_value=11000.0, equity_curve=[10000, 10500, 11000], - trades=[ - {'ref': 1, 'pnl': 500, 'side': 'buy'}, - {'ref': 2, 'pnl': 500, 'side': 'sell'}, - ], - stats={ - 'total_return': 10.0, - 'number_of_trades': 2, - 'win_rate': 100.0, - }, + trades=trades, + stats=stats, run_duration=1.0, ) @@ -148,15 +155,8 @@ class TestBacktestDeterminism: initial_capital=10000.0, final_value=11000.0, equity_curve=[10000, 10500, 11000], - trades=[ - {'ref': 1, 'pnl': 500, 'side': 'buy'}, - {'ref': 2, 'pnl': 500, 'side': 'sell'}, - ], - stats={ - 'total_return': 10.0, - 'number_of_trades': 2, - 'win_rate': 100.0, - }, + trades=trades, # Same trades + stats=stats, # Same stats run_duration=2.0, # Different runtime ) @@ -173,7 +173,7 @@ class TestBacktestDeterminism: initial_capital=10000.0, final_value=11000.0, equity_curve=[10000, 10500, 11000], - trades=[{'pnl': 1000}], + trades=[{'pnl': 1000, 'size': 0.1, 'open_price': 50000, 'close_price': 60000}], stats={'total_return': 10.0, 'win_rate': 100.0, 'number_of_trades': 1}, run_duration=1.0, ) @@ -186,15 +186,55 @@ class TestBacktestDeterminism: initial_capital=10000.0, final_value=10500.0, # Different final value equity_curve=[10000, 10250, 10500], # Different curve - trades=[{'pnl': 500}], # Different PnL + trades=[{'pnl': 500, 'size': 0.1, 'open_price': 50000, 'close_price': 55000}], # Different stats={'total_return': 5.0, 'win_rate': 100.0, 'number_of_trades': 1}, run_duration=1.0, ) assert result1.get_determinism_hash() != result2.get_determinism_hash() + def test_different_trade_sequence_different_hash(self): + """Test that different trade sequences produce different hashes even with same totals.""" + # Two trades: +500, +500 = same total as +200, +800 + result1 = create_backtest_result( + strategy_id='strategy-abc', + strategy_name='Test', + user_id=1, + backtest_id='bt-001', + initial_capital=10000.0, + final_value=11000.0, + equity_curve=[10000, 10500, 11000], + trades=[ + {'pnl': 500, 'size': 0.1, 'open_price': 50000, 'close_price': 55000}, + {'pnl': 500, 'size': 0.1, 'open_price': 55000, 'close_price': 60000}, + ], + stats={'total_return': 10.0, 'win_rate': 100.0, 'number_of_trades': 2}, + run_duration=1.0, + ) + + result2 = create_backtest_result( + strategy_id='strategy-abc', + strategy_name='Test', + user_id=1, + backtest_id='bt-001', + initial_capital=10000.0, + final_value=11000.0, # Same final value + equity_curve=[10000, 10200, 11000], # Different curve + trades=[ + {'pnl': 200, 'size': 0.1, 'open_price': 50000, 'close_price': 52000}, # Different + {'pnl': 800, 'size': 0.1, 'open_price': 52000, 'close_price': 60000}, # Different + ], + stats={'total_return': 10.0, 'win_rate': 100.0, 'number_of_trades': 2}, # Same totals + run_duration=1.0, + ) + + # Should be different even though total PnL is the same + assert result1.get_determinism_hash() != result2.get_determinism_hash() + def test_verify_determinism(self): """Test the verify_determinism method.""" + stats = {'total_return': 5.0, 'number_of_trades': 0, 'win_rate': 0.0, 'sharpe_ratio': 0.0, 'max_drawdown': 0.0, 'profit_factor': 0.0} + result1 = create_backtest_result( strategy_id='strategy-1', strategy_name='Test', @@ -204,7 +244,7 @@ class TestBacktestDeterminism: final_value=10500, equity_curve=[10000, 10250, 10500], trades=[], - stats={'total_return': 5.0, 'number_of_trades': 0, 'win_rate': 0.0}, + stats=stats, run_duration=1.0, ) @@ -218,7 +258,7 @@ class TestBacktestDeterminism: final_value=10500, equity_curve=[10000, 10250, 10500], trades=[], - stats={'total_return': 5.0, 'number_of_trades': 0, 'win_rate': 0.0}, + stats=stats, run_duration=2.0, ) @@ -226,7 +266,9 @@ class TestBacktestDeterminism: def test_floating_point_precision(self): """Test that floating point precision doesn't break determinism.""" - # Results with slightly different floating point representations + trades = [{'pnl': 500.123456, 'size': 0.123456, 'open_price': 50000.123, 'close_price': 55000.456}] + stats = {'total_return': 5.001234, 'number_of_trades': 1, 'win_rate': 100.0, 'sharpe_ratio': 1.234, 'max_drawdown': -0.5, 'profit_factor': 2.5} + result1 = create_backtest_result( strategy_id='strategy-1', strategy_name='Test', @@ -235,8 +277,8 @@ class TestBacktestDeterminism: initial_capital=10000.0, final_value=10500.123456, equity_curve=[10000.0, 10500.123456], - trades=[{'pnl': 500.123456}], - stats={'total_return': 5.001234, 'number_of_trades': 1, 'win_rate': 100.0}, + trades=trades, + stats=stats, run_duration=1.0, ) @@ -248,8 +290,8 @@ class TestBacktestDeterminism: initial_capital=10000.0, final_value=10500.123456, equity_curve=[10000.0, 10500.123456], - trades=[{'pnl': 500.123456}], - stats={'total_return': 5.001234, 'number_of_trades': 1, 'win_rate': 100.0}, + trades=trades, + stats=stats, run_duration=1.0, ) diff --git a/tests/test_brokers.py b/tests/test_brokers.py index a21daef..fa4c23d 100644 --- a/tests/test_brokers.py +++ b/tests/test_brokers.py @@ -164,6 +164,36 @@ class TestPaperBroker: # Unrealized P&L: (52000 - 50000) * 0.1 = 200 assert position.unrealized_pnl == 200 + def test_paper_broker_equity_calculation(self): + """Test that equity = cash + position value (not just unrealized PnL).""" + broker = PaperBroker(initial_balance=10000, commission=0, slippage=0) + broker.update_price('BTC/USDT', 100) + + # Initial equity should equal initial balance + assert broker.get_balance() == 10000 + + # Buy 1 unit at $100 = spend $100 + broker.place_order( + symbol='BTC/USDT', + side=OrderSide.BUY, + order_type=OrderType.MARKET, + size=1.0 + ) + + # Cash is now 9900, position value is 100 + # Total equity should still be 10000 (9900 + 100) + assert broker.get_available_balance() == 9900 # Cash + assert broker.get_balance() == 10000 # Total equity + + # Price doubles + broker.update_price('BTC/USDT', 200) + broker.update() + + # Cash still 9900, position value now 200 + # Total equity should be 10100 (9900 + 200) + assert broker.get_available_balance() == 9900 + assert broker.get_balance() == 10100 + def test_paper_broker_reset(self): """Test broker reset.""" broker = PaperBroker(initial_balance=10000) @@ -202,10 +232,12 @@ class TestBrokerFactory: ) assert isinstance(broker, BacktestBroker) - def test_create_live_broker_not_implemented(self): - """Test that live broker raises NotImplementedError.""" - with pytest.raises(NotImplementedError): - create_broker(mode=TradingMode.LIVE) + def test_create_live_broker_falls_back_to_paper(self): + """Test that live broker falls back to paper broker with warning.""" + broker = create_broker(mode=TradingMode.LIVE, initial_balance=5000) + # Should return a PaperBroker (fallback) + assert isinstance(broker, PaperBroker) + assert broker.get_balance() == 5000 def test_invalid_mode(self): """Test that invalid mode raises ValueError."""