-
Notifications
You must be signed in to change notification settings - Fork 709
StripeCryptoOnramp: API Review Feedback Part 3 #12296
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
Conversation
crypto-onramp-example/src/main/java/com/stripe/android/crypto/onramp/example/OnrampViewModel.kt
Outdated
Show resolved
Hide resolved
| fun build(): State { | ||
| return State( | ||
| authenticateUserCallback = requireNotNull(authenticateUserCallback) { | ||
| "authenticateUserCallback must not be null" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all of these truly required? Is there zero use case for a merchant not to supply them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A merchant should supply all of them, yes.
crypto-onramp/src/main/java/com/stripe/android/crypto/onramp/model/OnrampConfiguration.kt
Outdated
Show resolved
Hide resolved
crypto-onramp/src/main/java/com/stripe/android/crypto/onramp/OnrampCoordinator.kt
Outdated
Show resolved
Hide resolved
crypto-onramp/src/main/java/com/stripe/android/crypto/onramp/OnrampCoordinator.kt
Outdated
Show resolved
Hide resolved
| authorizeCallback = viewModel::onAuthorizeResult | ||
| ) | ||
| val callbacks = OnrampCallbacks() | ||
| .authenticateUserCallback(callback = viewModel::onAuthenticateUserResult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not for this PR) Given the view model is the one that owns the coordinator, and is the implementor of all of these callbacks, I think it does shine a light on the fact that these should likely just move to the coordinator constructor.
|
Diffuse output: APK |
|
@jaynewstrom-stripe Had to trigger a re-run due to the actions issues yesterday/this morning so I added some missing docs, otherwise nothing changed. |
Summary
Updates OnrampCallbacks and OnrampConfiguration models to be Builder based instead of constructor based (similar to how PaymentMethodMessagingElement internally works).
Motivation
API review comments requested this change. While the nature of OnrampCallbacks is still in flux in the review, if they do end up sticking around, this change was requested.
Testing