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

Replace floating point numbers with a Money lib #62

Closed
oyeb opened this issue Dec 28, 2017 · 5 comments
Closed

Replace floating point numbers with a Money lib #62

oyeb opened this issue Dec 28, 2017 · 5 comments

Comments

@oyeb
Copy link
Contributor

oyeb commented Dec 28, 2017

Update: This is WIP here

[Recommendation]

It's clear that using floating point numbers for representing money is a bad idea, and using plain integers fairly limits the application since there exist currencies with more than 2 decimal places. This rules out supporting money as it uses integers.

Which kind of monie then?

ex_money and monetized both use Decimals to represent money, but ex_money offers some more features than monetized, like:

  • ISO 4127 aware formatting.
  • Explicit rounding obeys the rounding rules as defined by the Unicode consortium in its CLDR repository and implemented by the hex package ex_cldr. These rules define the number of fractional digits for a currency and the rounding increment where appropriate.
  • Provides simple utilities for financial accounting (a plus point for merchants -- merchants are more likely to adopt ex_money).
  • Provides a way to perform currency conversion without writing extra code.

Who benefits from ex_money

There's not much to gain for Gringotts, all it does is format the amount (as documented in the gateway or as per ISO4127) to construct a correct POST request for the gateway. The real benefit is to merchants already using ex_money, or planning to do so. They won't have to convert their super awesome Money structs into floats/ints/string and be sure that Gringotts does not cause them any loss.

What will this break?

  • All methods accepting an amount parameter will now accept only Money.t.
    amount = ~M[12.34]USD # or Money.new("USD", 12.34)
    Gringotts.purchase(:payment_worker, Gringotts.Gateways.XYZ, amount, card, opts)
  • [Discuss] Should we deprecate usage of float/integer or should we completely remove support for it?
    • There's no point in accepting floats and converting internally to Money.t only to convert them back into plain strings in the end.
    • Working with integers will make it impossible to unify our API across gateways. Some gateways like to work in 1/100th units some in units, so if we allow integers, we'll have to decide if Gringotts works in 1/100th units or unit, then enforce that in our API. Let's just use Money.t instead as our unambiguous "protocol".
  • [Resolution] The ayes have it, the ayes have it: Remove support float and integer completely!

Migration tips (for merchants)

Any merchant doing even trivial financial accounting in their app would be using some Money lib.

  • Merchants still using integers or floats should consider using a Money lib, preferably ex_money.
  • Create a Money.t struct from whatever your representation is and pass that in Gringott's API methods.
  • We could work on @kipcole9's suggestion of implementing a protocol and accept any Money lib that implements it. But how do we go about doing that?

TL;DR

Let's use ex_money or a protocol and track it's integration into Gringotts here.

@oyeb oyeb self-assigned this Dec 28, 2017
@LostKobrakai
Copy link

We could work on @kipcole9's suggestion of implementing a protocol and accept any Money lib that implements it. But how do we go about doing that?

Let's use ex_money or a protocol and track it's integration into Gringotts here.

You could do both. Use a protocol for the values sent to your public interface functions and convert those to money structs used internally.

@kipcole9
Copy link
Contributor

I think its important not to tie Gringotts to a single lib in any way. That would limit choices and potentially stifle innovation.

I support @LostKobrakai's suggestion. Protocol support at the public api level where money is being submitted to the gateway. For a return value from the gateway use your own Gingotts.Money.t type defined however you need as long as it has a currency code and a decimal amount. And make sure your Gingotts.Money.t type supports the Protocol :-)

@oyeb oyeb mentioned this issue Jan 2, 2018
4 tasks
pkrawat1 pushed a commit that referenced this issue Jan 10, 2018
Building on the discussion in #62, this PR introduces a protocol to replace amount argument, and removes the need to specify currency in config or opts.

Supporting `ex_money` and `monetized` money libs out of the box.

Usage:-
Money can now be passed like this while calling the public API methods.

money = %{amount: Decimal.new(2017.18), currency: "USD"}

Conversation for this happened here on elixir forum and https://elixirforum.com/t/gringotts-a-complete-payment-library-for-elixir-and-phoenix-framework/11054
@oyeb
Copy link
Contributor Author

oyeb commented Jan 11, 2018

Context

  • In Introducing the Money protocol #71, the protocol defines just 2 methods: value and currency.
  • All money libs provide a to_string method that returns a string including the currency symbol/code.

Additions to the protocol

  • float_string and integer_string (or similarly named) methods because most gateways accept a pair of "amount-as-string" and "currency code"; and though it's easy to extract and format the currency, converting the Decimal amount into a "precision aware string" is non trivial (rounding issues).
    • Interested libs could perhaps provide this by super-charging their to_string:
      # Notice the automatic rounding
      price = Money.new(4.1234, :USD)
      Money.to_string(price, currency: false) #=> "4.12"
      Such a to_string will make implementation of float_string trivially simple.
    • integer_string is useful for gateways that require amount as integer (like cents instead of dollars)
      Examples:
      Gringotts.Money.float_string(price) #=> "4.12"
      Gringotts.Money.integer_string(price) #=> "412"
      Money.new(4.1234, :BHD)
      |> Gringotts.Money.integer_string #=> "4123"
      # the Bahraini dinar is divided into 1000 fils unlike the dollar
      # which is divided in 100 cents

@kipcole9
Copy link
Contributor

kipcole9 commented Jan 11, 2018

Looks good. I believe that the integer output option should be a separate function. Following this discussion I have implemented, for ex_money two functions, to_integer_exp/1 and from_integer/2

to_integer/1

# current ex_money version
iex> m = Money.new(:USD, "200.012356")
#Money<:USD, 200.012356>

# Returns {iso4217, integer, exponent, remainder}
iex> Money.to_integer_exp(m)
{:USD, 20001, -2, Money.new(:USD, 0.002356)}

The reason that this should be separate from to_string/1 and should also be part of the protocol is because the money libs should take care of the relevant rounding - this is not as straight forward as it seems since different currencies have different rounding rules. My current version also returns a remainder since after rounding a remainder my exists. That is not Gringotts concern however so I propose the protocol function be to_integer(Money.t) :: {ISO4217 :: String.t, amount :: integer, exponent, integer}. I think the exponent should be included because again, I don't think Gringotts should need to have domain knowledge to calculate that but I think its important as an integrity check on the interface.

from_integer/1

iex> Money.from_integer(20000, :USD)
#Money<:USD, 200.00>

iex> Money.from_integer(200, :JPY)
#Money<:JPY, 200>

I think that could be the same API for Gringotts, being from_integer(amount :: integer, currency :: String.t). Note that is the money libraries responsibility to determine the exponent for the currency type.

Its possible that the payment gateway always assumes that the exponent is -2 - it would be good to know if that is the case. If it is, then the exponent would have to be passed in from Gringotts as a third parameter.

As soon as you settle on a draft protocol I'll implement it in ex_money master so you can try it out.

@oyeb oyeb added this to In progress in Gringotts [next-release] Jan 15, 2018
@oyeb
Copy link
Contributor Author

oyeb commented Jan 17, 2018

Moving further progress to #85
A separate integer output function does seem better. We are not adding a from_integer right now.

oyeb added a commit that referenced this issue Jan 18, 2018
* Also added an implementation of `to_string` for `ex_money`
oyeb added a commit that referenced this issue Jan 19, 2018
* Adds `to_string` and `to_integer` to the protocol
* Fixes #62 and #85
* Implements all protocol methods for `ex_money`
* Adds integration test with ex_money
* Adapted Monei with protocol updates
* Adds test for `Any`
* The default rounding strategy for implementations of Gringotts.Money
protocol is HALF-EVEN.
* Updated public API docs with "perils of rounding".
@oyeb oyeb moved this from In progress to Done in Gringotts [next-release] Jan 19, 2018
@oyeb oyeb removed the discussion label Jan 19, 2018
pkrawat1 pushed a commit that referenced this issue Jan 25, 2018
Building on the discussion in #62, this PR introduces a protocol to replace amount argument, and removes the need to specify currency in config or opts.

Supporting `ex_money` and `monetized` money libs out of the box.

Usage:-
Money can now be passed like this while calling the public API methods.

money = %{amount: Decimal.new(2017.18), currency: "USD"}

Conversation for this happened here on elixir forum and https://elixirforum.com/t/gringotts-a-complete-payment-library-for-elixir-and-phoenix-framework/11054
pkrawat1 pushed a commit that referenced this issue Jan 25, 2018
* Adds `to_string` and `to_integer` to the protocol
* Fixes #62 and #85
* Implements all protocol methods for `ex_money`
* Adds integration test with ex_money
* Adapted Monei with protocol updates
* Adds test for `Any`
* The default rounding strategy for implementations of Gringotts.Money
protocol is HALF-EVEN.
* Updated public API docs with "perils of rounding".
@oyeb oyeb mentioned this issue Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

4 participants