Skip to content

Fix CSV import rounding errors with exchange rates (#5069)#5130

Closed
funnym0nk3y wants to merge 1 commit intoportfolio-performance:masterfrom
funnym0nk3y:claude/fix-github-issues-011CUvyYbJJ3SNMwYWB3ybcF
Closed

Fix CSV import rounding errors with exchange rates (#5069)#5130
funnym0nk3y wants to merge 1 commit intoportfolio-performance:masterfrom
funnym0nk3y:claude/fix-github-issues-011CUvyYbJJ3SNMwYWB3ybcF

Conversation

@funnym0nk3y
Copy link

This commit addresses issue #5069 where CSV imports fail validation due to rounding errors when dealing with foreign exchange rates.

Changes made:

  1. BaseCSVExtractor.java:

    • Replaced Math.round(BigDecimal.doubleValue()) with BigDecimal.setScale(0, RoundingMode.HALF_UP).longValue()
    • This avoids precision loss from converting BigDecimal to double
    • Applies to both extractGrossAmount() and createGrossValueIfNecessary()
  2. CheckForexGrossValueAction.java:

    • Added tolerance of ±2 (±0.02 in currency) to validation
    • This accounts for unavoidable rounding when Money values are limited to 2 decimal places
    • Prevents false positives while still catching actual errors

Root cause: Money stores amounts as longs with 2 decimal precision (factor of 100). When gross amounts are calculated with exchange rates, rounding to 2 decimals can cause small discrepancies that fail strict equality checks.

Example from issue:

  • Value: 95.26 CAD, FX Rate: 1.3795
  • Calculated: 95.26 / 1.3795 = 69.0540... → rounded to 69.05
  • Recalculated: 69.05 × 1.3795 = 95.254... → 95.25 ≠ 95.26

The fix improves calculation precision and adds reasonable tolerance.

…ance#5069)

This commit addresses issue portfolio-performance#5069 where CSV imports fail validation
due to rounding errors when dealing with foreign exchange rates.

Changes made:

1. BaseCSVExtractor.java:
   - Replaced Math.round(BigDecimal.doubleValue()) with
     BigDecimal.setScale(0, RoundingMode.HALF_UP).longValue()
   - This avoids precision loss from converting BigDecimal to double
   - Applies to both extractGrossAmount() and createGrossValueIfNecessary()

2. CheckForexGrossValueAction.java:
   - Added tolerance of ±2 (±0.02 in currency) to validation
   - This accounts for unavoidable rounding when Money values are
     limited to 2 decimal places
   - Prevents false positives while still catching actual errors

Root cause: Money stores amounts as longs with 2 decimal precision
(factor of 100). When gross amounts are calculated with exchange rates,
rounding to 2 decimals can cause small discrepancies that fail strict
equality checks.

Example from issue:
- Value: 95.26 CAD, FX Rate: 1.3795
- Calculated: 95.26 / 1.3795 = 69.0540... → rounded to 69.05
- Recalculated: 69.05 × 1.3795 = 95.254... → 95.25 ≠ 95.26

The fix improves calculation precision and adds reasonable tolerance.
@buchen buchen added the csv label Nov 9, 2025
@buchen
Copy link
Member

buchen commented Nov 9, 2025

I'll have a look - I also need to rebase because I just happen to touch that close (deriving the exchange rate if it is not present).

And a test case helps to avoid breaking this feature in the future.

Comment on lines -208 to +209
Money converted = Money.of(amount.getCurrencyCode(), Math.round(grossAmountConverted.doubleValue()));
Money converted = Money.of(amount.getCurrencyCode(),
grossAmountConverted.setScale(0, RoundingMode.HALF_UP).longValue());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what difference this make w.r.t. rounding. The double value is already rounded. If it is done first on the BigDecimal, that should not make a difference mathematically.

@buchen
Copy link
Member

buchen commented Dec 22, 2025

Thanks for the proposal. In the end, I did use a different approach than proposed by Cloude - see 39042f0 - unfortunately, I miss on character in the issue number in the commit message so it does not show up here.

@buchen buchen closed this Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants