FIX. Bug. STKs are not processed since version 0.39.2#175
Conversation
📝 WalkthroughWalkthrough
ChangesMulti-currency FX event handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
elmasvital
changed the title
![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
🤖 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`:
- Around line 134-139: The commission block uses trade.commissionCurrency
directly which can be undefined; create a fallback like const commCurrency =
trade.commissionCurrency || trade.currency and use commCurrency for the EUR
check, for the getEcbRate call (getEcbRate(rateMap, date, commCurrency)) and as
the event.currency so undefined is never passed; keep the existing commission
amount logic and only push the event when commCurrency !== "EUR".
🪄 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: 4a2f366d-3a27-462d-8086-23f44ad07f21
📒 Files selected for processing (2)
src/engine/fx-fifo.tssrc/types/tax.ts
| // Commission also consumes FCY (paid in commissionCurrency) | ||
| const commission = new Decimal(trade.commission).abs(); | ||
| if (commission.greaterThan(0) && trade.commissionCurrency !== "EUR") { | ||
| const commRate = getEcbRate(rateMap, date, trade.commissionCurrency); | ||
| events.push({ date, currency: trade.commissionCurrency, quantity: commission.negated(), ecbRate: commRate, trigger: "commission" }); | ||
| } |
There was a problem hiding this comment.
Missing fallback for commissionCurrency when undefined.
CASH trades at line 109 use trade.commissionCurrency || trade.currency as a fallback. Here, trade.commissionCurrency is used directly. If undefined, the condition !== "EUR" passes, then getEcbRate receives undefined and the event gets currency: undefined.
Proposed fix
// Commission also consumes FCY (paid in commissionCurrency)
const commission = new Decimal(trade.commission).abs();
- if (commission.greaterThan(0) && trade.commissionCurrency !== "EUR") {
- const commRate = getEcbRate(rateMap, date, trade.commissionCurrency);
- events.push({ date, currency: trade.commissionCurrency, quantity: commission.negated(), ecbRate: commRate, trigger: "commission" });
+ const commCcy = trade.commissionCurrency || trade.currency;
+ if (commission.greaterThan(0) && commCcy !== "EUR") {
+ const commRate = getEcbRate(rateMap, date, commCcy);
+ events.push({ date, currency: commCcy, quantity: commission.negated(), ecbRate: commRate, trigger: "commission" });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Commission also consumes FCY (paid in commissionCurrency) | |
| const commission = new Decimal(trade.commission).abs(); | |
| if (commission.greaterThan(0) && trade.commissionCurrency !== "EUR") { | |
| const commRate = getEcbRate(rateMap, date, trade.commissionCurrency); | |
| events.push({ date, currency: trade.commissionCurrency, quantity: commission.negated(), ecbRate: commRate, trigger: "commission" }); | |
| } | |
| // Commission also consumes FCY (paid in commissionCurrency) | |
| const commission = new Decimal(trade.commission).abs(); | |
| const commCcy = trade.commissionCurrency || trade.currency; | |
| if (commission.greaterThan(0) && commCcy !== "EUR") { | |
| const commRate = getEcbRate(rateMap, date, commCcy); | |
| events.push({ date, currency: commCcy, quantity: commission.negated(), ecbRate: commRate, trigger: "commission" }); | |
| } |
🤖 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 `@src/engine/fx-fifo.ts` around lines 134 - 139, The commission block uses
trade.commissionCurrency directly which can be undefined; create a fallback like
const commCurrency = trade.commissionCurrency || trade.currency and use
commCurrency for the EUR check, for the getEcbRate call (getEcbRate(rateMap,
date, commCurrency)) and as the event.currency so undefined is never passed;
keep the existing commission amount logic and only push the event when
commCurrency !== "EUR".
Review: REQUEST CHANGESHola @elmasvital, gracias por la contribución y por tu dedicación al proyecto. Sin embargo, tras una revisión exhaustiva (10 revisores independientes + análisis del historial git), este PR revierte un cambio de diseño intencional documentado en v0.39.2 (PR #171). No es un bug — es el comportamiento correcto. ❌ CI Tests: 5 fallosLos tests fallan porque fueron actualizados intencionalmente en v0.39.2 para reflejar el nuevo comportamiento:
Los crashes son especialmente reveladores: los rate maps de los tests solo incluyen fechas para CASH trades (las únicas que necesitaban ECB rates). El PR intenta buscar rates para TODAS las fechas de STK trades, que no están en los mapas. 🔴 Problemas Críticos1. Ganancias FX fantasma por falta de lotes previosCuando un usuario exporta solo el año fiscal actual (el caso más común), no existen lotes de divisa de años anteriores. Con este PR, cada compra de acciones en USD intenta consumir lotes inexistentes:
2. Doble contabilización en cuentas auto-convert (AFx)Para cuentas IBKR con auto-convert (la mayoría):
3. Modelo fiscal incorrectoArt. 37.1.l LIRPF grava la conversión de divisa (EUR↔FCY), no el gasto de FCY en valores. Comprar acciones en USD no "dispone de USD" — sigues expuesto a USD a través de las acciones. El hecho imponible es la conversión explícita de vuelta a EUR. DGT V2324-10 confirma FIFO para lotes de divisa, pero el hecho imponible es la conversión, no la aplicación a una compra de valores. 🟠 Problemas Altos4.
|
| PR | Problema |
|---|---|
| #124 | Primera detección de doble contabilización con FXCONV |
| #130 | Detección demasiado estrecha → EUR 5.800 ganancias espurias |
| #131 | AFx en Notes field no detectado → mismas ganancias espurias |
| #136 | Heurística de correlación de importes (4 señales, aún falla) |
| #143 | 48K EUR ganancias fantasma por lotes previos ausentes |
| #148 | Timing con settlement date vs trade date |
| #170 | Incluso CASH trades manuales en IDEALFX eran limpieza AFx |
| #171 | Eliminación definitiva — documentado como Hard Trace |
La eliminación fue explícita y deliberada. El commit dice: "Securities trades no longer produce implicit FX events (avoids phantom gains from missing prior-year lots and matches competitor behavior)". El sub-commit elimina stock_purchase/stock_sale del tipo FxTrigger.
💡 ¿Qué puede estar pasando en tu caso?
Si estás viendo resultados incorrectos con Lightyear, puede deberse a:
- Las filas
Conversionde tu CSV no se están parseando comoassetCategory: "CASH"— esto sería un bug del parser (src/parsers/lightyear.ts), no del motor FX - Tu cuenta de Lightyear realiza conversiones implícitas que no aparecen como filas
Conversion— en ese caso, no hay datos sobre los que calcular
En ambos casos, la solución correcta es mejorar el parser de Lightyear, no cambiar el motor FX.
Si quieres, comparte un ejemplo anonimizado de tu CSV (las filas relevantes de compra/venta y conversión) y revisamos si el parser las está interpretando correctamente. Abrimos un issue aparte si hay un bug real.
✅ Lo positivo
- La reestructuración de la rama CASH (mover
isFxconv()dentro del bloque CASH) es una mejora limpia - Tu instinto de cuestionar el comportamiento FX muestra compromiso con la corrección del proyecto
- El fix de
commissionCurrencysin fallback que señala CodeRabbit es válido (pero para el código actual, no para este PR)
Recomendación: Cerrar este PR. Si hay un caso concreto con Lightyear donde los resultados son incorrectos, abrimos un issue específico para investigar el parser.
|
Voy a revisar la documentación que aportas para poder comprender mejor el razonamiento que me has dado. Soy un programador amateur y esto me va grande y desde aquí pido disculpas pq se que te dará más trabajo. Pero creo que puedo aportar información valiosa. Con respecto a los AFx de IBKR me gustaría aportar lo que he detectado este finde semana para que al menos quede documentado. La mayoría de los lotes fantasmas que mencionas son los stocks que se están comprando sin soporte de lote fifo . Y es lógico pq se están ignorando los eventos cash que están catalogados como AFx en el que se abriría los lotes que se echan en falta. Si skipeas los eventos cash AFx habría que localizar qué stk se está comprando para ignorarlo también. Y hasta ahora no se ha podido identificar con garantías de falsos positivos. Pero claro no tenemos claro cómo relacionarlos, hasta ahora. Hay un parámetro que los relaciona y son los 16 primeros dígitos del parámetro brokerageOrderID. La mecánica de trabajo debería. En cuanto se detecte una autoconversión con note AFx se extrae su brokerageOrderID se localizan todos los eventos que coincidan con sus primeros 16 caracteres. Dejo un ejemplo en el que se muestra lo que indico. Con respecto a problemas con el parser de Lightyear. Como te comenté ahora mismo Declarenta está completamente roto con mis datos. En lightyear y en IBKR también. Voy a volver a revisar de porqué. Y también voy a hacer una prueba de concepto en mi Fork desde la versión anterior a 039.02 para que veas las diferencias. Ciero el PR como me suguieres. |
|
¡Gracias por cerrar el PR y por toda la información adicional, @elmasvital! No te disculpes — tu investigación fue valiosa y nos llevó a encontrar un bug real de polaridad que ya está corregido en v0.39.5. Sobre el Es un hallazgo interesante que documentaré. Sin embargo, por diseño en v0.39.2+ no necesitamos relacionar AFx con sus STK correspondientes, porque:
Así que los "lotes fantasma" que mencionas no deberían aparecer en v0.39.5 — eran el resultado del bug de polaridad (tu SELL manual se interpretaba como disposición en vez de adquisición). Sobre Lightyear roto: Si también te falla con Lightyear, ¿puedes abrir un issue con un extracto anonimizado del CSV? Así lo investigo por separado. Te pido que pruebes v0.39.5 en declarenta.com con tu XML y me digas si los resultados ahora tienen sentido. Si sigues viendo algo raro, abre un issue nuevo y lo miramos. ¡Gracias por tu paciencia y por la información del |
2 participants
Desde la versión 0.39.2 no se procesaban FxEvents que no fueran de cash. Los demás quedaban fuera. La lógica de esa parte también fue eliminada.
Se ha restaurado la lógica. Debería haber afectado a todos los brokers.
Summary by CodeRabbit