feat(revolut): support transaction-log format#154
Conversation
…ades) Revolut's Trading Account Statement has two XLSX formats: closed-positions summary (existing) and transaction log (Date/Ticker/Type/Quantity/Price per share/Total Amount/Currency/FX Rate). The transaction log is what users actually get from the Revolut app, and the parser was rejecting it entirely. - Add transaction-log detection in detectRevolutXlsx (both formats now detected) - Parse BUY-MARKET, BUY-LIMIT, SELL-MARKET, SELL-LIMIT as individual trades - Parse "CCY amount" strings (e.g. "EUR 15.61", "USD -217.14") - Parse ISO 8601 timestamps with ms/μs precision - Extract CASH TOP-UP / CASH WITHDRAWAL as cashTransactions - Infer open positions from unmatched buys (enables Modelo 720/D-6) - Update text-based detect() for transaction-log headers - 35 new tests (78 total), anonymized fixture from real user file
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. 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 (1)
📝 WalkthroughWalkthroughAdded Revolut transaction-log XLSX format support to the parser with new format detection and parsing logic, integrated it into the CLI broker-file flow, and implemented wash-sale detection for trades without ISIN by falling back to symbol-based matching. Includes comprehensive test coverage. ChangesRevolut Transaction-Log XLSX Support
Wash-Sale Symbol Fallback for Empty ISIN
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 53 minutes and 17 seconds.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
+ Coverage 97.61% 97.62% +0.01%
==========================================
Files 37 37
Lines 7380 7585 +205
Branches 1499 1551 +52
==========================================
+ Hits 7204 7405 +201
- Misses 175 179 +4
Partials 1 1
🚀 New features to boost your workflow:
|
- Add Math.abs + isFinite guard on transaction-log quantity to prevent negative and Infinity propagation - Cap sell quantity at net position to prevent negative position state - Tighten hasTxnLogHeaders to require "price per share" column - Add symbol-based fallback in wash-sale homogeneousKey() for trades without ISINs (Revolut, Lightyear, etc.) - Add Revolut XLSX detection in CLI before eToro - Add 9 edge case tests (BUY-LIMIT, negative qty, sell overflow, Infinity, corrupted XLSX, empty sheet, wash-sale symbol fallback)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/parsers/revolut.ts (1)
310-314: ⚡ Quick winPosition tracking uses
Numberarithmetic instead ofDecimal.The
netQty,totalCost, andcostPerUnitcalculations use JavaScriptNumber, which can introduce floating-point precision errors on large quantities or costs. While the output strings go to the FIFO engine (which uses Decimal), the open position inference here could accumulate rounding drift.Per coding guidelines: "Always use
Decimalfromdecimal.jsfor any monetary calculation."This is lower risk since open positions are informational (Modelo 720/D-6) rather than tax-critical P&L, but worth noting.
♻️ Suggested fix using Decimal for position tracking
+import Decimal from "decimal.js"; // Track net positions per symbol for open-position inference const positions = new Map<string, { symbol: string; currency: string; assetCategory: AssetCategory; - netQty: number; totalCost: number; + netQty: Decimal; totalCost: Decimal; }>(); // In BUY block: const pos = positions.get(ticker) ?? { - symbol: ticker, currency: currencyRaw, assetCategory, netQty: 0, totalCost: 0, + symbol: ticker, currency: currencyRaw, assetCategory, netQty: new Decimal(0), totalCost: new Decimal(0), }; -pos.netQty += qty; -pos.totalCost += absTotalAmount; +pos.netQty = pos.netQty.plus(qty); +pos.totalCost = pos.totalCost.plus(absTotalAmount); // In SELL block: -if (pos && pos.netQty > 0) { - const sellQty = Math.min(qty, pos.netQty); - const costPerUnit = pos.totalCost / pos.netQty; - pos.totalCost -= costPerUnit * sellQty; - pos.netQty -= sellQty; +if (pos && pos.netQty.greaterThan(0)) { + const sellQty = Decimal.min(qty, pos.netQty); + const costPerUnit = pos.totalCost.div(pos.netQty); + pos.totalCost = pos.totalCost.minus(costPerUnit.times(sellQty)); + pos.netQty = pos.netQty.minus(sellQty); }Also applies to: 428-434, 440-458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/revolut.ts` around lines 310 - 314, The positions Map currently stores netQty, totalCost and computes costPerUnit using JavaScript Number which can cause floating-point drift; change the positions value shape to hold Decimal instances (import Decimal from 'decimal.js'), initialize netQty and totalCost as new Decimal(0), and replace all arithmetic updates to use Decimal methods (plus, minus, times, div) when adjusting positions and computing costPerUnit; ensure any outputs passed to the FIFO engine are converted to string (or toNumber only where safe), and apply the same Decimal changes to the other occurrences noted (the update blocks around the regions referenced as 428-434 and 440-458) while keeping symbol/assetCategory fields as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/parsers/revolut.ts`:
- Around line 310-314: The positions Map currently stores netQty, totalCost and
computes costPerUnit using JavaScript Number which can cause floating-point
drift; change the positions value shape to hold Decimal instances (import
Decimal from 'decimal.js'), initialize netQty and totalCost as new Decimal(0),
and replace all arithmetic updates to use Decimal methods (plus, minus, times,
div) when adjusting positions and computing costPerUnit; ensure any outputs
passed to the FIFO engine are converted to string (or toNumber only where safe),
and apply the same Decimal changes to the other occurrences noted (the update
blocks around the regions referenced as 428-434 and 440-458) while keeping
symbol/assetCategory fields as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: daf575d4-75e0-4d77-84be-a21d4f1de380
⛔ Files ignored due to path filters (1)
tests/fixtures/revolut-txnlog-sample.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (5)
src/cli/index.tssrc/engine/wash-sale.tssrc/parsers/revolut.tstests/engine/wash-sale.test.tstests/parsers/revolut.test.ts
Address CodeRabbit nitpick: replace Number with Decimal.js for netQty, totalCost, and costPerUnit in open-position inference to prevent floating-point drift per project financial precision rules.
1 participant
Summary
What's new in the parser
detectRevolutXlsx()(both formats detected)"EUR 15.61"/"USD -217.14"formatdetect()updated for transaction-log headers (no false positives with Trading 212)Tests
Test plan
npm testSummary by CodeRabbit
New Features
Tests