diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 5c2b2cd..e0a53b4 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -2258,6 +2258,63 @@ trade.realizedPnL += actualRealizedPnL // NOT: result.realizedPnL from SDK - **Files changed:** `lib/trading/position-manager.ts` (interface + logic) - **Lesson:** When introducing wait periods in financial systems, always add flags to prevent duplicate state updates during the wait +49. **P&L exponential compounding in external closure detection (CRITICAL - Fixed Nov 17, 2025):** + - **Symptom:** Database P&L shows 15-20× actual value (e.g., $92.46 when Drift shows $6.00) + - **Root Cause:** `trade.realizedPnL` was being mutated during each external closure detection cycle + - **Real incident (Nov 17, 13:54 CET):** + * SOL-PERP SHORT closed by on-chain orders: 1.54 SOL at -1.95% + 2.3 SOL at -0.57% + * Actual P&L from Drift: ~$6.00 profit + * Database recorded: $92.46 profit (15.4× too high) + * Rate limiting caused 15+ detection cycles before trade removal + * Each cycle compounded: $6 → $12 → $24 → $48 → $96 + - **Bug mechanism (line 799 in position-manager.ts):** + ```typescript + // BROKEN CODE: + const previouslyRealized = trade.realizedPnL // Gets from mutated in-memory object + const totalRealizedPnL = previouslyRealized + runnerRealized + trade.realizedPnL = totalRealizedPnL // ← BUG: Mutates in-memory trade object + + // Next monitoring cycle (2 seconds later): + const previouslyRealized = trade.realizedPnL // ← Gets ACCUMULATED value from previous cycle + const totalRealizedPnL = previouslyRealized + runnerRealized // ← Adds it AGAIN + trade.realizedPnL = totalRealizedPnL // ← Compounds further + // Repeats 15-20 times before activeTrades.delete() removes trade + ``` + - **Why Common Pitfall #48 didn't prevent this:** + * `closingInProgress` flag only applies when Position Manager initiates the close + * External closures (on-chain TP/SL orders) don't set this flag + * External closure detection runs in monitoring loop WITHOUT closingInProgress protection + * Rate limiting delays cause monitoring loop to detect closure multiple times + - **Fix:** + ```typescript + // CORRECT CODE (line 798): + const previouslyRealized = trade.realizedPnL // Get original value from DB + const totalRealizedPnL = previouslyRealized + runnerRealized + // DON'T mutate trade.realizedPnL here - causes compounding on re-detection! + // trade.realizedPnL = totalRealizedPnL ← REMOVED + console.log(` Realized P&L calculation → Previous: $${previouslyRealized.toFixed(2)} | Runner: $${runnerRealized.toFixed(2)} ... | Total: $${totalRealizedPnL.toFixed(2)}`) + + // Later in same function (line 850): + await updateTradeExit({ + realizedPnL: totalRealizedPnL, // Use local variable for DB update + // ... other fields + }) + ``` + - **Impact:** Every external closure (on-chain TP/SL fills) affected, especially with rate limiting + - **Database correction:** Manual UPDATE required for trades with inflated P&L + - **Verification:** Check that updateTradeExit uses `totalRealizedPnL` (local variable) not `trade.realizedPnL` (mutated field) + - **Why activeTrades.delete() before DB update didn't help:** + * That fix (Common Pitfall #27) prevents duplicates AFTER database update completes + * But external closure detection calculates P&L BEFORE calling activeTrades.delete() + * If rate limits delay the detection→delete cycle, monitoring loop runs detection multiple times + * Each time, it mutates trade.realizedPnL before checking if trade already removed + - **Git commit:** 6156c0f "critical: Fix P&L compounding bug in external closure detection" + - **Related bugs:** + * Common Pitfall #27: Duplicate external closure updates (fixed by delete before DB update) + * Common Pitfall #48: P&L compounding during close verification (fixed by closingInProgress flag) + * This bug (#49): P&L compounding in external closure detection (fixed by not mutating trade.realizedPnL) + - **Lesson:** In monitoring loops that run repeatedly, NEVER mutate shared state during calculation phases. Calculate locally, update shared state ONCE at the end. Immutability prevents compounding bugs in retry/race scenarios. + 46. **100% position sizing causes InsufficientCollateral (Fixed Nov 16, 2025):** - **Symptom:** Bot configured for 100% position size gets InsufficientCollateral errors, but Drift UI can open same size position - **Root Cause:** Drift's margin calculation includes fees, slippage buffers, and rounding - exact 100% leaves no room