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

Should user agent validate currency? #490

Closed
perja12 opened this Issue Mar 30, 2017 · 36 comments

Comments

Projects
None yet
8 participants
@perja12
Copy link

perja12 commented Mar 30, 2017

In the section "5. PaymentCurrencyAmount dictionary" the spec says:

currency: A string containing a currency identifier. The value of currency can be any string that is valid within the currency system indicated by currencySystem.

The last sentence there makes sense, but it is not clear exactly how the user agent should behave here. Should the UA validate the currency if it knows about the currency system and then throw TypeError if the currency isn't valid in that system? And what should be the behavior of the UA if there is an unknown currency system?

I think it would be good for implementers if the spec was clearer or provided some guidelines here.

@ianbjacobs

This comment has been minimized.

Copy link
Collaborator

ianbjacobs commented Mar 30, 2017

There is no expectation of UA validation of currency.

@perja12

This comment has been minimized.

Copy link
Author

perja12 commented Mar 30, 2017

Thanks, would it be good to add that as a note in the spec?

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Mar 31, 2017

Yeah, this part of the spec needs to be clarified, specially if we are putting currencySystem "at risk". As it currently reads (using the word "valid"), it sounds like some validation would take place.

I don't currently know what happens if a developer sets: currency: "usd" vs currency: "uSd" vs currency: "USd" and so on... The ISO whatever for currencies has them all in upper case.

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Mar 31, 2017

@rsolomakhin, are you folks doing any kind of checks or normalization of currency values in Chrome?

@marcoscaceres marcoscaceres added this to the CR milestone Mar 31, 2017

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Mar 31, 2017

I'll note that at Intl.NumberFormat doesn't care about case (it normalizes to uppercase), but does do validation:

const f = new Intl.NumberFormat(navigator.languages, {
      style: "currency", 
      currency: "uSd",
      currencyDisplay: "symbol",
});

f.resolvedOptions(); // currency: "USD"

// Throws RangeError: currency is not a well-formed currency code
new Intl.NumberFormat(navigator.languages, {
      style: "currency", 
      currency: "what is this I don't even", 
      currencyDisplay: "symbol",
});

So it might make sense to actually do validation also, given that we know what the currency system a priori.

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Mar 31, 2017

Also, the reference to ISO4217 should be normative.

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Mar 31, 2017

I'd be in support of doing validation of currencies - chiefly because without it, we don't have have the means to do localization of currencies when we display them in the payment sheet (e.g., for the total).

For example:
https://github.com/marcoscaceres/web-payments-proto/blob/gh-pages/src/PaymentSheet.Total.js#L25

@domenic, @zkoch?

@perja12

This comment has been minimized.

Copy link
Author

perja12 commented Mar 31, 2017

Regarding localization and UAs: we could have a fallback formatting for unknown currencies (like " "), but I see that this can be problematic with f.ex. RTL. It also depends a lot on how the UA chooses to implement the UI for this. One could f.ex. separate the value and the currency instead of having both of them combined in one string.

It is maybe problematic to have any strict validation requirements in the spec (and throw TypeError) if the currency isn't supported by NumberFormat or similar classes? Implementations will then only accept known currencies and will have to be updated to support new ones.

@domenic

This comment has been minimized.

Copy link
Collaborator

domenic commented Mar 31, 2017

@domenic, @zkoch?

I don't have any opinions on currency systems, as my understanding is they're not implemented anywhere nor do any browsers have plans to do so. There are much more fundamental issues here, such as #305 and #343 that would need to be solved before considering how we validate strings.

@domenic

This comment has been minimized.

Copy link
Collaborator

domenic commented Mar 31, 2017

Oh, I misunderstood... the original post talks about currency systems, but then the post got into currencies in general. It would be good to spec what happens if you do currency: "usD" or currency: "what is this I don't even".

@rsolomakhin

This comment has been minimized.

Copy link
Collaborator

rsolomakhin commented Mar 31, 2017

Chrome does not normalize or validate currencies. This is to match the spec.

@rsolomakhin

This comment has been minimized.

Copy link
Collaborator

rsolomakhin commented Mar 31, 2017

We would love to validate currencies according to either a regex (^[A-Z]{3}$), a predefined list from CLDR, or OS-dependent.

@adrianhopebailie

This comment has been minimized.

Copy link
Collaborator

adrianhopebailie commented Apr 1, 2017

Implementations should IMO validate currencies if they can. In other words, for any currency system for which they have the ability to do validation.

I assume that this will only include ISO4217 defined currencies initially but since there are browsers inventing their own currencies [1] I expect that won't be the case for long :)

[1] https://www.americanbanker.com/news/web-pioneer-plans-blockchain-based-digital-ad-platform

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Apr 3, 2017

We should make it use the infrastructure from the Intl API, IMO:

  • It's basically /^[a-z]{3}$/i (case insensitive match) using IsWellFormedCurrencyCode.
  • Throws RangeError when invalid.
  • We don't throw when the currency is unknown (so long as IsWellFormedCurrencyCode returns true, it's valid).
  • Canonicalize code internally to uppercase.

When the value is not in ISO 4217, the canonicalized code can be used as the symbol - we make a note of this in a l18n guidance section.

Thus:

// This is fine
const details = {
  total: {
    label: "Total due",
    // FOO50.00
    amount: { currency: "Foo", value: "50.00" }, 
  }
};

// This throws
const details = {
  total: {
    label: "Total due",
    amount: { currency: "this is no good", value: "50.00" }, 
  }
};
@rsolomakhin

This comment has been minimized.

Copy link
Collaborator

rsolomakhin commented Apr 3, 2017

Sounds reasonable.

@adrianhopebailie

This comment has been minimized.

Copy link
Collaborator

adrianhopebailie commented Apr 3, 2017

Be aware that using language features related to currency is a dangerous trap. It's like trying to use domain or email parsing features that have a hardcoded list of TLDs.

Sure, it's convenient for the poor designer that has to figure out the UI but it will likely come back to bite you later.

Forcing a format like /^[a-z]{3}$/i is also dangerous because it will encourage squatting on codes that may become legitimate ISO 4217 codes in future.

I recommend @marcoscaceres approach for the default currency system (ISO4217) but would encourage some sane limits for other systems too (maybe just a max length and limited charset?)

Use case: Buy an upgrade on delta.com using the currency SKYMILES

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Apr 4, 2017

I recommend @marcoscaceres approach for the default currency system (ISO4217) but would encourage some sane limits for other systems too (maybe just a max length and limited charset?)

Oh, yeah - absolutely. We will work those out as we start adding them in the future. Let's just get the ISO4217 ones done right first. What I am proposing currently only applies to that.

@delapuente

This comment has been minimized.

Copy link

delapuente commented Apr 12, 2017

Why does this throw?

// This throws
const details = {
  total: {
    label: "Total due",
    // FOO50.00
    amount: { currency: "this is no good", value: "50.00" }, 
  }
};
@rsolomakhin

This comment has been minimized.

Copy link
Collaborator

rsolomakhin commented Apr 12, 2017

currency should be either ^[A-Z]{3}$ or a URL.

@adrianhopebailie

This comment has been minimized.

Copy link
Collaborator

adrianhopebailie commented Apr 12, 2017

currency should be either ^[A-Z]{3}$ or a URL.

@rsolomakhin is that correct?

I would have said it must be ^[A-Z]{3}$ because that is the required format for the default currencySystem.

If you want to use anything else you must specify the currencySystem and it should not be urn:iso:std:iso:4217.

Where do you get "or a URL"?

@rsolomakhin

This comment has been minimized.

Copy link
Collaborator

rsolomakhin commented Apr 12, 2017

I think you're right, @adrianhopebailie . The default currency system requires ^[A-Z]{3}$ format.

@zkoch

This comment has been minimized.

Copy link
Contributor

zkoch commented Jul 28, 2017

This feels like a blocker for moving to CR, but I'm not 100% sure on the path forward. Is it to simply validate the currency with the regex mentioned above? @domenic @marcoscaceres

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Jul 28, 2017

Need to check the ES Intl spec (and effectively copy that). Will do that soon.

marcoscaceres added a commit that referenced this issue Jul 31, 2017

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Jul 31, 2017

@zkoch, sent a PR. Won't make the Editor's call this week, so any feedback welcome. Should have tests ready tomorrow also.

@adrianhopebailie

This comment has been minimized.

Copy link
Collaborator

adrianhopebailie commented Sep 18, 2017

This is is referenced as the place to log use cases for currencySystem, which is currently at risk.

Re-opening for that purpose unless the editors want to create a new issue and update the reference in the spec.

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Sep 19, 2017

Oh, my bad. The spec should have referenced #617

@adrianhopebailie

This comment has been minimized.

Copy link
Collaborator

adrianhopebailie commented Sep 19, 2017

@ianbjacobs is this something we can still change in the spec?

@ianbjacobs

This comment has been minimized.

Copy link
Collaborator

ianbjacobs commented Sep 19, 2017

Not in the published TR version, but in the editor's draft, sure.
Ian

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Sep 19, 2017

@ianbjacobs, I thought we could just email @deniak and get it republished without any pain?

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Sep 19, 2017

(i.e., non-normative changes during CR are fine)

@ianbjacobs

This comment has been minimized.

Copy link
Collaborator

ianbjacobs commented Sep 19, 2017

I stand corrected. It looks like we can send a publication request to publish a new
CR version.

Rather than send a volley of (re)publication requests, should we wait a couple of
weeks to see if there are other small fixes?

Ian

@domenic

This comment has been minimized.

Copy link
Collaborator

domenic commented Sep 19, 2017

No, please republish on every commit. If this is currently inefficient, the system needs to get better, and we can help encourage that :)

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Sep 19, 2017

What @domenic said. I'd like to deliberately email for republish on every commit to make the point that CR should also have auto-publish enabled.

@ianbjacobs

This comment has been minimized.

Copy link
Collaborator

ianbjacobs commented Sep 19, 2017

I don't mind sending a publication request (tomorrow my time). Should I take this commit:
7e4a2ca#diff-eacf331f0ffc35d4b482f1d15a887d3b

Ian

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Sep 19, 2017

@marcoscaceres

This comment has been minimized.

Copy link
Member

marcoscaceres commented Sep 25, 2017

🎉 spec now pointing to correct issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.