Fix critical Phase 4 issues: wire SL/TP to broker, reconcile trades, fix schema
High priority fixes: - Wire time_in_force, stop_loss, take_profit to broker.place_order() * Pass time_in_force from received_new_trade() to new_trade() * Pass SL/TP/TIF from new_trade() to broker.place_order() - Reconcile Trade ledger when SL/TP triggers * Find all matching paper trades for symbol * Settle trades at trigger price * Move from active_trades to settled_trades * Save to database - Fix fresh schema missing SL/TP columns * Add stop_loss and take_profit to CREATE TABLE statement * Ensures first-run trade persistence works Medium priority fixes: - Hide SL/TP fields for SELL orders (inventory-only model) * SL/TP only makes sense for BUY (opening positions) * SELL closes existing positions, no SL/TP needed * Added _updateSltpVisibility() method * Side change listener shows/hides SL/TP row * Removed SELL-side SL/TP validation Tests: - Added 2 integration tests for manual trading SL/TP path - 353 tests pass (4 pre-existing failures unrelated) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
4bfa51f0b1
commit
5866319b5e
|
|
@ -1420,7 +1420,8 @@ class BrighterTrades:
|
|||
strategy_id=strategy_id,
|
||||
testnet=testnet,
|
||||
stop_loss=stop_loss,
|
||||
take_profit=take_profit
|
||||
take_profit=take_profit,
|
||||
time_in_force=time_in_force
|
||||
)
|
||||
|
||||
if status == 'Error':
|
||||
|
|
|
|||
29
src/app.py
29
src/app.py
|
|
@ -221,30 +221,43 @@ def strategy_execution_loop():
|
|||
event_type = event.get('type', 'fill')
|
||||
|
||||
if event_type == 'sltp_triggered':
|
||||
# SL/TP triggered - find related trade and notify user
|
||||
# SL/TP triggered - find and settle related trades
|
||||
symbol = event.get('symbol')
|
||||
trigger_price = event.get('trigger_price', 0)
|
||||
user_id = event.get('user_id')
|
||||
|
||||
# Find trades for this symbol to get the user
|
||||
related_trade = None
|
||||
for trade in brighter_trades.trades.active_trades.values():
|
||||
# Find ALL matching paper trades for this symbol and settle them
|
||||
trades_to_settle = []
|
||||
for trade in list(brighter_trades.trades.active_trades.values()):
|
||||
if trade.symbol == symbol and (trade.is_paper or trade.broker_kind == 'paper'):
|
||||
related_trade = trade
|
||||
trades_to_settle.append(trade)
|
||||
user_id = user_id or trade.creator
|
||||
break
|
||||
|
||||
# Settle each matching trade
|
||||
for trade in trades_to_settle:
|
||||
# Settle the trade at the trigger price
|
||||
trade.settle(qty=trade.stats.get('qty_filled', trade.base_order_qty), price=trigger_price)
|
||||
# Move from active to settled
|
||||
if trade.unique_id in brighter_trades.trades.active_trades:
|
||||
del brighter_trades.trades.active_trades[trade.unique_id]
|
||||
brighter_trades.trades.settled_trades[trade.unique_id] = trade
|
||||
brighter_trades.trades._save_trade(trade)
|
||||
_loop_debug.debug(f"Settled trade {trade.unique_id} via SL/TP at {trigger_price}")
|
||||
|
||||
# Notify user
|
||||
if user_id:
|
||||
user_name = brighter_trades.users.get_username(user_id=user_id)
|
||||
if user_name:
|
||||
trade_ids = [t.unique_id for t in trades_to_settle]
|
||||
socketio.emit('message', {
|
||||
'reply': 'sltp_triggered',
|
||||
'data': sanitize_for_json({
|
||||
'trigger': event.get('trigger'),
|
||||
'symbol': symbol,
|
||||
'trigger_price': event.get('trigger_price'),
|
||||
'trigger_price': trigger_price,
|
||||
'size': event.get('size'),
|
||||
'pnl': event.get('pnl'),
|
||||
'trade_id': related_trade.unique_id if related_trade else None
|
||||
'trade_ids': trade_ids
|
||||
})
|
||||
}, room=user_name)
|
||||
_loop_debug.debug(f"Emitted sltp_triggered to room={user_name}")
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ class TradeUIManager {
|
|||
this.takeProfitInput = null;
|
||||
this.timeInForceSelect = null;
|
||||
this.exchangeRow = null;
|
||||
this.sltpRow = null;
|
||||
this.onCloseTrade = null;
|
||||
|
||||
// Exchanges known to support testnet/sandbox mode
|
||||
|
|
@ -58,7 +59,8 @@ class TradeUIManager {
|
|||
stopLossId = 'stopLoss',
|
||||
takeProfitId = 'takeProfit',
|
||||
timeInForceId = 'timeInForce',
|
||||
exchangeRowId = 'exchange-row'
|
||||
exchangeRowId = 'exchange-row',
|
||||
sltpRowId = 'sltp-row'
|
||||
} = config;
|
||||
|
||||
this.targetEl = document.getElementById(targetId);
|
||||
|
|
@ -86,6 +88,7 @@ class TradeUIManager {
|
|||
this.takeProfitInput = document.getElementById(takeProfitId);
|
||||
this.timeInForceSelect = document.getElementById(timeInForceId);
|
||||
this.exchangeRow = document.getElementById(exchangeRowId);
|
||||
this.sltpRow = document.getElementById(sltpRowId);
|
||||
|
||||
// Set up event listeners
|
||||
this._setupFormListeners();
|
||||
|
|
@ -162,6 +165,13 @@ class TradeUIManager {
|
|||
this._updateSellAvailability();
|
||||
});
|
||||
}
|
||||
|
||||
// Side changes affect SL/TP visibility (not applicable for SELL/close)
|
||||
if (this.sideSelect) {
|
||||
this.sideSelect.addEventListener('change', () => {
|
||||
this._updateSltpVisibility();
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -321,6 +331,28 @@ class TradeUIManager {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Updates SL/TP row visibility based on order side.
|
||||
* SL/TP only applies to BUY orders (opening positions).
|
||||
* SELL orders close existing positions, so SL/TP is not applicable.
|
||||
*/
|
||||
_updateSltpVisibility() {
|
||||
if (!this.sltpRow || !this.sideSelect) return;
|
||||
|
||||
const side = this.sideSelect.value.toLowerCase();
|
||||
|
||||
if (side === 'sell') {
|
||||
// Hide SL/TP for SELL (closing positions)
|
||||
this.sltpRow.style.display = 'none';
|
||||
// Clear any values
|
||||
if (this.stopLossInput) this.stopLossInput.value = '';
|
||||
if (this.takeProfitInput) this.takeProfitInput.value = '';
|
||||
} else {
|
||||
// Show SL/TP for BUY (opening positions)
|
||||
this.sltpRow.style.display = 'contents';
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Populates the exchange selector with connected exchanges.
|
||||
* @param {string[]} connectedExchanges - List of connected exchange names.
|
||||
|
|
@ -441,6 +473,13 @@ class TradeUIManager {
|
|||
if (this.testnetCheckbox) {
|
||||
this.testnetCheckbox.checked = true;
|
||||
}
|
||||
// Reset side to BUY and show SL/TP row
|
||||
if (this.sideSelect) {
|
||||
this.sideSelect.value = 'buy';
|
||||
}
|
||||
if (this.sltpRow) {
|
||||
this.sltpRow.style.display = 'contents';
|
||||
}
|
||||
|
||||
this.formElement.style.display = 'grid';
|
||||
|
||||
|
|
@ -1154,6 +1193,7 @@ class TradeUIManager {
|
|||
// If SELL is currently selected but no longer valid, reset to BUY
|
||||
if (!hasPosition && this.sideSelect.value === 'SELL') {
|
||||
this.sideSelect.value = 'BUY';
|
||||
this._updateSltpVisibility();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -1633,7 +1673,7 @@ class Trade {
|
|||
return;
|
||||
}
|
||||
|
||||
// SL/TP validation
|
||||
// SL/TP validation (only for BUY orders - SELL closes existing positions)
|
||||
if (side.toUpperCase() === 'BUY') {
|
||||
if (stopLoss && stopLoss >= price) {
|
||||
alert('Stop Loss must be below entry price for BUY orders.');
|
||||
|
|
@ -1643,17 +1683,8 @@ class Trade {
|
|||
alert('Take Profit must be above entry price for BUY orders.');
|
||||
return;
|
||||
}
|
||||
} else {
|
||||
// SELL
|
||||
if (stopLoss && stopLoss <= price) {
|
||||
alert('Stop Loss must be above entry price for SELL orders.');
|
||||
return;
|
||||
}
|
||||
if (takeProfit && takeProfit >= price) {
|
||||
alert('Take Profit must be below entry price for SELL orders.');
|
||||
return;
|
||||
}
|
||||
}
|
||||
// Note: SL/TP fields are hidden for SELL orders (inventory-only model)
|
||||
|
||||
// Show confirmation for production live trades
|
||||
if (!isPaperTrade && !testnet) {
|
||||
|
|
|
|||
|
|
@ -74,15 +74,16 @@
|
|||
<label for="tradeValue"><b>Est. Value:</b></label>
|
||||
<output name="tradeValue" id="tradeValue" for="quantity price">0</output>
|
||||
|
||||
<!-- Stop Loss (optional) -->
|
||||
<label for="stopLoss"><b>Stop Loss:</b></label>
|
||||
<input type="number" min="0" step="0.00000001" name="stopLoss" id="stopLoss"
|
||||
placeholder="Optional - triggers auto-close" style="width: 100%;">
|
||||
<!-- Stop Loss / Take Profit (only for BUY orders - not applicable for closing positions) -->
|
||||
<div id="sltp-row" style="display: contents;">
|
||||
<label for="stopLoss"><b>Stop Loss:</b></label>
|
||||
<input type="number" min="0" step="0.00000001" name="stopLoss" id="stopLoss"
|
||||
placeholder="Optional - triggers auto-close" style="width: 100%;">
|
||||
|
||||
<!-- Take Profit (optional) -->
|
||||
<label for="takeProfit"><b>Take Profit:</b></label>
|
||||
<input type="number" min="0" step="0.00000001" name="takeProfit" id="takeProfit"
|
||||
placeholder="Optional - triggers auto-close" style="width: 100%;">
|
||||
<label for="takeProfit"><b>Take Profit:</b></label>
|
||||
<input type="number" min="0" step="0.00000001" name="takeProfit" id="takeProfit"
|
||||
placeholder="Optional - triggers auto-close" style="width: 100%;">
|
||||
</div>
|
||||
|
||||
<!-- Time in Force -->
|
||||
<label for="timeInForce"><b>Time in Force:</b></label>
|
||||
|
|
|
|||
15
src/trade.py
15
src/trade.py
|
|
@ -289,7 +289,9 @@ class Trades:
|
|||
broker_mode TEXT,
|
||||
broker_exchange TEXT,
|
||||
broker_order_id TEXT,
|
||||
exchange_order_id TEXT
|
||||
exchange_order_id TEXT,
|
||||
stop_loss REAL,
|
||||
take_profit REAL
|
||||
)
|
||||
"""
|
||||
self.data_cache.db.execute_sql(create_sql, params=[])
|
||||
|
|
@ -551,7 +553,8 @@ class Trades:
|
|||
def new_trade(self, target: str, symbol: str, price: float, side: str,
|
||||
order_type: str, qty: float, user_id: int = None,
|
||||
strategy_id: str = None, testnet: bool = False, exchange: str = None,
|
||||
stop_loss: float = None, take_profit: float = None) -> tuple[str, str | None]:
|
||||
stop_loss: float = None, take_profit: float = None,
|
||||
time_in_force: str = 'GTC') -> tuple[str, str | None]:
|
||||
"""
|
||||
Creates a new trade (paper or live).
|
||||
|
||||
|
|
@ -565,6 +568,9 @@ class Trades:
|
|||
:param strategy_id: Optional strategy ID if from a strategy.
|
||||
:param testnet: Whether to use testnet/sandbox mode for live trades.
|
||||
:param exchange: The actual exchange for price data (for paper trades).
|
||||
:param stop_loss: Optional stop loss price.
|
||||
:param take_profit: Optional take profit price.
|
||||
:param time_in_force: Order time-in-force ('GTC', 'IOC', 'FOK').
|
||||
:return: Tuple of (status, trade_id or error message).
|
||||
"""
|
||||
from brokers.base_broker import OrderSide, OrderType, OrderStatus
|
||||
|
|
@ -666,7 +672,10 @@ class Trades:
|
|||
side=order_side,
|
||||
order_type=order_type_enum,
|
||||
size=float(qty),
|
||||
price=effective_price if order_type.upper() == 'LIMIT' else None
|
||||
price=effective_price if order_type.upper() == 'LIMIT' else None,
|
||||
stop_loss=stop_loss,
|
||||
take_profit=take_profit,
|
||||
time_in_force=time_in_force
|
||||
)
|
||||
|
||||
if not result.success:
|
||||
|
|
|
|||
|
|
@ -508,3 +508,96 @@ class TestPaperBrokerSLTP:
|
|||
position = broker.get_position('BTC/USDT')
|
||||
assert position is not None
|
||||
assert position.size == 0.1
|
||||
|
||||
|
||||
class TestManualTradingSLTP:
|
||||
"""Integration tests for SL/TP in manual trading path (Trades.new_trade -> broker)."""
|
||||
|
||||
@pytest.fixture
|
||||
def mock_users(self):
|
||||
"""Create a mock Users object."""
|
||||
from unittest.mock import MagicMock
|
||||
users = MagicMock()
|
||||
users.get_username.return_value = 'test_user'
|
||||
return users
|
||||
|
||||
def test_new_trade_passes_sltp_to_broker(self, mock_users):
|
||||
"""Test that new_trade() passes SL/TP to broker.place_order()."""
|
||||
from manual_trading_broker import ManualTradingBrokerManager
|
||||
|
||||
trades = Trades(mock_users)
|
||||
|
||||
# Set up manual broker manager
|
||||
broker_manager = ManualTradingBrokerManager()
|
||||
trades.manual_broker_manager = broker_manager
|
||||
|
||||
# Get the paper broker and set price
|
||||
broker = broker_manager.get_paper_broker(user_id=1)
|
||||
broker.update_price('BTC/USDT', 50000)
|
||||
|
||||
# Create trade with SL/TP
|
||||
status, trade_id = trades.new_trade(
|
||||
target='test_exchange',
|
||||
symbol='BTC/USDT',
|
||||
price=50000.0,
|
||||
side='buy',
|
||||
order_type='MARKET',
|
||||
qty=0.1,
|
||||
user_id=1,
|
||||
stop_loss=45000.0,
|
||||
take_profit=60000.0
|
||||
)
|
||||
|
||||
assert status == 'Success'
|
||||
|
||||
# Verify trade has SL/TP
|
||||
trade = trades.get_trade_by_id(trade_id)
|
||||
assert trade.stop_loss == 45000.0
|
||||
assert trade.take_profit == 60000.0
|
||||
|
||||
# Verify broker has SL/TP tracking
|
||||
assert 'BTC/USDT' in broker._position_sltp
|
||||
assert broker._position_sltp['BTC/USDT']['stop_loss'] == 45000.0
|
||||
assert broker._position_sltp['BTC/USDT']['take_profit'] == 60000.0
|
||||
|
||||
def test_new_trade_sltp_triggers_on_price_drop(self, mock_users):
|
||||
"""Test that SL/TP triggers work through the full manual trading path."""
|
||||
from manual_trading_broker import ManualTradingBrokerManager
|
||||
|
||||
trades = Trades(mock_users)
|
||||
|
||||
# Set up manual broker manager
|
||||
broker_manager = ManualTradingBrokerManager()
|
||||
trades.manual_broker_manager = broker_manager
|
||||
|
||||
# Get the paper broker and set price
|
||||
broker = broker_manager.get_paper_broker(user_id=1)
|
||||
broker.update_price('BTC/USDT', 50000)
|
||||
|
||||
# Create trade with SL
|
||||
status, trade_id = trades.new_trade(
|
||||
target='test_exchange',
|
||||
symbol='BTC/USDT',
|
||||
price=50000.0,
|
||||
side='buy',
|
||||
order_type='MARKET',
|
||||
qty=0.1,
|
||||
user_id=1,
|
||||
stop_loss=45000.0
|
||||
)
|
||||
|
||||
assert status == 'Success'
|
||||
assert trade_id in trades.active_trades
|
||||
|
||||
# Price drops below SL
|
||||
broker.update_price('BTC/USDT', 44000)
|
||||
events = broker.update()
|
||||
|
||||
# Verify SL triggered
|
||||
sltp_events = [e for e in events if e.get('type') == 'sltp_triggered']
|
||||
assert len(sltp_events) == 1
|
||||
assert sltp_events[0]['trigger'] == 'stop_loss'
|
||||
|
||||
# Position should be closed at broker level
|
||||
position = broker.get_position('BTC/USDT')
|
||||
assert position is None
|
||||
|
|
|
|||
Loading…
Reference in New Issue