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

[Connect] Using FinancialConnections SDK in Connect #4159

Merged

Conversation

mludowise-stripe
Copy link
Collaborator

@mludowise-stripe mludowise-stripe commented Oct 18, 2024

Summary

Enables using the mobile native FinancialConnections SDK from Connect web components to retrieve the user's bank token.

  • Adds a dependency from StripeConnect -> StripeFinancialConnections
  • Adds a new JS message handler and sender to trigger the FinancialConnections flow from web and pass back the bank token:
    • openFinancialConnections message handler: Triggers the FinancialConnectionsSheet to open:
      • Param clientSecret: The Financial Connections Session client secret for the connected account
      • Param connectedAccountId: The ID of the connected account
      • Param id: A unique UUID that gets passed back to the web layer on returnFromFinancialConnections
    • returnedFromFinancialConnections message sender: Returns the connected bank token to the web layer
      • Param bankToken: The bank account token of the connected account. This value will be null if the user canceled out of the flow or an error occurred
      • Param id: The id originally passed to the mobile layer from openFinancialConnections

Motivation

https://jira.corp.stripe.com/browse/MXMOBILE-2526
https://docs.google.com/document/d/1WUTvnK8kqsB00zcmUzwVC_CW5MMmXC_CDk8eQTAzaaQ

Testing

CleanShot.2024-12-11.at.16.41.29.mp4

@mludowise-stripe mludowise-stripe changed the title [Connect] Using FinancialConnections SDK in Connect [DRAFT] [Connect] Using FinancialConnections SDK in Connect Oct 18, 2024
@mludowise-stripe mludowise-stripe force-pushed the mludowise/connect_financial_connections_prototype branch from a37d5e1 to 141c9d3 Compare October 18, 2024 18:08
Copy link

⚠️ Public API changes detected:

StripePaymentSheet

- public var paymentMethodLayout: StripePaymentSheet.PaymentSheet.PaymentMethodLayout
- public enum PaymentMethodLayout {
- case horizontal
- case vertical
- case automatic
- public static func == (a: StripePaymentSheet.PaymentSheet.PaymentMethodLayout, b: StripePaymentSheet.PaymentSheet.PaymentMethodLayout) -> Swift.Bool
- public func hash(into hasher: inout Swift.Hasher)
- public var hashValue: Swift.Int {
- get
- }
- }

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.

ℹ️ If this comment appears to be left in error, make sure your branch is up-to-date with master.

@mludowise-stripe mludowise-stripe force-pushed the mludowise/connect_financial_connections_prototype branch from d4937c6 to 5ce1981 Compare October 18, 2024 21:25
Copy link

emerge-tools bot commented Dec 11, 2024

6 builds increased size

Name Version Download Change Install Change Approval
StripeSize
com.stripe.StripeSize
1.0 (1) 2.1 MB ⬆️ 532 B (0.03%) 6.9 MB ⬆️ 1.2 kB (0.02%) N/A
StripeApplePaySize
com.stripe.StripeApplePaySize
1.0 (1) 464.5 kB ⬆️ 621 B (0.13%) 1.6 MB ⬆️ 828 B (0.05%) N/A
StripeFinancialConnectionsSize
com.stripe.StripeFinancialConnectionsSize
1.0 (1) 1.4 MB ⬆️ 384 B (0.03%) 4.4 MB ⬆️ 828 B (0.02%) N/A
StripePaymentsSize
com.stripe.StripePaymentsSize
1.0 (1) 1.2 MB ⬆️ 234 B (0.02%) 4.2 MB ⬆️ 820 B (0.02%) N/A
StripePaymentsUISize
com.stripe.StripePaymentsUISize
1.0 (1) 1.9 MB ⬆️ 612 B (0.03%) 6.4 MB ⬆️ 820 B (0.01%) N/A
StripePaymentSheetSize
com.stripe.StripePaymentSheetSize
1.0 (1) 3.7 MB ⬆️ 721 B (0.02%) 11.0 MB ⬆️ 820 B N/A

StripeSize 1.0 (1)
com.stripe.StripeSize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 1.2 kB (0.02%)
Total download size change: ⬆️ 532 B (0.03%)

Largest size changes

Item Install Size Change
📝 StripeCore.STPAPIClient.makeCopy ⬆️ 804 B
Other ⬆️ 432 B
View Treemap

Image of diff

StripeApplePaySize 1.0 (1)
com.stripe.StripeApplePaySize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 828 B (0.05%)
Total download size change: ⬆️ 621 B (0.13%)

Largest size changes

Item Install Size Change
📝 StripeCore.STPAPIClient.makeCopy ⬆️ 804 B
Other ⬆️ 24 B
View Treemap

Image of diff

StripeFinancialConnectionsSize 1.0 (1)
com.stripe.StripeFinancialConnectionsSize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 828 B (0.02%)
Total download size change: ⬆️ 384 B (0.03%)

Largest size changes

Item Install Size Change
📝 StripeCore.STPAPIClient.makeCopy ⬆️ 804 B
Other ⬆️ 24 B
View Treemap

Image of diff

StripePaymentsSize 1.0 (1)
com.stripe.StripePaymentsSize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 820 B (0.02%)
Total download size change: ⬆️ 234 B (0.02%)

Largest size changes

Item Install Size Change
📝 StripeCore.STPAPIClient.makeCopy ⬆️ 804 B
Other ⬆️ 16 B
View Treemap

Image of diff

StripePaymentsUISize 1.0 (1)
com.stripe.StripePaymentsUISize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 820 B (0.01%)
Total download size change: ⬆️ 612 B (0.03%)

Largest size changes

Item Install Size Change
📝 StripeCore.STPAPIClient.makeCopy ⬆️ 804 B
Other ⬆️ 16 B
View Treemap

Image of diff

StripePaymentSheetSize 1.0 (1)
com.stripe.StripePaymentSheetSize

⚖️ Compare build
⏱️ Analyze build performance

Total install size change: ⬆️ 820 B
Total download size change: ⬆️ 721 B (0.02%)

Largest size changes

Item Install Size Change
📝 StripeCore.STPAPIClient.makeCopy ⬆️ 804 B
Other ⬆️ 16 B
View Treemap

Image of diff


🛸 Powered by Emerge Tools

update comment

comment

Remove returnUrl and use presentForToken

revert accidental change

test coverage

rebase fixes

Add test dependencies

Update financial connections to accept Connected Account Id

makeCopy
@mludowise-stripe mludowise-stripe force-pushed the mludowise/connect_financial_connections_prototype branch from 5556d61 to 78dfb21 Compare December 12, 2024 02:28
@mludowise-stripe mludowise-stripe changed the title [DRAFT] [Connect] Using FinancialConnections SDK in Connect [Connect] Using FinancialConnections SDK in Connect Dec 12, 2024
Comment on lines +56 to +57
authenticatedWebViewManager: AuthenticatedWebViewManager = .init(),
financialConnectionsPresenter: FinancialConnectionsPresenter = .init()
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 starting to second guess whether these should have default values or not. I'm worried that one day we will do a handoff to a new ConnectComponentWebViewController instance and not provide the existing managers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we end up having that future case, then I agree we should stop configuring default values here since it could lead to issues. But given that we don't have that case today and these are only explicitly set for test injection, I'm not too concerned about it right now.

// Make a copy before modifying so we don't unexpectedly modify the shared API client
financialConnectionsSheet.apiClient = apiClient.makeCopy()
financialConnectionsSheet.apiClient.stripeAccount = connectedAccountId
return await withCheckedContinuation { continuation in
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to check to see if we have weighed the pros and cons of checked vs unchecked continuation here. If we stick with checked then this will cause a crash if resume is called more than once. It shouldn't get called more than once in this instance, but maybe we can add some extra protections here to ensure we don't call resume more than once and instead of crashing we log an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't spend too much time thinking about it in this instance since it shouldn't be possible for it to get called more than once. I don't think we have a strong opinion as to which to use the SDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that's ok, I don't think this should be a blocker or anything. Just wanted to call out that we have been bitten by this before. Especially within an SDK it would be ideal to not crash on a programmer error like this.

Copy link
Collaborator

@mats-stripe mats-stripe left a comment

Choose a reason for hiding this comment

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

FC integration looks good to me!

@mludowise-stripe mludowise-stripe merged commit b3d69c9 into master Dec 12, 2024
6 checks passed
@mludowise-stripe mludowise-stripe deleted the mludowise/connect_financial_connections_prototype branch December 12, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants