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

[money-protocol] Add integer and string methods #85

Closed
oyeb opened this issue Jan 17, 2018 · 5 comments
Closed

[money-protocol] Add integer and string methods #85

oyeb opened this issue Jan 17, 2018 · 5 comments

Comments

@oyeb
Copy link
Contributor

oyeb commented Jan 17, 2018

Additions to the protocol

to_integer (for lack of a better name)

@spec to_integer(Money.t) :: {ISO4217 :: String.t, amount :: integer, exponent :: integer}

The exponent is not used and is just being made available to the application if need be.

This is useful for gateways that require amount as integer (like cents instead of dollars)
Examples:

usd_price = Money.new(4.1234, :USD)
Gringotts.Money.to_integer(usd_price) #=> {"USD", 412, -2}
bhd_price = Money.new(4.1234, :BHD)
Gringotts.Money.to_integer(bhd_price) #=> {"BHD", 4123, -3}
# the Bahraini dinar is divided into 1000 fils unlike the dollar which is divided in 100 cents

to_string

@spec to_string(Money.t) :: {ISO4217 :: String.t, amount :: String.t}

Returns a tuple of ISO4217 currency code and the amount as string. The amount must match this regex: ~r[\d+\.\d\d{n}] where n+1 should match the required precision for the currency. Gringotts cannot and will not validate this of course.

Examples:

usd_price = Money.new(4.1234, :USD)
Gringotts.Money.to_string(usd_price) #=> {"USD", 4.12}
bhd_price = Money.new(4.1234, :BHD)
Gringotts.Money.to_string(bhd_price) #=> {"BHD", 4.123}
@oyeb oyeb self-assigned this Jan 17, 2018
oyeb added a commit that referenced this issue Jan 18, 2018
* Also added an implementation of `to_string` for `ex_money`
@oyeb oyeb added this to In progress in Gringotts [next-release] Jan 18, 2018
@kipcole9
Copy link
Contributor

kipcole9 commented Jan 18, 2018

Good work team. I've implemented the protocol in https://github.com/kipcole9/money in the gringotts branch.

I have a few observations. Some with to_string/1 there are now implications for money and potentially localisation:

  1. value/1 will return the decimal amount. This amount will have an arbitrary number of decimal places. This is because $1.3456 is completely valid (think currency conversion, or financial trading). So value will return a potentially different result to to_integer since to_integer needs to round. This is in part why I return a remainder in to_integer_exp/1. This probably should be made clear in the docs.

  2. to_string/1 is defined to return a formatted string. Separators are locale specific, some are ., some are , and there are others. Is the formatting intended to have other separators? How is 10_000 going to be formatted? 10,000? 10,000.00? I think the protocol documentation needs to be really explicit about expectations for the formatting. What about negative numbers? - to the left, or right or anywhere? String formatting numbers and money amounts is non-trivial unless the use case is well understood. If the intent is entirely related to returning a formatted value to the user then a money packages default is probably ok. In my case, ex_money will format the number with localised separators etc etc.

  3. Rounding needs very careful documentation and handling. Ideally you would support a rounding_mode parameter with a default of :half_even which is what most financial texts recommend. But I think the implications of rounding should be more visible.

@oyeb
Copy link
Contributor Author

oyeb commented Jan 19, 2018

We've addressed your valid concerns by updating the docs. We've put this in bold:

When this highly precise amount is serialized into the network request, we
use a potentially lossy Gringotts.Money.to_string/1 or
Gringotts.Money.to_integer/1 to perform rounding (if required) using the
half-even strategy.

Hence, to ensure transparency, protect sanity and save real money, we
STRONGLY RECOMMEND that merchants perform any required rounding and handle
remainders in their application logic -- before passing the amount to
Gringotts's API.

After all, the merchant must understand the subtleties of finance and currency conversion to take care of their profits and losses.

  1. to_integer/1
    The perils about implicit rounding and potential loss are made clear. All protocol implementations will round using :half_even strategy.
    • The remainder is not made available by the protocol, because the protocol is "invoked" in the gateway implementation and there's no way to handle this remainder there (apart form returning it -- which does not make sense).
  2. to_string/1
    • All stringified amounts must match this (documented) regex: ~r/-?\d+\.\d\d{n}/, so there's only 1 separator: the decimal point (.).
    • The intent is to serialize the amount to the network request, not to show it to the user.
  3. We want all rounding to happen outside Gringotts in the merchant's business logic. Still, when required, :half_even is the default strategy.

@kipcole9
Copy link
Contributor

Got it, looks good. Especially helpful to know that to_string/1 is not localised. I've updated my implementation to reflect that. I still need to add tests though.

@oyeb
Copy link
Contributor Author

oyeb commented Jan 19, 2018

We have some test cases here. I'm sure you'll devise some more test cases than we have! 🙂

@kipcole9
Copy link
Contributor

And i'm sure you'll tell me if any tests are failing because of my bad code :-)

I'm really enjoying the collaboration - I think this is a great way to leverage the efforts of many in a way that multiplies the benefit for all.

@oyeb oyeb closed this as completed Jan 19, 2018
@issue-sh issue-sh bot removed the In Progress label Jan 19, 2018
@oyeb oyeb reopened this Jan 19, 2018
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
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".
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

2 participants