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

Fix for rounded value in ExchangeRate #60

Closed
rutgervanwilligen opened this issue Jan 13, 2018 · 1 comment · Fixed by #61
Closed

Fix for rounded value in ExchangeRate #60

rutgervanwilligen opened this issue Jan 13, 2018 · 1 comment · Fixed by #61

Comments

@rutgervanwilligen
Copy link
Contributor

Hi! I'm using NodaMoney with custom currencies and exchange rates and was very happy to see that custom currencies can be initialized with a high number of decimals. Recently, however, I've discovered that the constructor of the ExchangeRate rounds the instantiated exchange rate value to 6 decimals, which essentially means that I won't be able to use this object for currency conversion, as we need exchange rates with more than 6 decimals.

The culprit is found in ExchangeRate.cs, line 33: Value = Math.Round(rate, 6); // value is a ratio

I've removed the rounding step in my local checkout, but this results in failing tests that involve ExchangeRates that are instantiated with the double data type (instead of decimal). I understand why that happens and why rounding is necessary for this data type, and therefore I would like to discuss the following:

  • Why are exchange rates rounded specifically to 6 decimals? (i.e. why six? That number seems arbitrary)
  • Why is there a constructor accepting a double in the first place? Especially in the Money domain, I think it would be logical to force the use of decimals when interacting with the library. I would vouch for removing this constructor.
  • If removing that constructor is not an option, then an alternative solution would be to add an additional parameter to that constructor indicating to which number of decimals the exchange rate should be rounded. Rounding it down to 6 decimals could be the default value; but hardcoding that value into the code is very nontransparent.

I'm very interested in hearing your opinions on this issue!

@RemyDuijkeren
Copy link
Owner

RemyDuijkeren commented Feb 3, 2018

Hi! Sorry for the late response, somehow didn't get a notification.

Rounding to 6 decimals
It isn't arbitrary ;-). This number was chosen, because of several sources and my conclusion of it. One of them is wikipedia section of ExchangeRate

Market convention from the early 1980s to 2006 was that most currency pairs were quoted to four decimal places for spot transactions and up to six decimal places for forward outrights or swaps. (The fourth decimal place is usually referred to as a "pip"). An exception to this was exchange rates with a value of less than 1.000 which were usually quoted to five or six decimal places. Although there is no fixed rule, exchange rates numerically greater than around 20 were usually quoted to three decimal places and exchange rates greater than 80 were quoted to two decimal places. Currencies over 5000 were usually quoted with no decimal places (for example, the former Turkish Lira). e.g. (GBPOMR : 0.765432 - : 1.4436 - EURJPY : 165.29). In other words, quotes are given with five digits. Where rates are below 1, quotes frequently include five decimal places.

Also other sources as Accounting Rules and how financial applications handle exchange rates. If you see the sources, most of them agree on 5 or 6 decimals. If you even look closer, they mean 5 to 6 significant figures, so the current implementation Value = Math.Round(rate, 6); could be improved. Especially if it used for cryptocurrencies, I can assume the 6 decimals would be a problem and 6 significant figures not. Are cryptocurrencies giving you problems in this area?

I need to look up the sources with accounting examples, but they prove that this rounding was good enough in conversions. Accounting apps seems to store this with max 6 decimals, because more would not improve your conversions. Only with very unlikely big amounts of money, you would lose or gain a penny., which seems to be acceptable.

So for these reasons the 6 decimals were chosen, to kind of forcibly get rid of the non significant figures. The override with a parameter for the number of decimals wasn't created (yet), because it seems to be kind of an golden rule when working with exchange rates and I want to prevent consumers to do the wrong thing.

I am happy to discuss this with sources/use cases/etc to improve the NodaMoney. I just want a library that helps app builders to use the most common rules by default (and where needed could be overridden).

Why is there a constructor accepting a double in the first place?
There are a couple of reasons for this:

  • An exchange rate isn't money, it is a ratio. A ratio is acceptable to be a double. It is could be that an Exchange Rate Provider would deliver exchange rates as doubles, so a constructor that can handle this would help in this area. Double is converted in the constructor immediately to Decimal, because it is known that we going to calculate with money and we don't want to have any conversion later on in the calculation.
  • In guidelines of building libraries (need to look up the source), it is ok to have constructor like this, even if there are side-effects. A constructor doesn't mean it is impliciet, but expliciet. It allows the consumer to think about it. Money and Decimal have the same kind of constructors, with remarks about the using. The remarks are missing for the constructor of ExchangeRate, so these should be added to make it clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants