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

Instant Debits: first iteration of Instant Debits in Payment Sheet #3528

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kgaidis-stripe
Copy link
Contributor

@kgaidis-stripe kgaidis-stripe commented Apr 19, 2024

Summary

This PR implements the first iteration of Instant Debits in the Payment Sheet. The PR:

  1. Adds a new payment method (instant debits) so we could display a new "Bank Account" payment method in the payment sheet
  2. If "Bank Account" (Instant Debits) is selected, the code displays UI, and routes through Payment Sheet and Financial Connections

⚠️ Some important notes for reviewing:

  • This is just the first iteration. Despite Android equivalent already being landed, we still have a lot of things to resolve and improve before launching to merchants: handle flags for rollout, design, testing, etc.

Testing

See two videos:

PaymentIntent Succeeds

payment_intent.mp4

SetupIntent Succeeds

setup_intent.mp4

Financial Connections Still Works

https://app.bitrise.io/build/30cd250d-0695-4af1-bd81-5c5de2f882f3

FinancialConnectionsNetworkingUITests
    ✓ testNativeNetworkingTestMode (111.850 seconds)
    ✓ testNativeNetworkingTestModeSignUpWithMultiSelectAndPrefilledEmail (30.682 seconds)
FinancialConnectionsUITests
    ✓ testDataLiveModeOAuthNativeAuthFlow (29.338 seconds)
    ✓ testDataLiveModeOAuthWebAuthFlow (18.610 seconds)
    ✓ testDataTestModeOAuthNativeAuthFlow (18.501 seconds)
    ✓ testPaymentSearchInLiveModeNativeAuthFlow (26.620 seconds)
    ✓ testPaymentTestModeLegacyNativeAuthFlow (20.153 seconds)
    ✓ testPaymentTestModeManualEntryNativeAuthFlow (24.347 seconds)

@kgaidis-stripe kgaidis-stripe force-pushed the kg-instadeb branch 2 times, most recently from 11c541c to 0d9cc61 Compare April 22, 2024 20:32
Comment on lines +159 to +164
if case .new(let confirmParams) = paymentOption {
if let paymentMethodId = confirmParams.instantDebitsLinkedBank?.paymentMethodId {
params.paymentMethodId = paymentMethodId
params.paymentMethodParams = nil
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might need updating

Comment on lines +186 to +199
if case .new(let confirmParams) = paymentOption {
if let paymentMethodId = confirmParams.instantDebitsLinkedBank?.paymentMethodId {
setupIntentParams.paymentMethodID = paymentMethodId
setupIntentParams.paymentMethodParams = nil

let mandateCustomerAcceptanceParams = STPMandateCustomerAcceptanceParams()
let onlineParams = STPMandateOnlineParams(ipAddress: "", userAgent: "")
// Tell Stripe to infer mandate info from client
onlineParams.inferFromClient = true
mandateCustomerAcceptanceParams.onlineParams = onlineParams
mandateCustomerAcceptanceParams.type = .online
setupIntentParams.mandateData = STPMandateDataParams(customerAcceptance: mandateCustomerAcceptanceParams)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might need updating

Comment on lines 751 to 724
emailElement: (
// TODO: does this email logic make sense?
(configuration.billingDetailsCollectionConfiguration.email != .never)
? makeEmail()
: nil
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might need updates

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you needed an email address: Is that not the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidme-stripe

I wasn't sure what the logic is (billingDetailsCollectionConfiguration).

I'll just make this logic be makeEmail() though correct me if that's wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends — if you always need an email address, I guess we technically would want to hide Instant Debits entirely if billingDetailsCollectionConfiguration.email == .never. But I'm okay with it as makeEmail() for now

Comment on lines 102 to 107
// Fail loudly: This is an integration error
assert(
// TODO: is it a mandatory step to provide an email???
(willShowEmailField || hasDefaultEmail),
"If email is not collected, they must be provided through defaults"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this logic make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleting it

@@ -742,7 +742,8 @@ public class STPPaymentHandler: NSObject {
.konbini,
.promptPay,
.swish,
.twint:
.twint,
.instantDebits:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so!

@@ -1299,6 +1300,8 @@ extension STPPaymentMethodParams {
return "Revolut Pay"
case.twint:
return "TWINT"
case .instantDebits:
return "Bank Account"
Copy link
Contributor

Choose a reason for hiding this comment

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

We have many things named bank account, not sure if we're worried about confusion or if this should look the same as an ACHv2 to the consumer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea agree; many parts, particularly design, are still going to need iterations/feedback from design

There is no public API for this first iteration (everything is behind the scenes), so Android has been merging PR's and iterating

Copy link

emerge-tools bot commented May 7, 2024

⚠️ 5 new unused protocols, 6 builds increased size

Name Version Download Change Install Change Approval
StripeApplePaySize
com.stripe.StripeApplePaySize
1.0 (1) 422.3 kB ⬆️ 240 B (0.06%) 1.5 MB ⬆️ 1.7 kB (0.11%) N/A
StripePaymentsSize
com.stripe.StripePaymentsSize
1.0 (1) 1.1 MB ⬆️ 856 B (0.08%) 3.9 MB ⬆️ 2.6 kB (0.07%) N/A
StripePaymentSheetSize
com.stripe.StripePaymentSheetSize
1.0 (1) 3.2 MB ⬆️ 10.8 kB (0.34%) 9.6 MB ⬆️ 27.3 kB (0.29%) N/A
StripeSize
com.stripe.StripeSize
1.0 (1) 2.3 MB ⬆️ 1.3 kB (0.05%) 7.6 MB ⬆️ 2.6 kB (0.03%) N/A
StripePaymentsUISize
com.stripe.StripePaymentsUISize
1.0 (1) 1.8 MB ⬆️ 1.3 kB (0.07%) 6.1 MB ⬆️ 3.0 kB (0.05%) N/A
StripeFinancialConnectionsSize
com.stripe.StripeFinancialConnectionsSize
1.0 (1) 1.2 MB ⬆️ 9.8 kB (0.84%) 3.9 MB ⬆️ 23.5 kB (0.61%) N/A

StripeApplePaySize 1.0 (1)
com.stripe.StripeApplePaySize

⚠️ 2 new unused protocols: FinancialConnectionsLinkedBank and InstantDebitsLinkedBank
⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 1.7 kB (0.11%)
Total download size change: ⬆️ 240 B (0.06%)

Largest size changes

Item Install Size Change
📝 StripeCore.FinancialConnectionsSDKResult.Completed.value witness ⬆️ 624 B
Other ⬆️ 1.0 kB
View Treemap

Image of diff

StripePaymentsSize 1.0 (1)
com.stripe.StripePaymentsSize

⚠️ Found new unused protocol: InstantDebitsLinkedBank
⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 2.6 kB (0.07%)
Total download size change: ⬆️ 856 B (0.08%)

Largest size changes

Item Install Size Change
🗑 StripePayments.StubbedLinkedBank ⬇️ -1.2 kB
📝 StripePayments.StubbedFinancialConnectionsLinkedBank.value witnes... ⬆️ 740 B
📝 StripeCore.FinancialConnectionsSDKResult.Completed.value witness ⬆️ 624 B
StripePayments.STPAPIClient.linkAccountSessions(endpoint,clientSe... ⬆️ 536 B
Other ⬆️ 1.9 kB
View Treemap

Image of diff

StripePaymentSheetSize 1.0 (1)
com.stripe.StripePaymentSheetSize

⚠️ Found new unused protocol: InstantDebitsLinkedBank
⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 27.3 kB (0.29%)
Total download size change: ⬆️ 10.8 kB (0.34%)

Largest size changes

Item Install Size Change
📝 StripePaymentSheet.AddPaymentMethodViewController.handleCollectIn... ⬆️ 2.7 kB
📝 StripePaymentSheet.PaymentSheetFormFactory.makeInstantDebits ⬆️ 2.0 kB
📝 StripePaymentSheet.InstantDebitsPaymentMethodElement.didTapXIcon ⬆️ 1.5 kB
📝 StripePaymentSheet.InstantDebitsPaymentMethodElement.init(configu... ⬆️ 1.5 kB
🗑 StripePayments.StubbedLinkedBank ⬇️ -1.2 kB
View Treemap

Image of diff

StripeSize 1.0 (1)
com.stripe.StripeSize

⚠️ Found new unused protocol: InstantDebitsLinkedBank
⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 2.6 kB (0.03%)
Total download size change: ⬆️ 1.3 kB (0.05%)

Largest size changes

Item Install Size Change
🗑 StripePayments.StubbedLinkedBank ⬇️ -1.2 kB
📝 StripePayments.StubbedFinancialConnectionsLinkedBank.value witnes... ⬆️ 740 B
📝 StripeCore.FinancialConnectionsSDKResult.Completed.value witness ⬆️ 624 B
StripePayments.STPAPIClient.linkAccountSessions(endpoint,clientSe... ⬆️ 536 B
Other ⬆️ 1.9 kB
View Treemap

Image of diff

StripePaymentsUISize 1.0 (1)
com.stripe.StripePaymentsUISize

⚠️ Found new unused protocol: InstantDebitsLinkedBank
⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 3.0 kB (0.05%)
Total download size change: ⬆️ 1.3 kB (0.07%)

Largest size changes

Item Install Size Change
🗑 StripePayments.StubbedLinkedBank ⬇️ -1.2 kB
📝 StripePayments.StubbedFinancialConnectionsLinkedBank.value witnes... ⬆️ 740 B
📝 StripeCore.FinancialConnectionsSDKResult.Completed.value witness ⬆️ 624 B
StripePayments.STPAPIClient.linkAccountSessions(endpoint,clientSe... ⬆️ 536 B
Other ⬆️ 2.3 kB
View Treemap

Image of diff

StripeFinancialConnectionsSize 1.0 (1)
com.stripe.StripeFinancialConnectionsSize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 23.5 kB (0.61%)
Total download size change: ⬆️ 9.8 kB (0.84%)

Largest size changes

Item Install Size Change
📝 StripeFinancialConnections.HostControllerResult.value witness ⬆️ 3.9 kB
📝 StripeFinancialConnections.HostControllerResult.Completed.value w... ⬆️ 3.6 kB
📝 StripeFinancialConnections.FinancialConnectionsWebFlowViewControl... ⬆️ 1.4 kB
StripeFinancialConnections.HostController.hostViewController(didF... ⬆️ 1.4 kB
📝 StripeFinancialConnections.FinancialConnectionsWebFlowViewControl... ⬆️ 1.3 kB
View Treemap

Image of diff


🛸 Powered by Emerge Tools

@kgaidis-stripe kgaidis-stripe force-pushed the kg-instadeb branch 4 times, most recently from 0763e43 to 5eb722c Compare May 8, 2024 14:09
!recommendedStripePaymentMethodTypes.contains(.USBankAccount),
!intent.isDeferredIntent,
intent.linkFlags["link_disable_instant_debits_on_mobile"] != true
// intent.linkFundingSources?.contains(.bankAccount) == true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will need correcting before merging

@kgaidis-stripe kgaidis-stripe changed the title [DRAFT] Instant Debits support Instant Debits: first iteration of Instant Debits in Payment Sheet May 8, 2024
@kgaidis-stripe kgaidis-stripe force-pushed the kg-instadeb branch 3 times, most recently from 6529b1c to 98735d2 Compare May 8, 2024 17:48
Copy link

github-actions bot commented May 8, 2024

⚠️ Public API changes detected:

StripePayments

- get
- }
- @objc public var mobilePay: StripePayments.STPPaymentMethodMobilePay? {
+ case instantDebits
- @objc deinit
- @objc public var allResponseFields: [Swift.AnyHashable : Any] {
- get
- }
- @objc override dynamic public var description: Swift.String {
- @objc get
- }
- @objc public static func decodedObject(fromAPIResponse response: [Swift.AnyHashable : Any]?) -> Self?

StripeCore

+ public enum Name : Swift.String {
+ case open
+ case manualEntryInitiated
+ case consentAcquired
+ case searchInitiated
+ case institutionSelected
+ case institutionAuthorized
+ case accountsSelected
+ case success
+ case error
+ case cancel
+ case flowLaunchedInBrowser
+ public init?(rawValue: Swift.String)
+ public typealias RawValue = Swift.String
+ public var rawValue: Swift.String {
+ get
+ }
+ }
+ public struct Metadata {
+ public let dictionary: [Swift.String : Any]
+ public var manualEntry: Swift.Bool? {
+ get
+ }
+ public var institutionName: Swift.String? {
+ get
+ }
+ public var errorCode: StripeCore.FinancialConnectionsEvent.ErrorCode? {
+ get
+ }
+ }
+ public enum ErrorCode : Swift.String {
+ case accountNumbersUnavailable
+ case accountsUnavailable
+ case noDebitableAccount
+ case authorizationFailed
+ case institutionUnavailablePlanned
+ case institutionUnavailableUnplanned
+ case institutionTimeout
+ case unexpectedError
+ case sessionExpired
+ case failedBotDetection
+ public init?(rawValue: Swift.String)
+ public typealias RawValue = Swift.String
+ public var rawValue: Swift.String {
+ get
+ }
+ }
+ public let name: StripeCore.FinancialConnectionsEvent.Name
+ public let metadata: StripeCore.FinancialConnectionsEvent.Metadata
- public enum Name : Swift.String {
- case open
- case manualEntryInitiated
- case consentAcquired
- case searchInitiated
- case institutionSelected
- case institutionAuthorized
- case accountsSelected
- case success
- case error
- case cancel
- case flowLaunchedInBrowser
- public init?(rawValue: Swift.String)
- public typealias RawValue = Swift.String
- public var rawValue: Swift.String {
- get
- }
- }
- public struct Metadata {
- public let dictionary: [Swift.String : Any]
- public var manualEntry: Swift.Bool? {
- get
- }
- public var institutionName: Swift.String? {
- get
- }
- public var errorCode: StripeCore.FinancialConnectionsEvent.ErrorCode? {
- get
- }
- }
- public enum ErrorCode : Swift.String {
- case accountNumbersUnavailable
- case accountsUnavailable
- case noDebitableAccount
- case authorizationFailed
- case institutionUnavailablePlanned
- case institutionUnavailableUnplanned
- case institutionTimeout
- case unexpectedError
- case sessionExpired
- case failedBotDetection
- public init?(rawValue: Swift.String)
- public typealias RawValue = Swift.String
- public var rawValue: Swift.String {
- get
- }
- }
- public let name: StripeCore.FinancialConnectionsEvent.Name
- public let metadata: StripeCore.FinancialConnectionsEvent.Metadata

If you are adding a new public API consider the following:

  • Do these APIs need to be public or can they be protected with @_spi(STP)?
  • If these APIs need to be public, assess whether they require an API review.

If you are modifying or removing a public API:

  • Does this require a breaking version change?
  • Do these changes require API review?

If you confirm these APIs need to be added/updated and have undergone necessary review, add the label modifies public API to this PR to acknowledge and bypass this check.

intent.supportsLink,
!recommendedStripePaymentMethodTypes.contains(.USBankAccount),
!intent.isDeferredIntent,
intent.linkFundingSources?.contains(.bankAccount) == true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for .bankAccount to appear in linkFundingSources has a bunch of extra controls so this if-statement is unlikely to be "accidentally" executed

for example, there's a special flag that needs to be enabled for this mobile Instant Debits feature

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried this might cause older SDK versions to see a half-finished version of Instant Debits when this rolls out more widely. We'll want to make sure to check server-side that the request has the minimum SDK version (the one you end up GA-ing in) or a flag before returning .bankAccount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes great point! We will make sure to account for this (this is quite common in Financial Connections too - and we even have some custom backend code/"library" to handle this type of versioning easier across platforms). It's a little sad our flag system does not seem to support versioning as well.

@@ -445,6 +445,7 @@ extension PlaygroundController {
"use_link": settings.linkEnabled == .on,
"use_manual_confirmation": settings.integrationType == .deferred_mc,
"require_cvc_recollection": settings.requireCVCRecollection == .on,
// "supported_payment_methods": ["card", "link"], // Uncomment to override payment methods (also make sure Automatic PM's is off)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now, this needs to be uncommented for Instant Debits to show up via playground

there's a precedent for it below (😁 ):

// "set_shipping_address": true // Uncomment to make server vend PI with shipping address populated

likely not the end state we want but its OK for now given the state of the project (still iterating on design etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does turning the Link flag on work? That should set [card, link] as the PMs. If this is necessary, might be nice to make it a switch, it's not too difficult to add a new one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidme-stripe

What is the Link flag? Or, in other words, given the current state, what would I change/do to try turning the Link flag on? (also asked some other folks in the meanwhile too)

I do see this (...but doesn't seem to do the intended):

Screenshot 2024-05-17 at 11 25 13 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

That should be it — if it's on, the backend should send [card, link, (a few others)].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidme-stripe

With that on...

in:

static func filteredPaymentMethodTypes(from intent: Intent, configuration: Configuration, logAvailability: Bool = false) -> [PaymentMethodType]

intent.recommendedPaymentMethodTypes returns:

(lldb) po intent.recommendedPaymentMethodTypes
▿ 7 elements
  - 0 : card
  - 1 : link
  - 2 : afterpayClearpay
  - 3 : USBankAccount
  - 4 : klarna
  - 5 : Amazon Pay
  - 6 : affirm

they key problem, as I understand it, is that the "USBankAccount" is returned (where if that exists, Instant Debits is disabled - by intent.linkFundingSources?.contains(.bankAccount) == true NOT being true)

LMK if there's other easy way around this, right now Android added a comma-separated list to the playground app to support this through the UI and the current plan was to just copy it: stripe/stripe-android#8428

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, got it! That makes sense to me.

Comment on lines +35 to +36
// TODO(kgaidis): localize
string = "By continuing, you agree to authorize payments pursuant to <terms>these terms</terms>."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not the end state we want but its OK for now given the state of the project (still iterating on design, API's are private, feature behind a flag etc.)

@kgaidis-stripe kgaidis-stripe marked this pull request as ready for review May 8, 2024 20:22
@kgaidis-stripe kgaidis-stripe requested review from a team as code owners May 8, 2024 20:22
…ts in payment sheet which also connections to financial connections SDK.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants