Currency conversion precision

kmichailg commented 4 years ago

Hi!

Coming over from other packages, I've got to say this is definitely what I needed for this use case which deals with multiple currencies and down-to-the-last-cent precision work needed. However, one thing that bugs me is that I lose precision using the CurrencyConverter class. In the class, you are using BigRational to store and represent the money before returning it as a new Money instance.

Would it be possible instead to return it as as RationalMoney or even BigRational instead of a Money instance to be able to keep precision? Maybe a boolean parameter? This gives developers freedom to apply a context and/or rounding even after conversion to another currency.

My use case is I store the rational equivalents (as a string with it's currency through RationalMoney, maybe a different or query efficient approach can be suggested?) on the database to keep it precise, only using context and rounding upon display. So if there is another subsequent operation on the money object, precision will not be lost.

BenMorel commented 4 years ago

Hi, this is asking for even more precision that I thought anyone would even need, as by definition exchange rates are very volatile.

But you're right, CurrencyConverter is missing a method to convert to a RationalMoney, without rounding. How would you name it? convertToRationalMoney()? Any better idea?

BenMorel commented 4 years ago

Any feedback here?

jkuchar commented 4 years ago

@kmichailg As I cannot think about any usecase for myself I would like hear what are you going to use it for? 🙂

kmichailg commented 4 years ago

Hi @BenMorel and @jkuchar,

Just got back from vacation out of town, sorry for the gap in replies!

toRational(), toRationalMoney(), convertToRational(), or convertToRationalMoney() would be an awesome addition.

With regards to the use-case, we want to store all transactions in an easily audited currency (USD) and the user's local currency as well as all supported currencies on the platform at the time of transaction, which means a lot of conversions, and they should be the same all around. Instead of persisting the conversion rates at the time, converting to supported currencies seemed more viable since transactions are WORA (write-once, read-anywhere).

We needed the precision for conversion at the time of transaction persistence. We have a cache functionality to capture the conversion rate so we've solved the volatility problem. Because of the nature of the platform, we can't have rounded off pennies and nickels (unless we're displaying them), so manipulating money values with RationalMoney and storing them in <numerator>/<denominator> configuration currently works for us.

Thoughts?

BenMorel commented 4 years ago

@kmichailg That makes sense to me.

I'm all for adding the extra method, I just need some feedback on the naming: convertToRationalMoney()?

jkuchar commented 4 years ago

Why not just toRationalMoney()? It returns the same thing on which it is called. Am I right?

BenMorel commented 4 years ago

@jkuchar the current method is called convert(), so adding toRationalMoney() sounds weird to me.

I'd rather go with convertToRationalMoney(), or just convertToRational() to keep it short. What do you think?

BenMorel commented 4 years ago

Moving forward, I've called it convertToRational() and released in 0.4.3.

A few thoughts for the future:

Passing a context in the constructor does not make much sense anymore, as this only applies to convert() and not convertToRational(). A future version may move the context parameter from the constructor to convert().
convert() is now just a proxy for convertToRational(...)->to(...), does it make even sense to keep it in CurrencyConverter at all? This is a convenience method at best.

We could argue that CurrencyConverter::convert() could be changed to always return a RationalMoney, and the doc could explain that you should use ->to($context, $roundingMode) if you want a Money. This would be cleaner, but maybe less user-friendly?

Comments welcome!

kmichailg commented 4 years ago

We could argue that CurrencyConverter::convert() could be changed to always return a RationalMoney, and the doc could explain that you should use ->to($context, $roundingMode) if you want a Money. This would be cleaner, but maybe less user-friendly?

An argument for this is that whatever type you put in, it should also come out the same type. If you put in a RationalMoney instance, you should return a RationalMoney instance. If you put a Money instance, you should get another Money instance.

A type-check or class-check (like instanceof) should suffice I think?

BenMorel commented 4 years ago

@kmichailg This would not be a clean API, as when passing a RationalMoney, you wouldn't need a context and/or a rounding mode; but when passing a Money, you'd need at least to pass a rounding mode to get a Money back.

So it would be confusing IMO to have:

convert(AbstractMoney $money, $currency, $roundingMode) : AbstractMoney

because the rounding mode would be unnecessary when converting a RationalMoney.

So I'd either keep 2 methods, or have just one, that returns a RationalMoney, and advise users to call ->to() to convert it back to a Money if necessary.

brick / money

Currency conversion precision #22