solidusio / solidus

🛒 Solidus, the open-source eCommerce framework for industry trailblazers.
https://solidus.io
Other
4.99k stars 1.29k forks source link

Improve Handling of Currency that do not have 2 decimal places #708

Closed gmacdougall closed 7 years ago

gmacdougall commented 8 years ago

As noted in #707 we don't correclty handle currencies that do not have 2 decimal places in areas like reimbursement. The value is rounded off to the nearest 2 decimal places, which is not correct for currencies which do not use decimal places (like the Japanese Yen or South Korean Won) or currencies which use more than 2 decimal places (like Bahrani Dinars).

We should refactor this to math based on the appropriate level of precision.

tvdeyen commented 8 years ago

Just a fix idea coming to my mind: Shouldn't this solidus_i18n or similar library handle? Type conversion, rounding, etc.

mamhoff commented 8 years ago

There's a great library for handling multiple currencies: https://github.com/RubyMoney/money - and we're already using it for display purposes. However, all the math currently done in Solidus uses BigDecimal or Float, and, to make matters more difficult, our database schema assumes a two-digit currency.

Money objects are instantiated using the "fractional_unit of the given currency". This means we could potentially move from storing BigDecimal in the database to storing fractional units as Integer.

As for arithmetic, the Money class does it smartly. Have a look at the spec for that: https://github.com/RubyMoney/money/blob/master/spec/money/arithmetic_spec.rb

In the models, this would mean stuff like:

def amount 
   Spree::Money.new(super, currency: currency) 
end 

Across the project. Huge.

tvdeyen commented 8 years ago

Huge, but the "only" realâ„¢ solution to rounding issues. Every serious software uses fractal units over decimal ones.

:+1: for using money

mamhoff commented 8 years ago

OMG Spree::Money is substantially different from a proper Money object :crying_cat_face:

gmacdougall commented 8 years ago

Yeah. It's just a worse version of the RubyMoney Money.

mamhoff commented 8 years ago

So what would be a viable way of doing this? First remove Spree::Money and replace every occurence with proper Money objects, then convert the database to Integer and do the attribute accessors on all the models? I have a hard time imagining how this could be done piecemeal...

jhawthorn commented 8 years ago

I think this is something core should support, as core currently supports multiple currencies.

Here's a nice list of currencies and their decimal places https://en.wikipedia.org/wiki/ISO_4217#cite_ref-ReferenceA_6-0

Just as another data point, I've previously been asked to support 8 decimal places for bitcoin enthusiasts, but I feel that belongs in an extension as it's really excessive for physical currencies.

Yeah. It's just a worse version of the RubyMoney Money.

It's a thin wrapper at this point, we could probably begin delegating functionality to the underlying object with the goal of eventually just using ::Money

mamhoff commented 8 years ago

If we actually did the "store amounts in fractional units" thing, we wouldn't have to worry about how many decimal places any currency has:

2.2.1 :010 > Money.new(1234, :BTC).to_s
 => "0.00001234" 
2.2.1 :011 > Money.new(1234, :USD).to_s
 => "12.34" 

Note how integers were used to initialize the object in both cases.

gmacdougall commented 7 years ago

Close due to being stale. We've made some improvements here, but I don't believe we've fixed everything. We can highlight additional issues with individual issues.