Fix (Lightyear) Error in commission calculation. Error in discounting the stock sale commission.#163
Fix (Lightyear) Error in commission calculation. Error in discounting the stock sale commission.#163elmasvital wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLightyear CSV parser refines normalized monetary fields: income, FX conversions, and buy/sell trades now prefer net-derived decimals ( ChangesMonetary field normalization across transaction types
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/parsers/lightyear.ts`:
- Around line 275-285: The commission field is incorrectly emitting a negative
buy-side fee on top of netAmount (double-counting); update the object so
buy-side commission is "0" (since tradeMoney/cost use netAmount that already
includes the fee) and do not emit feeDec.neg() for buys—i.e., change the
commission logic around the commission field (refer to tradeMoney, cost,
commission, feeDec and isSell) so buys produce "0" for commission while
preserving the intended sell behavior.
- Around line 259-261: The fallback price calculation currently uses
netAmount/qty which folds fees into the per-share price; change it to derive
trade price from the gross leg by adding fees back before dividing. In the block
using feeDec, netDec, qtyDec and pricePerShare, compute grossDec =
netDec.plus(feeDec) (both are already Decimal.abs()) and set price =
pricePerShare || (qtyDec.isZero() ? "0" : grossDec.div(qtyDec).toString());
ensure you replace the existing price computation so fees are not included in
the unit price.
🪄 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: 07385152-546d-4b2e-afac-977f22056dde
📒 Files selected for processing (1)
src/parsers/lightyear.ts
elmasvital
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/parsers/lightyear.ts (1)
262-262:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix price fallback to differentiate buys vs sells.
The fallback uses
grossAmount / qtyfor both transaction types, but that's only correct for sells:
- BUY:
GrossAmount = NetAmount + Fee, sogrossAmount/qty = Price + Fee/qty(overstates execution price by fee per share).- SELL:
GrossAmount = Qty × Price, sogrossAmount/qty = Price✓Use
netAmount / qtyfor buys,grossAmount / qtyfor sells. This was approved in the prior review thread but not applied.Proposed fix
- const price = pricePerShare || (qtyDec.isZero() ? "0" : grossDec.div(qtyDec).toString()); + const price = pricePerShare || (qtyDec.isZero() ? "0" : (isSell ? grossDec : netDec).div(qtyDec).toString());🤖 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/parsers/lightyear.ts` at line 262, The computed price fallback currently sets price = pricePerShare || (qtyDec.isZero() ? "0" : grossDec.div(qtyDec).toString()); but grossDec/qtyDec is only correct for sells; update the fallback to branch on the transaction side: if qtyDec.isZero() return "0", else use netDec.div(qtyDec) for BUY transactions and grossDec.div(qtyDec) for SELL transactions; keep the existing pricePerShare override and reference the same symbols (price, pricePerShare, qtyDec, grossDec, netDec) and the transaction side field used in this parser (e.g., type/side) to decide BUY vs SELL.
🧹 Nitpick comments (1)
src/parsers/lightyear.ts (1)
214-214: ⚡ Quick winUpdate comment to reflect
netAmountusage.The comment references "gross" but the code now uses
netAmountfor direction. Update line 214 to say "Positive net = acquiring that currency (BUY), negative = spending it (SELL)."🤖 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/parsers/lightyear.ts` at line 214, Update the inline comment that still refers to "gross" to reflect the current variable name `netAmount` used for direction in src/parsers/lightyear.ts; replace the text on that line with: "Positive net = acquiring that currency (BUY), negative = spending it (SELL)." and ensure the comment is adjacent to the `netAmount` usage so future readers see it tied to the `netAmount` variable in the parsing logic.
🤖 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.
Duplicate comments:
In `@src/parsers/lightyear.ts`:
- Line 262: The computed price fallback currently sets price = pricePerShare ||
(qtyDec.isZero() ? "0" : grossDec.div(qtyDec).toString()); but grossDec/qtyDec
is only correct for sells; update the fallback to branch on the transaction
side: if qtyDec.isZero() return "0", else use netDec.div(qtyDec) for BUY
transactions and grossDec.div(qtyDec) for SELL transactions; keep the existing
pricePerShare override and reference the same symbols (price, pricePerShare,
qtyDec, grossDec, netDec) and the transaction side field used in this parser
(e.g., type/side) to decide BUY vs SELL.
---
Nitpick comments:
In `@src/parsers/lightyear.ts`:
- Line 214: Update the inline comment that still refers to "gross" to reflect
the current variable name `netAmount` used for direction in
src/parsers/lightyear.ts; replace the text on that line with: "Positive net =
acquiring that currency (BUY), negative = spending it (SELL)." and ensure the
comment is adjacent to the `netAmount` usage so future readers see it tied to
the `netAmount` variable in the parsing logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e667afc-99fb-4222-8bbb-f9c6edbd46b4
📒 Files selected for processing (1)
src/parsers/lightyear.ts
|
Hey Igor, thanks for putting up a PR — the core insight here is correct! There's a real double-counting bug with fees because the FIFO engine adds Your fix logic is sound:
Issues to fix1. Empty Net Amt fallback The test Here Fix: add a fallback in the FX conversion block and the Interest/Reward block: // FX conversions
const netDec = new Decimal(netAmount || grossAmount);
// Interest/Reward
const amountDec = new Decimal(netAmount || grossAmount).abs();2. tradePrice fallback uses wrong base (CodeRabbit caught this one) When // Current (wrong when fee > 0):
const price = pricePerShare || (qtyDec.isZero() ? "0" : netDec.div(qtyDec).toString());
// Fix — use grossDec for price derivation:
const price = pricePerShare || (qtyDec.isZero() ? "0" : grossDec.div(qtyDec).toString());Could you push both fixes? Then CI should go green and I'll approve. Re: your Telegram question about dividends
The software reports them as AEAT expects:
So it uses |
2 participants
Se han corregido varios aspectos que hacían que los cash FX no cuadraran.
Dejo un csv para que se compruebe que con el sistema anterior los lotes_Fx no cuadran.
Lightyear Test Fallos FX comisiones 4.csv
Summary by CodeRabbit