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

Launch Link Eagerly with new ConfirmationDefinition #9940

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

toluo-stripe
Copy link
Contributor

@toluo-stripe toluo-stripe commented Jan 19, 2025

Summary (Proof of Concept)

This is PR adds link_express as a new confirmation option. This is the scenario where Link is launched eagerly as 2FA dialog (see screenshot below).

link_express will be launched with a LinkAccount, and after the 2FA is done, this confirmation definition returns a NextStep to launch native_link with an updated LinkAccount.

Why are we passing LinkAccount around? LinkAccountManager is scoped to the module that uses it, so the Link auth state in paymentsheet does not exist in NativeLink. We initialize LinkAccountManager with a LinkAccount if provided.

Why a new ConfirmationDefinition?
I chose to do it this way because I want to keep LinkActivity (native link) simple. This eager flow would add more complexity and require at least a small refactor because of the translucent background and other small things like navigation. I'm open to other ideas.

Motivation

Testing

  • Added tests
  • Modified tests
  • Manually verified

Screenshots

Screen.Recording.2025-01-21.at.9.30.41.AM.mov

Changelog

Copy link
Contributor

github-actions bot commented Jan 19, 2025

Diffuse output:

OLD: identity-example-release-base.apk (signature: V1, V2)
NEW: identity-example-release-pr.apk (signature: V1, V2)

          │          compressed          │         uncompressed         
          ├───────────┬───────────┬──────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff │ old       │ new       │ diff 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
      dex │     2 MiB │     2 MiB │  0 B │   4.1 MiB │   4.1 MiB │  0 B 
     arsc │     1 MiB │     1 MiB │  0 B │     1 MiB │     1 MiB │  0 B 
 manifest │   2.3 KiB │   2.3 KiB │  0 B │     8 KiB │     8 KiB │  0 B 
      res │ 302.6 KiB │ 302.6 KiB │  0 B │ 456.7 KiB │ 456.7 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   7.1 KiB │   7.1 KiB │  0 B │   6.9 KiB │   6.9 KiB │  0 B 
    other │  90.4 KiB │  90.4 KiB │ +9 B │ 170.7 KiB │ 170.7 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ +9 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19975 │ 19975 │ 0 (+0 -0) 
   types │  6193 │  6193 │ 0 (+0 -0) 
 classes │  4985 │  4985 │ 0 (+0 -0) 
 methods │ 29820 │ 29820 │ 0 (+0 -0) 
  fields │ 17538 │ 17538 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3624 │ 3624 │  0
APK
   compressed    │   uncompressed   │                                           
──────────┬──────┼───────────┬──────┤                                           
 size     │ diff │ size      │ diff │ path                                      
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 28.5 KiB │ +6 B │  63.1 KiB │  0 B │ ∆ META-INF/CERT.SF                        
 25.4 KiB │ +4 B │    63 KiB │  0 B │ ∆ META-INF/MANIFEST.MF                    
    271 B │ -1 B │     120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 54.2 KiB │ +9 B │ 126.2 KiB │  0 B │ (total)

@toluo-stripe toluo-stripe changed the title Link express args Launch Link Eagerly with ConfirmationHandler Jan 21, 2025
@toluo-stripe toluo-stripe changed the title Launch Link Eagerly with ConfirmationHandler Launch Link Eagerly with new ConfirmationDefinition Jan 21, 2025
@samer-stripe
Copy link
Collaborator

I think this makes sense as a confirmation definition since we are expecting this to be a payment flow that immediately completes PaymentSheet.

A side note, I think this is the first case of a three-step confirmation flow we have in the repo.

Comment on lines +166 to +183
@Provides
@NativeLinkScope
fun provideLinkAccountManager(
configuration: LinkConfiguration,
linkRepository: LinkRepository,
linkEventsReporter: LinkEventsReporter,
errorReporter: ErrorReporter,
linkAccount: LinkAccount?
): LinkAccountManager {
return DefaultLinkAccountManager(
config = configuration,
linkRepository = linkRepository,
linkEventsReporter = linkEventsReporter,
errorReporter = errorReporter
).apply {
setAccount(linkAccount)
}
}
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 is a design question mark. I feel like it's safe but would love to discuss.

val isVerified: Boolean = consumerSession.containsVerifiedSMSSession() ||
consumerSession.isVerifiedForSignup()

@IgnoredOnParcel
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm kinda surprised that all these fields are ignored on parcel -- how do we actually use the LinkAccount if none of the fields are parcelized? Or am I misunderstanding?

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.

3 participants