fix(fx): correct polarity inversion for EUR.XXX CASH trade pairs#176
Conversation
For EUR.USD (and other EUR-base pairs), IBKR's quantity field is in EUR (the base currency), not in trade.currency (the quote). Additionally, SELL means selling EUR (acquiring the quote), not disposing it. The fix detects EUR.XXX pairs via symbol/description prefix and: - Uses tradeMoney (the actual quote-currency amount) instead of quantity - Inverts polarity: SELL = acquiring, BUY = disposing Non-EUR pairs continue using the original quantity-based logic. Closes #176
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 17 minutes and 33 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR corrects FX event extraction for EUR-base currency pairs by detecting EUR.xxx symbols and sourcing quantities from tradeMoney with inverted polarity, while keeping non-EUR pairs on the original quantity-based logic. Event emission is reshaped to use a derived acquiring boolean, pushing signed quantities for acquisitions and disposals. ChangesEUR.xxx FX event extraction polarity fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
=======================================
Coverage 97.56% 97.57%
=======================================
Files 38 38
Lines 7764 7780 +16
Branches 1588 1596 +8
=======================================
+ Hits 7575 7591 +16
Misses 188 188
Partials 1 1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/engine/fx-fifo.test.ts (1)
202-216: ⚡ Quick winAdd a regression case for description-driven EUR detection.
Please add one case where
symbolis non-EUR (or empty) anddescriptionstarts withEUR.to lock the intended “symbol/description” detection behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/engine/fx-fifo.test.ts` around lines 202 - 216, Add a regression test where symbol is non-EUR (or empty) but description starts with "EUR." to ensure description-driven EUR detection still works: in the tests/engine fx-fifo.test.ts add an it() case (similar to the existing "non-EUR pair" tests) that constructs a trade via makeTrade with symbol "USD.JPY" (or empty) and description "EUR.XXX" and passes it to FxFifoEngine.extractFxEvents using rateMap, then assert events length is 1 and that the resulting event reflects EUR detection (e.g., correct sign/quantity handling as in the BUY/SELL cases); reference makeTrade and FxFifoEngine.extractFxEvents to locate where to add the case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engine/fx-fifo.ts`:
- Line 105: The current isEurBase predicate uses (trade.symbol ||
trade.description || "").toUpperCase().startsWith("EUR.") which short-circuits
on a non-EUR symbol and misses an EUR-prefixed description; update the check in
isEurBase to test both fields individually (e.g., check (trade.symbol || "") and
(trade.description || "") separately, .toUpperCase(), and OR their
.startsWith("EUR.") results) so either field can detect an EUR-base pair while
safely handling undefined values.
---
Nitpick comments:
In `@tests/engine/fx-fifo.test.ts`:
- Around line 202-216: Add a regression test where symbol is non-EUR (or empty)
but description starts with "EUR." to ensure description-driven EUR detection
still works: in the tests/engine fx-fifo.test.ts add an it() case (similar to
the existing "non-EUR pair" tests) that constructs a trade via makeTrade with
symbol "USD.JPY" (or empty) and description "EUR.XXX" and passes it to
FxFifoEngine.extractFxEvents using rateMap, then assert events length is 1 and
that the resulting event reflects EUR detection (e.g., correct sign/quantity
handling as in the BUY/SELL cases); reference makeTrade and
FxFifoEngine.extractFxEvents to locate where to add the case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a67ead75-a602-432a-9b79-8ba2b6f0b2f9
📒 Files selected for processing (3)
src/engine/fx-fifo.tstests/engine/fx-fifo.test.tstests/generators/report.test.ts
|
Estaba preparando un PR sobre este tema pero estaba pendiente del anterior. Me gustaría proponer una solución que a mi entender sería más limpia. Creo que sería positivo que las correcciones debidas a paricularidades del broker no deberían complicar la mecánica del engine. Creo que deberían ser resueltas en el propio parser y entregar una estructura de datos idéntica para todos. Tengo ensayado esto y está funcionando |
Extracts isCurrencyQuote() to detect when trade.currency matches the quote side of any pair (not just EUR.XXX). Fixes the same latent bug for cross-rate pairs like GBP.USD with currency=USD. Updates JSDoc, adds EUR.GBP and GBP.USD cross-rate tests, ensures tradeMoney differs from quantity in test assertions to prove correct field is read.
|
Gracias por la propuesta, @elmasvital. La idea de normalizar en el parser es atractiva conceptualmente, pero tiene algunos problemas prácticos: 1. Solo cubre una dirección. Tu código fuerza 2. 3. La solución que implementamos en v0.39.5 es genérica: detecta cuándo La diferencia con Lightyear (que sí normaliza en el parser) es que el CSV de Lightyear ya proporciona importes explícitos por divisa con signo correcto. IBKR no — hay que interpretar la semántica BASE.QUOTE, que es lógica del dominio FX y por eso vive en el engine. Sobre "completamente roto con mis datos": v0.39.5 corrige exactamente el bug de polaridad que afectaba a tu conversión manual del 28/03 ( |
2 participants
Summary
quantityfield is in EUR (the base currency), not intrade.currency(the quote). Additionally,SELLmeans selling EUR (acquiring the quote), not disposing it.tradeMoney(the actual quote-currency amount) with inverted polarityRoot cause
IBKR Flex Query uses forex market convention: for
EUR.USD,quantity= EUR amount (base),tradeMoney= USD amount (quote).SELL EUR.USD= selling EUR = acquiring USD. The code was treating SELL as disposing USD and using the EUR quantity as if it were USD.This was masked because all test fixtures used AFx-noted trades (which are filtered out by
isFxconv()), so no non-AFx EUR.XXX pair was ever exercised.Impact
Users with manual (non-AFx) EUR.XXX CASH conversions were getting inverted FX events: acquisitions recorded as disposals and vice versa, with wrong amounts (EUR instead of USD). This caused phantom disposals from nonexistent lots.
Reported by @elmasvital — his PR #175 identified the symptom but proposed the wrong fix (wholesale revert of v0.39.2). This is the targeted correction.
Test plan
Closes #176
Summary by CodeRabbit