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 <noreply@anthropic.com>
This commit is contained in:
parent
5dbda13924
commit
b555f6e004
|
|
@ -352,13 +352,24 @@ class ExchangeInterface:
|
||||||
"""
|
"""
|
||||||
Get the executed price of a trade order.
|
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.
|
:param trade: The trade object.
|
||||||
:return: Executed price.
|
:return: Executed price.
|
||||||
"""
|
"""
|
||||||
# For paper trading / backtesting, use order price
|
# For paper trading / backtesting, use order price if set
|
||||||
return trade.order_price
|
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:
|
def get_user_balance(self, user_id: int) -> float:
|
||||||
"""
|
"""
|
||||||
|
|
|
||||||
|
|
@ -117,21 +117,36 @@ class BacktestResult:
|
||||||
Generate a hash of the deterministic parts of the result.
|
Generate a hash of the deterministic parts of the result.
|
||||||
|
|
||||||
Excludes non-deterministic fields like run_datetime and run_duration.
|
Excludes non-deterministic fields like run_datetime and run_duration.
|
||||||
|
Includes full trade sequence to detect different trade distributions.
|
||||||
"""
|
"""
|
||||||
import hashlib
|
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 = {
|
deterministic_data = {
|
||||||
'strategy_id': self.strategy_id,
|
'strategy_id': self.strategy_id,
|
||||||
'initial_capital': self.initial_capital,
|
'initial_capital': self.initial_capital,
|
||||||
'final_portfolio_value': round(self.final_portfolio_value, 6),
|
'final_portfolio_value': round(self.final_portfolio_value, 6),
|
||||||
'equity_curve': [round(e, 6) for e in self.equity_curve],
|
'equity_curve': [round(e, 6) for e in self.equity_curve],
|
||||||
'trades_count': len(self.trades),
|
'trades': normalized_trades, # Full trade sequence
|
||||||
'trades_pnl': sum(t.get('pnl', 0) for t in self.trades),
|
|
||||||
'metrics': {
|
'metrics': {
|
||||||
'total_return': round(self.metrics.total_return, 6),
|
'total_return': round(self.metrics.total_return, 6),
|
||||||
'number_of_trades': self.metrics.number_of_trades,
|
'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),
|
'win_rate': round(self.metrics.win_rate, 6),
|
||||||
|
'profit_factor': round(self.metrics.profit_factor, 6),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -71,10 +71,18 @@ def create_broker(
|
||||||
)
|
)
|
||||||
|
|
||||||
elif mode == TradingMode.LIVE:
|
elif mode == TradingMode.LIVE:
|
||||||
# Live broker will be implemented in Phase 5
|
# Live broker not yet implemented - fall back to paper trading with warning
|
||||||
raise NotImplementedError(
|
logger.warning(
|
||||||
"Live trading broker not yet implemented. "
|
"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:
|
else:
|
||||||
|
|
|
||||||
|
|
@ -144,8 +144,18 @@ class PaperBroker(BaseBroker):
|
||||||
message=f"Cannot get current price for {symbol}"
|
message=f"Cannot get current price for {symbol}"
|
||||||
)
|
)
|
||||||
|
|
||||||
# Calculate required margin/cost
|
# Calculate required margin/cost using the actual execution price
|
||||||
order_value = size * current_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)
|
required_funds = order_value * (1 + self.commission)
|
||||||
|
|
||||||
if side == OrderSide.BUY and required_funds > self._cash:
|
if side == OrderSide.BUY and required_funds > self._cash:
|
||||||
|
|
@ -306,10 +316,11 @@ class PaperBroker(BaseBroker):
|
||||||
return open_orders
|
return open_orders
|
||||||
|
|
||||||
def get_balance(self, asset: Optional[str] = None) -> float:
|
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
|
total = self._cash + self._locked_balance
|
||||||
for position in self._positions.values():
|
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
|
return total
|
||||||
|
|
||||||
def get_available_balance(self, asset: Optional[str] = None) -> float:
|
def get_available_balance(self, asset: Optional[str] = None) -> float:
|
||||||
|
|
|
||||||
|
|
@ -120,6 +120,20 @@ class TestBacktestDeterminism:
|
||||||
|
|
||||||
def test_same_inputs_same_hash(self):
|
def test_same_inputs_same_hash(self):
|
||||||
"""Test that identical inputs produce the same hash."""
|
"""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(
|
result1 = create_backtest_result(
|
||||||
strategy_id='strategy-abc',
|
strategy_id='strategy-abc',
|
||||||
strategy_name='Test Strategy',
|
strategy_name='Test Strategy',
|
||||||
|
|
@ -128,15 +142,8 @@ class TestBacktestDeterminism:
|
||||||
initial_capital=10000.0,
|
initial_capital=10000.0,
|
||||||
final_value=11000.0,
|
final_value=11000.0,
|
||||||
equity_curve=[10000, 10500, 11000],
|
equity_curve=[10000, 10500, 11000],
|
||||||
trades=[
|
trades=trades,
|
||||||
{'ref': 1, 'pnl': 500, 'side': 'buy'},
|
stats=stats,
|
||||||
{'ref': 2, 'pnl': 500, 'side': 'sell'},
|
|
||||||
],
|
|
||||||
stats={
|
|
||||||
'total_return': 10.0,
|
|
||||||
'number_of_trades': 2,
|
|
||||||
'win_rate': 100.0,
|
|
||||||
},
|
|
||||||
run_duration=1.0,
|
run_duration=1.0,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -148,15 +155,8 @@ class TestBacktestDeterminism:
|
||||||
initial_capital=10000.0,
|
initial_capital=10000.0,
|
||||||
final_value=11000.0,
|
final_value=11000.0,
|
||||||
equity_curve=[10000, 10500, 11000],
|
equity_curve=[10000, 10500, 11000],
|
||||||
trades=[
|
trades=trades, # Same trades
|
||||||
{'ref': 1, 'pnl': 500, 'side': 'buy'},
|
stats=stats, # Same stats
|
||||||
{'ref': 2, 'pnl': 500, 'side': 'sell'},
|
|
||||||
],
|
|
||||||
stats={
|
|
||||||
'total_return': 10.0,
|
|
||||||
'number_of_trades': 2,
|
|
||||||
'win_rate': 100.0,
|
|
||||||
},
|
|
||||||
run_duration=2.0, # Different runtime
|
run_duration=2.0, # Different runtime
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -173,7 +173,7 @@ class TestBacktestDeterminism:
|
||||||
initial_capital=10000.0,
|
initial_capital=10000.0,
|
||||||
final_value=11000.0,
|
final_value=11000.0,
|
||||||
equity_curve=[10000, 10500, 11000],
|
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},
|
stats={'total_return': 10.0, 'win_rate': 100.0, 'number_of_trades': 1},
|
||||||
run_duration=1.0,
|
run_duration=1.0,
|
||||||
)
|
)
|
||||||
|
|
@ -186,15 +186,55 @@ class TestBacktestDeterminism:
|
||||||
initial_capital=10000.0,
|
initial_capital=10000.0,
|
||||||
final_value=10500.0, # Different final value
|
final_value=10500.0, # Different final value
|
||||||
equity_curve=[10000, 10250, 10500], # Different curve
|
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},
|
stats={'total_return': 5.0, 'win_rate': 100.0, 'number_of_trades': 1},
|
||||||
run_duration=1.0,
|
run_duration=1.0,
|
||||||
)
|
)
|
||||||
|
|
||||||
assert result1.get_determinism_hash() != result2.get_determinism_hash()
|
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):
|
def test_verify_determinism(self):
|
||||||
"""Test the verify_determinism method."""
|
"""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(
|
result1 = create_backtest_result(
|
||||||
strategy_id='strategy-1',
|
strategy_id='strategy-1',
|
||||||
strategy_name='Test',
|
strategy_name='Test',
|
||||||
|
|
@ -204,7 +244,7 @@ class TestBacktestDeterminism:
|
||||||
final_value=10500,
|
final_value=10500,
|
||||||
equity_curve=[10000, 10250, 10500],
|
equity_curve=[10000, 10250, 10500],
|
||||||
trades=[],
|
trades=[],
|
||||||
stats={'total_return': 5.0, 'number_of_trades': 0, 'win_rate': 0.0},
|
stats=stats,
|
||||||
run_duration=1.0,
|
run_duration=1.0,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -218,7 +258,7 @@ class TestBacktestDeterminism:
|
||||||
final_value=10500,
|
final_value=10500,
|
||||||
equity_curve=[10000, 10250, 10500],
|
equity_curve=[10000, 10250, 10500],
|
||||||
trades=[],
|
trades=[],
|
||||||
stats={'total_return': 5.0, 'number_of_trades': 0, 'win_rate': 0.0},
|
stats=stats,
|
||||||
run_duration=2.0,
|
run_duration=2.0,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -226,7 +266,9 @@ class TestBacktestDeterminism:
|
||||||
|
|
||||||
def test_floating_point_precision(self):
|
def test_floating_point_precision(self):
|
||||||
"""Test that floating point precision doesn't break determinism."""
|
"""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(
|
result1 = create_backtest_result(
|
||||||
strategy_id='strategy-1',
|
strategy_id='strategy-1',
|
||||||
strategy_name='Test',
|
strategy_name='Test',
|
||||||
|
|
@ -235,8 +277,8 @@ class TestBacktestDeterminism:
|
||||||
initial_capital=10000.0,
|
initial_capital=10000.0,
|
||||||
final_value=10500.123456,
|
final_value=10500.123456,
|
||||||
equity_curve=[10000.0, 10500.123456],
|
equity_curve=[10000.0, 10500.123456],
|
||||||
trades=[{'pnl': 500.123456}],
|
trades=trades,
|
||||||
stats={'total_return': 5.001234, 'number_of_trades': 1, 'win_rate': 100.0},
|
stats=stats,
|
||||||
run_duration=1.0,
|
run_duration=1.0,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -248,8 +290,8 @@ class TestBacktestDeterminism:
|
||||||
initial_capital=10000.0,
|
initial_capital=10000.0,
|
||||||
final_value=10500.123456,
|
final_value=10500.123456,
|
||||||
equity_curve=[10000.0, 10500.123456],
|
equity_curve=[10000.0, 10500.123456],
|
||||||
trades=[{'pnl': 500.123456}],
|
trades=trades,
|
||||||
stats={'total_return': 5.001234, 'number_of_trades': 1, 'win_rate': 100.0},
|
stats=stats,
|
||||||
run_duration=1.0,
|
run_duration=1.0,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -164,6 +164,36 @@ class TestPaperBroker:
|
||||||
# Unrealized P&L: (52000 - 50000) * 0.1 = 200
|
# Unrealized P&L: (52000 - 50000) * 0.1 = 200
|
||||||
assert position.unrealized_pnl == 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):
|
def test_paper_broker_reset(self):
|
||||||
"""Test broker reset."""
|
"""Test broker reset."""
|
||||||
broker = PaperBroker(initial_balance=10000)
|
broker = PaperBroker(initial_balance=10000)
|
||||||
|
|
@ -202,10 +232,12 @@ class TestBrokerFactory:
|
||||||
)
|
)
|
||||||
assert isinstance(broker, BacktestBroker)
|
assert isinstance(broker, BacktestBroker)
|
||||||
|
|
||||||
def test_create_live_broker_not_implemented(self):
|
def test_create_live_broker_falls_back_to_paper(self):
|
||||||
"""Test that live broker raises NotImplementedError."""
|
"""Test that live broker falls back to paper broker with warning."""
|
||||||
with pytest.raises(NotImplementedError):
|
broker = create_broker(mode=TradingMode.LIVE, initial_balance=5000)
|
||||||
create_broker(mode=TradingMode.LIVE)
|
# Should return a PaperBroker (fallback)
|
||||||
|
assert isinstance(broker, PaperBroker)
|
||||||
|
assert broker.get_balance() == 5000
|
||||||
|
|
||||||
def test_invalid_mode(self):
|
def test_invalid_mode(self):
|
||||||
"""Test that invalid mode raises ValueError."""
|
"""Test that invalid mode raises ValueError."""
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue