feat(fx): opt-in FX-FIFO movement trace for audit/diagnostics (#230)#238
Conversation
Adds an opt-in audit ledger of the FX-FIFO engine's movements so a developer or advisor can verify exactly how a 1633/1637 figure was built (acquire → park → unpark → discard → profit → dispose, each with running pool/parked FCY balances). Born from issue #230: the reporter kept a hand console.log ledger to check the FX math — this makes it a first-class, lossless artifact. - Engine: FxFifoEngine.enableTrace()/getTrace() record each pool/park movement. OFF by default and ZERO-COST when off (record() returns immediately unless enabled); all 32 existing FX tests pass unchanged. New FxTraceEvent/FxTraceKind types (decimal strings, no Decimal dep in consumers). - Report: ReportOptions.fxTrace -> TaxSummary.fxTrace (full all-year ledger); ignored under monodivisa (skipFx). Never changes any computed casilla. - Serializer: src/generators/fx-trace.ts serializeFxTrace(trace, jsonl|csv). - CLI: --fx-trace [file] / --fx-trace-format jsonl|csv (file or stderr). - Web: download gated behind #debug / localStorage.declarenta_debug — NEVER in the standard UI (the reporter's explicit ask). - The 'discard' kind is the audit-visible proof of the loss-sell principle: un-converted loss-spent dollars show as discard (no FX), never dispose. 42 new tests (engine trace, serializer, e2e golden-ledger reconciliation: Sigma dispose gainLossEur === fxGains.netGainLoss). Full suite 1653 passing, typecheck + typecheck:tests + eslint clean. Refs #230
|
Warning Review limit reached
More reviews will be available in 49 minutes and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an opt-in FX-FIFO movement audit trace to the tax report engine. ChangesFX-FIFO Opt-in Audit Trace Ledger
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 #238 +/- ##
==========================================
+ Coverage 95.14% 95.19% +0.05%
==========================================
Files 52 53 +1
Lines 4981 5038 +57
Branches 1666 1686 +20
==========================================
+ Hits 4739 4796 +57
Misses 209 209
Partials 33 33
🚀 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
🤖 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 `@tests/engine/fx-fifo-trace.test.ts`:
- Around line 35-39: In the Op type definition, replace all `number` type
annotations that represent monetary amounts, rates, and prices with `string`
across all tuple variants (fund, conv, buy, and sell) to avoid binary rounding
before values are converted to Decimal, ensuring compliance with the coding
guideline requiring Decimal usage for all monetary calculations.
🪄 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: b93bba53-f40f-4661-adc9-b0ef659b45d6
📒 Files selected for processing (11)
CLAUDE.mdsrc/cli/index.tssrc/engine/fx-fifo.tssrc/generators/fx-trace.tssrc/generators/report.tssrc/index.tssrc/types/tax.tssrc/web/main.tstests/engine/fx-fifo-trace.test.tstests/generators/fx-trace.test.tstests/integration/fx-trace-e2e.test.ts
| type Op = | ||
| | ["fund", string, number, number] | ||
| | ["conv", string, number, number] | ||
| | ["buy", string, number] // cost (rate irrelevant — buy parks, never realizes) | ||
| | ["sell", string, number, number, number]; // cost, proceeds, saleRate |
There was a problem hiding this comment.
Avoid number in the monetary Op DSL.
Op currently models amounts/rates as number, which can introduce binary rounding before values are converted to Decimal. Model these operands as strings instead.
Suggested patch
-type Op =
- | ["fund", string, number, number]
- | ["conv", string, number, number]
- | ["buy", string, number] // cost (rate irrelevant — buy parks, never realizes)
- | ["sell", string, number, number, number]; // cost, proceeds, saleRate
+type Op =
+ | ["fund", string, string, string]
+ | ["conv", string, string, string]
+ | ["buy", string, string] // cost (rate irrelevant — buy parks, never realizes)
+ | ["sell", string, string, string, string]; // cost, proceeds, saleRateAs per coding guidelines, “Always use Decimal from decimal.js for any monetary calculation; never use JavaScript Number for amounts, rates, or prices.”
🤖 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-trace.test.ts` around lines 35 - 39, In the Op type
definition, replace all `number` type annotations that represent monetary
amounts, rates, and prices with `string` across all tuple variants (fund, conv,
buy, and sell) to avoid binary rounding before values are converted to Decimal,
ensuring compliance with the coding guideline requiring Decimal usage for all
monetary calculations.
Source: Coding guidelines
…edger test Review follow-up (PR #238 review, MEDIUM finding): the FX trace is the full all-year ledger but the casillas are year-filtered, so the golden-ledger identity is 'Sigma dispose.gainLossEur WHERE date in year === fxGains.netGainLoss' — summing ALL dispose rows over/under-states the box on a multi-year account. - Document the per-year reconciliation rule on TaxSummary.fxTrace (types/tax.ts) and in the CLAUDE.md trace note. - Add a multi-year e2e test pinning BOTH directions: all-year sum (450) != year casilla (300), and year-filtered sum (300) === casilla. Engine-verified. Docs + test only; no engine/behavior change. Suite 1656 passing. Refs #230
1 participant
What
An opt-in audit ledger of the FX-FIFO engine's internal movements, so a developer or advisor can verify exactly how a Casilla 1633/1637 figure was built — the full
acquire → park → unpark → discard → profit → disposesequence, each row carrying the running pool and parked FCY balances after the event.Born from issue #230: the reporter was keeping a hand
console.logledger to check the carry-basis FX math. This turns that into a first-class, lossless artifact (and makes the engine mechanically testable via golden-ledger tests).Design — a developer/advisor tool, never a user feature
FxFifoEngine.enableTrace()flips a flag; everyrecord()returns immediately unless enabled. All 32 pre-existing FX tests pass unchanged — the trace never alters a computed casilla (pinned by an e2e test asserting byte-identical figures with/without the trace).#debug(orlocalStorage.declarenta_debug); the wizard/results flow is byte-identical for everyone else.skipFx) — the FX engine doesn't run, sofxTraceis gracefullyundefined.Surface
fx-fifo.ts):enableTrace()/getTrace(). New typesFxTraceEvent/FxTraceKind(all monetary fields are decimal strings — noDecimaldep in consumers).report.ts):ReportOptions.fxTrace→TaxSummary.fxTrace(the full all-year ledger, so a lot's whole lifecycle reconciles).generators/fx-trace.ts):serializeFxTrace(trace, "jsonl" | "csv").--fx-trace [file]/--fx-trace-format jsonl|csv(writes to a file, or stderr so it never corrupts a stdout JSON/CSV payload).#debug-gated "Descargar traza de cálculo FX" download.src/index.ts; documented inCLAUDE.md.The
discardkind — the audit-visible proof of the loss-sell principle (issue #230)Dollars spent inside a losing position that never converted to EUR appear in the ledger as
discard(no FX realized), never asdispose. That's the mechanical evidence that an un-converted loss-sell books nothing into 1633/1637 (Art. 14.2.e) — exactly the reporter's "hamburger in dollars" principle.Verified end-to-end
Smoke-tested on a real IBKR multicurrency fixture → a 31-row CSV ledger (park/unpark/profit, running balances, uncovered notes, per-position keys). 42 new tests (engine trace, serializer, e2e golden-ledger reconciliation:
Σ dispose.gainLossEur === fxGains.netGainLoss). Full suite 1653 passing, typecheck + typecheck:tests + eslint clean.Refs #230
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests