Skip to content

Accept Apple Pay amounts in minor units - #53

Draft
Max Harrison (maxharrison) wants to merge 2 commits into
mainfrom
ios-amount-minor-units
Draft

Accept Apple Pay amounts in minor units#53
Max Harrison (maxharrison) wants to merge 2 commits into
mainfrom
ios-amount-minor-units

Conversation

@maxharrison

Copy link
Copy Markdown
Contributor

Why

Amount only accepted a major-unit decimal string, so converting from minor units was left to merchant code. The web SDKs take minor-unit integers, so a merchant integrating both handles the same charge two different ways, and the exponent logic (JPY has no minor unit, KWD has three) sits in their code rather than ours. Nothing validated the string either: NSDecimalNumber(string:) returns NaN for an unparseable value, and PassKit raises on a NaN summary item, so a typo surfaced as a crash when the sheet was presented.

Part of the cross-SDK amount work. Android is #52, web is evervault/evervault-js#977 and #978.

How

  • Amount now holds either a decimal or a minor-unit integer, resolved against the transaction's currency when the request is built. That way there is one currency and it is always the transaction's, rather than an Amount carrying a currency that could disagree with the transaction it belongs to.
  • Amount(minorUnits: 5499) is the new entry point. The exponent comes from NumberFormatter in .currency style, and NSDecimalNumber(mantissa:exponent:isNegative:) keeps the conversion exact.
  • Amount("54.99") is unchanged. An unparseable string is now rejected when the transaction is constructed, throwing the new EvervaultError.InvalidAmountError. That boundary already throws and already validates currency and country, so no merchant call site needs a new try.
  • Negative amounts are allowed, since discount line items are legitimate on Apple Pay. Android rejects them, because Google's totalPrice format does not permit them.
  • Tests for both cases, negatives, and the validation.

Source-breaking in one spot: Amount.amount is replaced by resolve(currency:), since the value now depends on the currency. The Demo app is updated to match. Construction is unchanged, which is what merchant code mostly does.

I could not build this locally (no Xcode on this machine, and PassKit is iOS-only) — relying on CI.

@ana-maksimovskikh ana-maksimovskikh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Max!
Approved with the nitties nit possible

formatter.numberStyle = .currency
formatter.currencyCode = currency
return NSDecimalNumber(
mantissa: UInt64(abs(minorUnits)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe
mantissa: UInt64(minorUnits.magnitude),?

Almost nonexistent opportunity for a bug here 🤡

minorUnits is a plain Int (signed, 64-bit). Signed integers are asymmetric: the range is -9223372036854775808 (Int.min) to 9223372036854775807 (Int.max) — one more negative value than positive.

abs(x) is implemented as roughly "if negative, negate it." For every value except Int.min, that's fine. For Int.min specifically, the negation itself overflows.

So -Int.min would need to equal 9223372036854775808, which is one larger than Int.max and literally cannot be represented as an Int.

Int.magnitude returns the absolute value as UInt, whose range fully covers Int.min's magnitude, so it never overflows:

case let .decimal(decimal):
return !decimal.decimalValue.isNaN
case .minorUnits:
return true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe
return minorUnits > Int.min

Same reason as below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants