Test ex_money support of Gringotts.Money protocol

[kipcole9]

Team, congratulations on getting 1.1 out with the Gringotts.Money protocol - apologies, I only just noticed.

I have merged my Gringotts branch into ex_money. Would you consider:

1. Testing from your side that all is ok. The master branch of ex_money now has the merged protocol support. I won't release it to hex until you're ok that it tests correctly.

2. I note there are quite a few compile-time warnings in compiling Gringotts on 1.6 and 1.7-dev. It disturbs me a little, enough to ask if you intend to remove the warnings in the next version?

Cheers, --Kip

[kipcole9]

My biggest concern on my implementation is that to_string/1 has a fixed format of 2 decimal places. I'm sure you indicated that this is what most gateways expect. But I know that some currencies have 3 or 4 decimal places and at least one crypto currency has up to 8 decimal places. The current implementation the Gringotts.Money protocol has currently:

    @simple_format "###.##"
    def to_string(%Money{} = money) do
      rounded_string =
        money
        |> Money.round
        |> Map.get(:amount)
        |> Cldr.Number.to_string!(format: @simple_format)

      {currency(money), rounded_string}
    end

Any comments would be welcome.

[oyeb]

Thank you @kipcole9! Addition of more gateways has been the focus in the last few months, but we did manage to bring out 1.1 somehow :smile: We're aware of those annoying compile time warnings and hate them as much as everyone else, I'll take some time out and release 1.1.1-rc this weekend with some fixes.

Though most gateways expect just 2 places, Gringotts.Money.to_string/1 does not require that all values have only 2 decimal places. In 1.1.0, we've shipped our own implementation for the Money type, and our to_string/1 takes the into consideration the currency's precision.

Many gateways (even the major players) don't take the effort to document such details, but we've seen that they generally work as we expect them to, or at least return a descriptive error message.

I'll probably add a section on the README to explain how we work with amounts and how this protocol protects the users' interest. For example, it is best if the user performs the rounding and handle any remainder amount themselves, otherwise Gringotts.Money will do an unsafe rounding!

[kipcole9]

@oyeb Totally understand your focus. I'll remove the forced formatting for the string output on my implementation so that it defaults to the currencies defined precision. My implementation does do the correct rounding (as far as I can test at least) so that should be all good.

Let me know if there is anything else you need me to do. And if you have your own implementation then perhaps I shouldn't provide one so as to avoid confusion?

Let me know what you think!

[oyeb]

Yeah, the default formatting should do it. IMO, it is best if the implementation is in ex_money, because it can be maintained and tested better there. We'll remove our implementation when we bump up the ex_money version in our deps.

We can't write a test in gringotts to test the implementation in ex_money, (because ex_money will get compiled before gringotts), so you'll have to write tests like this one.

[oyeb]

@kipcole9 I see that the protocol is implemented in ex_money v2.6.0 but there are no tests for it. Could you please add a few tests to your library, just to stay protected from future regression (failures).

I've written down some tests that represent the contract well in this gist, feel free to drop them as is in your tests.

[kipcole9]

Arrgggghh thats embarrassing that I let that slip in that way. And thanks much for the tests which I will add right away.

[kipcole9]

Done on master and all tests pass. Thanks for the prompt.