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

Add a section clarifying currency units; specify behavior when no currency is given #388

Merged
merged 3 commits into from
Oct 28, 2019

Conversation

thekidder
Copy link
Contributor

Explain pull request

This PR fixes a couple of bugs introduced in #379.

Firstly, the schema only allows integral costs; this is insufficient for many currencies that contain a fractional part (including USD). This PR changes the types of the cost fields to number instead of integer. Since number is a superset of integer, this should not break backwards compatibility.

Additionally, the wording does not specify the behavior when the currency field is not present. This change clarifies the language to use the old definition (cents of USD) to keep backwards compatibility.

Is this a breaking change

  • No, not breaking

Provider or agency

  • provider

@thekidder thekidder requested review from hunterowens, thekaveman and a team as code owners October 21, 2019 16:51
@hunterowens hunterowens added this to the 0.4.0 milestone Oct 21, 2019
@thekaveman
Copy link
Collaborator

@thekidder #379 didn't necessarily introduce a bug of integer-only costs; this has been in the spec/schema since the beginning as the cost fields were defined as USD cents.

Your proposal here does highlight some ambiguity. Allowing fractional costs only works if we're defining the cost at the "dollar" level, vs. the "cents/centavos/etc." level. The currency code carries no information about the scale of the value.

The other way to do this is to define the cost fields in terms of the 1/100 equivalent ("cent") for the identified currency code. I don't know enough about international currencies to know if this would handle all cases.

@thekidder
Copy link
Contributor Author

thekidder commented Oct 22, 2019

The currency code carries no information about the scale of the value.

If no scale is given, I would assume the major unit is implied (i.e. dollars for USD). MDS could specify a different scale, but given that different currencies may have different minor units, this is less clear to me.

But perhaps keeping integers is a better solution to avoid having money amounts as a floating point value and the resulting potential nondeterminism. As an example, Stripe's API documentation specifies using integers in the "currency's smallest unit": https://stripe.com/docs/currencies#zero-decimal

@thekaveman
Copy link
Collaborator

The Stripe example you referenced sounds like a good approach - do you want to add that wording into the descriptions?

@thekidder
Copy link
Contributor Author

Yes, agreed. Will update this shortly.

@hunterowens
Copy link
Collaborator

@thekidder any update on the the update?

@thekidder thekidder force-pushed the fix-cost-schema-type branch 2 times, most recently from 6bdebbc to 08b21a1 Compare October 28, 2019 03:23
@thekidder thekidder force-pushed the fix-cost-schema-type branch from 08b21a1 to 99e3c36 Compare October 28, 2019 03:25
@thekidder thekidder changed the title Allow fractional numbers for cost fields; clarify currency when null Add a section clarifying currency units; specify behavior when no currency is given Oct 28, 2019
@thekidder
Copy link
Contributor Author

Updated the wording here, using Stripe's behavior as an example.

There may still be some ambiguity here - I am not a currency expert - but I think this is an improvement.

@thekaveman thekaveman merged commit 4212429 into openmobilityfoundation:dev Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants