Add VenmoLauncher API

[sarahkoop]

Summary of changes

• Add VenmoLauncher and associated API to decouple VenmoClient from activity result handling
• Remove VenmoClient API for activity result handling

The VenmoFragment demo app integration and v5_MIGRATION_GUIDE show what this new integration pattern looks like. I opted to rename the existing tokenizeVenmoAccount method to requestAuthChallenge, then the merchant launches the auth challenge, and passes that result to a new tokenizeVenmoAccount method (previously internal method called onVenmoResult).

I think keeping a tokenizeVenmoAccount method existing, and adding one step the merchant has to take before it makes most sense, but please weigh in if that feels complicated and we should just do new method names for both.

Checklist

• Added a changelog entry

Authors

• @sarahkoop

[sshropshire]

🔥 ! I have a couple q's that are out of scope I'll add in. This is great!

[sshropshire]

This isn't relevant to this PR, but worth mentioning. I remember from that one discussion a merchant dev mentioned nullability in callbacks being a hinderance. It may be worth considering a single result type for result callbacks so that null checks aren't required.

I recently thought about having an interface type in PPCP e.g. VenmoResult that would allow logic like:

if (result instanceof VenmoSuccessResult) {
  // handle success
} else if (result instanceof VenmoErrorResult) {
  // handle error
}

There are other ways too, like using enums. Just a thought I wanted to share. We have to figure something out on PPCP too. It'll be a good topic to brainstorm on going forward.

[sarahkoop]

Ah yes I remember that feedback - I think a single result type could be a good move. I'll go forward with these API changes with the dual results for now and create a ticket to reconsider the approach and align across all modules

[sshropshire]

More 🧠 🌦️ : I wonder if composing a reference to BraintreeClient within each Launcher would be a good design pattern once ClientTokenProvider is no longer required. It would make a method like venmoLauncher.authenticateVenmoUser() possible. It might be cool, but it might be worth holding off to keep it simple.

[sarahkoop]

Agreed I have a separate ticket to do this across all the modules - I think it would make most merchant integration stories simpler and we can add a params object for customization if needed

[jaxdesmarais]

🔥

[scannillo]

I think of "authentication challenge" as a barrier to protect against unauthorized users, and less so for general login. According to this definition though, a basic password authentication does qualify.

Just food for thought that this language might be confusing since for other flows (3DS) the "challenge" is optional and not required.

[scannillo]

Maybe continue or presentLogin or startLogin?

[sarahkoop]

@sshropshire what are your thoughts on changing up this naming scheme?

I'd be fine with something like venmoClient.presentLogin returning a LoginResult or keeping with this authChallenge naming. Thinking through the other modules if we want consistent naming:

PayPal and Venmo are "login" screens
Google Pay is just the Google Pay dialog so not sure either naming makes sense for that one (same with PayPalNativeCheckout)
3DS is a true auth challenge
Local Payment is likely a "login" screen
SEPA launches a mandate

Maybe it won't make sense to have naming consistency and we should just describe what each flow is doing? I do like the clarity across integrations of a single pattern though

[scannillo]

Yeh, I'm with you on preferring a unified name across modules!

[scannillo]

I have a question which is slightly tangential, but I think could inform the discussion:

For 3DS, we'll still require merchants to call continuePerformVerification in order to proceed with the challenge?

So would that look something like this (in pseudocode/short-hand)?

class MyActivity {
    fun onCreate() {
        threeDSecureLauncher { authChallengeResult ->
            threeDSecureClient.performVerification() {
                threeDSecureClient.continuePerformVerification()
            }
        }
    }

    func onClick() {
        threeDSecureClient.requestAuthChallenge()
    }
}

class MyClass: ThreeDSecureListener {
    fun onThreeDSecureSuccess() {
        // handle result/nonce
    }
}

Would the launcher & client steps be flipped?

[sarahkoop]

In the 3DS case, the performVerification method becomes requestAuthChallenge (or we could leave the performVerification naming as is)

class MyActivity {
    fun onCreate() {
        threeDSecureLauncher = ThreeDSecureLauncher(this) { authChallengeResult ->
              threeDSecureClient.continuePerformVerification() { nonce, error -> 
                  // handle nonce/error
             }
        }
    }

    func onClick() {
        threeDSecureClient.requestAuthChallenge(this, request) { authChallenge, error -> 
          threeDSecureLauncher.launch(authChallenge)
        }
    }
}

[sarahkoop]

I've added a ticket to re-asses the naming of the requestAuthChallenge method and make sure it makes sense across modules so I'm going to leave it as is in this PR

[sshropshire]

I'm all for merchants having an intuitive interface. It's a good callout to try and improve naming whenever we can!

I don't have a major preference, but with this new design pattern the Launcher will be presenting the login flow instead of the Client. We talked about potentially allowing the Launcher to have a reference to the Client, that would make methods like launcher.startLogin() possible.

The current naming is tightly coupled to the underlying Android frameworks we use. The way it is now helps us a lot more than it'll help merchants. We should iterate on naming before GA to maximize API clarity.

[scannillo]

Just an idea - but toward the end of our iOS V6 work, we realized that we could create code diffs in Markdown snippets (like this in the 3DS Section). It's pretty neat and really clear for what lines of code in a sample integration have to change.

[sarahkoop]

Very cool - added that!

[scannillo]

Question - is fragment.getViewLifecycleOwner() something that would be acceptable for a merchant to do? Just curious if we need to offer 2 VenmoLauncher inits, or if doing so prevents issues.

[sarahkoop]

Yes merchants can call that method!

But if we change the parameters of the method to VenmoLauncher(FragmentActivity activity, LifecycleOwner lifecycleOwner, VenmoAuthChallengeResultCallback callback) it gets a little weird because merchants that are instantiating the class in an activity would call VenmoLauncher(this, this, callback) and pass in their activity twice, and merchants instantiating in a fragment would call VenmoLauncher(getActivity(), fragment.getViewLifeCycleOwner(), callback) - where they shouldn't pass in the activity twice.

It introduces the possibility that if they pass in any other LifecycleOwner that isn't their fragment's view lifecycle owner (i.e if they pass in their activity twice), it wont work. I think there are too many other options for LifecycleOwner that it adds more complication than just having to constructors.

[scannillo]

That's really helpful, thanks Sarah!

[scannillo]

👏 This is super clear

[scannillo]

No blockers, just a few questions!