google-pay / flutter-plugin

Apache License 2.0
143 stars 129 forks source link

Some ApplePay amounts may be wrong in some locales. #84

Open kristoffer-zliide opened 3 years ago

kristoffer-zliide commented 3 years ago

Regarding the use of NSDecimalNumber in https://github.com/google-pay/flutter-plugin/blob/main/pay_ios/ios/Classes/PaymentHandler.swift#L125, please read the Discussion section in https://developer.apple.com/documentation/foundation/nsdecimalnumber/1409902-init.

Should amounts really be strings? What are the odds of users of this plugin will convert their amounts to string correctly? It should always be with a . as decimal separator, right?

JlUgia commented 3 years ago

ACK @kristoffer-zliide. Thank you for bringing it up. My suggestion, for consistency reasons would be to use the decimal point element for all prices. It seems that Double does not account for the locale at instantiation. I'm suggesting the following:

NSDecimalNumber(value: Double(item["amount"] as! String)!)

There's the more explicit alternative to use a number formatter, though I don't think the extra overhead justifies the marginal added clarity. Did you have other ideas in mind?

kristoffer-zliide commented 3 years ago

I don't really know anything about Apple's APIs, sorry. I just thought that since you're representing decimal numbers as strings, there must be bugs in there, and, lo and behold, there was.

Next up are all the bugs people using this plugin will make converting their decimal amounts to strings to pass them to this plugin. How to fix those? I can't help thinking the right way to go is using double for amounts in the API. Then pass it as-is to Apple Pay and take care of using the correct formatting that Google Pay expects (. and exactly two decimal places?)

Note that these bugs probably won't be caught during regular QA, as this issue also demonstrates, since all test devices use a locale where it works, so bugs that cause payment to fail sometimes for some people will be released to the public.

JlUgia commented 3 years ago

This is indeed a situation where there's going to be discrepancies between platforms (Apple Pay using NSDecimalNumber and Google Pay using a String), and hence, there's the opportunity to pick what's more convenient. I personally see reasons for and against both approaches:

Looking at it from the eyes of this plugin, choosing either of them would require converting to/from the types on one of the platforms, so another question to ask is whether it's preferable to convert from a number to a string on Android, or from a string to a number on iOS. They seem equivalent to me.

All in all, it mostly comes down to dev preference on whether to work with numbers or strings when defining a price amount for an item. To me, both options are equally reasonable.

For now, I'll add a PR to take localization out of the equation when converting amounts on iOS. In the meantime, it's worthwhile considering opening up a new discussion on the topic to gather general thoughts/preferences from users of the library.

kristoffer-zliide commented 3 years ago

Just to be clear, the String used in the API is not the string used for display right? The API string needs to be English, whereas the display strings are in the phone's locale. That might lead to more confusion.

JlUgia commented 2 years ago

Your assumption is correct. For now, the API has been updated to remove ambiguity and unexpected results. With that, I feel that the API can benefit from using univocal representations of amounts like cents, or base 10 decimal numbers, so I suggest leaving this thread open for now, and considering taking it to a formal discussion.