w3c / payment-request

Payment Request API
https://www.w3.org/TR/payment-request/
Other
489 stars 135 forks source link

Should user agent validate currency? #490

Closed perja12 closed 7 years ago

perja12 commented 7 years ago

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 commented 7 years ago

There is no expectation of UA validation of currency.

perja12 commented 7 years ago

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

marcoscaceres commented 7 years ago

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 commented 7 years ago

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

marcoscaceres commented 7 years ago

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 commented 7 years ago

Also, the reference to ISO4217 should be normative.

marcoscaceres commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

rsolomakhin commented 7 years ago

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

adrianhopebailie commented 7 years ago

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 commented 7 years ago

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

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 commented 7 years ago

Sounds reasonable.

adrianhopebailie commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Why does this throw?

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

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

adrianhopebailie commented 7 years ago

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 commented 7 years ago

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

zkoch commented 7 years ago

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 commented 7 years ago

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

marcoscaceres commented 7 years ago

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

adrianhopebailie commented 7 years ago

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 commented 7 years ago

Oh, my bad. The spec should have referenced https://github.com/w3c/payment-request/issues/617

adrianhopebailie commented 7 years ago

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

ianbjacobs commented 7 years ago

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

marcoscaceres commented 7 years ago

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

marcoscaceres commented 7 years ago

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

ianbjacobs commented 7 years ago

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 commented 7 years ago

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

marcoscaceres commented 7 years ago

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 commented 7 years ago

I don't mind sending a publication request (tomorrow my time). Should I take this commit: https://github.com/w3c/payment-request/commit/7e4a2caeb60593e83e5b471f14ec37b3e1bede54#diff-eacf331f0ffc35d4b482f1d15a887d3b

Ian

marcoscaceres commented 7 years ago

@ianbjacobs, take the top from https://github.com/w3c/payment-request/commits/gh-pages

marcoscaceres commented 7 years ago

🎉 spec now pointing to correct issue.