Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Precision may be lost when saving the exchange rate in the database #479

Closed
rivaldi8 opened this issue Feb 28, 2016 · 7 comments
Closed
Labels

Comments

@rivaldi8
Copy link
Collaborator

When the user enters an exchange rate, it's not saved directly. Instead, the
relation between the original amount and the converted amount is stored. As a
consequence, the exchange rate precision will depend on the precision of the
two currencies.

In addition, the converted amount is (wrongly) stored with the currency of the
original amount and rounded to its decimal places. If the original amount has
no decimal places, even more precision is lost.

For example, if the user transfers from a Yens account (no decimal places) to an
Euros account (2 decimal places) and enters:

  • transaction amount = 10 (Yens, no decimal places)
  • exchange rate = 0.123456

GnuCash calculates:

  • converted transaction amount: 1 (1.23456 rounded to 0 decimals)
  • exchange rate (stored in DB): 1/10 = 0.1

GnuCash should store directly the exchange rate instead of calculating it.

@rivaldi8
Copy link
Collaborator Author

I'm working on a fix.

By the way, should we be storing the exchange rate calculated from the user entered converted amount? Given the precision problems, I'd rather store exchange rate only when the user enters it explicitly.

@codinguser
Copy link
Owner

Thanks for catching this @rivaldi8
You're right, we try to compute the exchange rate even when it was explicity entered. I think we should save the exchange rate which the user entered. We should only compute the exchange rate when the user enters the converted amount directly.

And yes, we should still save the rate when the user only entered the converted amount. This is used for convenience the next time the user creates another transaction with the same currencies.
The user is still shown the dialog to confirm, but the value is pre-filled.

@codinguser codinguser added the bug label Feb 29, 2016
rivaldi8 added a commit to rivaldi8/gnucash-android that referenced this issue Mar 22, 2016
We were storing the rate as originAmount/convertedAmount, which was
limited to the precision of the currencies. Now we store directly the
entered rate.

Fixes codinguser#479
@codinguser
Copy link
Owner

@rivaldi8 The PriceDbAdapterTest.shouldOnlySaveOnePricePerCommodityPair() now fails in develop branch (after merging hotfix/patches). It seems when a Price is saved to the database, it's value is halved. You added a call to reduce() when getting the value numerator Price.getValueNum(). What does that do?

(you can checkout the current develop and test this)

@rivaldi8
Copy link
Collaborator Author

The call to reduce() in Price.getValueNum() (and getValueDenom()) is to avoid having to do this call. If we are to store it reduced, I think it's better to just do it automatically.

If you are ok with it, I can fix the unit test.

@codinguser
Copy link
Owner

oh sure, please go ahead and fix the test. Thanks 👍
And please add some documentation to the reduce() method.
It took me a while to figure out what the method does. I guess we need to work on our documentation. :)

@codinguser
Copy link
Owner

@rivaldi8 never mind. I'll fix it. I'm in the midst of some changes already anyway :)

@rivaldi8
Copy link
Collaborator Author

Ok, as you will :)

rivaldi8 added a commit to rivaldi8/gnucash-android that referenced this issue Apr 24, 2016
…nator.

Fixes a regression introduced in 66cc533 (bugfix codinguser#479) in which the
numerator/denominator division fails with an ArithmeticException if no
MathContext is provided. BigDecimal tries to return an exact result,
which is not possible when rounding and precision aren't specified.

See BigDecimal javadoc:
http://docs.oracle.com/javase/7/docs/api/java/math/BigDecimal.html

Fixes Crashlitics issue codinguser#196
https://fabric.io/gnucash/android/apps/org.gnucash.android/issues/571b4a95ffcdc04250f98ba0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants