native link dagger setup #9465
native link dagger setup #9465
Conversation
|
Diffuse output: APKMANIFESTDEX |
|
Risky Change This is considered a risky change because it adjusts the sample app build.gradle, please review carefully. By adding the label |
toluo-stripe
force-pushed
the
tolu/link_dagger_setup
branch
from
5a34cfb to
c58d6e9
Compare
| * retained component) to the factory to facilitate its creation via dependency injection. | ||
| */ | ||
| @Composable | ||
| internal inline fun <reified T : ViewModel> paneViewModel( |
There was a problem hiding this comment.
ViewModel creation logic and general DI setup was inspired by financial-connections
There was a problem hiding this comment.
thanks! This is useful context. out of curiosity, do you know why this is called "pane" view model?
There was a problem hiding this comment.
And is this needed in this PR? It looks unused so far
There was a problem hiding this comment.
There's no specific reason for the name, I'll change it to linkViewModel. I'll remove it from this PR since we're not creating any viewmodel at the moment. It's going to be useful for the follow up, which is the sign up UI
| * retained component) to the factory to facilitate its creation via dependency injection. | ||
| */ | ||
| @Composable | ||
| internal inline fun <reified T : ViewModel> paneViewModel( |
There was a problem hiding this comment.
thanks! This is useful context. out of curiosity, do you know why this is called "pane" view model?
| * retained component) to the factory to facilitate its creation via dependency injection. | ||
| */ | ||
| @Composable | ||
| internal inline fun <reified T : ViewModel> paneViewModel( |
There was a problem hiding this comment.
And is this needed in this PR? It looks unused so far
| val savedStateHandle: SavedStateHandle = createSavedStateHandle() | ||
| val app = this[APPLICATION_KEY] as Application | ||
| val args: NativeLinkArgs = | ||
| requireNotNull(getArgs(savedStateHandle)) |
There was a problem hiding this comment.
how can we ensure that the args aren't null here when the savedStateHandle was just created? Where are the args set?
There was a problem hiding this comment.
The createSavedStateHandle will obtain arguments from the viewModelStoreOwner, which is LinkActivity in this case. The args were set here. The LinkActivityContractTest will make sure these args are always set.
There was a problem hiding this comment.
FWIW, we shouldn't do these kinds of things. Merchants get mad at us when they see crashes when pen testing tools launch these.
We should add a test that the activity is finished without throwing in the case we don't get the arguments we expect.
link/src/main/java/com/stripe/android/link/ui/signup/SignUpViewModel.kt
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,6 @@ | |||
| package com.stripe.android.link | |||
|
|
|||
| object NativeLinkEnabled { | |||
There was a problem hiding this comment.
Are there any security concerns with including this in the SDK? I think long-term we wouldn't want this setting to live client-side, because I think an app could theoretically access and manipulate this data. Are there similar concerns about exposing it now while testing?
If we are ok with adding this, it should definitely not be public
There was a problem hiding this comment.
Yh, this should be restricted to library group, my bad. I'll update it.
There was a problem hiding this comment.
This should probably be a feature flag instead. We have the ability to not allow setting it to true when in release mode.
There was a problem hiding this comment.
This will be a flag in elementSession, but that's not ready yet. We'll also need a signal from the attestation module to tell us if hardware attestation is available. That's also not ready yet. I added this as a stop gap to manually enable native link. It will evolve into a proper gate when the pieces mentioned above are available.
Is this still a risk if it's restricted to the library group? If yes, I can take it out and hardcode the launcher when I'm testing or creating a test build.
There was a problem hiding this comment.
You can use a local feature flag for now, which will prevent anything happening in published versions of the SDK.
| @@ -32,7 +33,7 @@ class LinkActivityContractTest { | |||
|
|
|||
| @Test | |||
| fun `LinkActivityContract creates intent with URL with native link disabled`() { | |||
| NativeLinkEnabled.enabled = false | |||
| FeatureFlags.nativeLinkEnabled.setEnabled(false) | |||
There was a problem hiding this comment.
We have a test rule you should use (so that these don't leak to other tests).
|
|
||
| @VisibleForTesting | ||
| internal lateinit var navController: NavHostController | ||
|
|
||
| @Suppress("SwallowedException") |
There was a problem hiding this comment.
we can also just log the exception and then we don't have to suppress this here
| try { | ||
| viewModel = ViewModelProvider(this, LinkActivityViewModel.factory())[LinkActivityViewModel::class.java] | ||
| } catch (e: NoArgsException) { | ||
| setResult(Activity.RESULT_CANCELED) |
There was a problem hiding this comment.
Why is this canceled instead of being a failure?
There was a problem hiding this comment.
I followed the pattern here, but I'm happy to change it to a failure
| internal var viewModelFactory: ViewModelProvider.Factory = LinkActivityViewModel.Factory() | ||
| private val viewModel: LinkActivityViewModel by viewModels { viewModelFactory } | ||
| internal class LinkActivity : ComponentActivity() { | ||
| internal var viewModel: LinkActivityViewModel? = null |
There was a problem hiding this comment.
can you make this a lateinit variable? I think then you won't need to make the type nullable
There was a problem hiding this comment.
I made it nullable because onDestroy will get called when we finish the activity and viewModel.unregisterActivity() will crash since it's not initialized
| @@ -7,6 +7,7 @@ import com.stripe.android.core.BuildConfig | |||
| @Suppress("unused") | |||
There was a problem hiding this comment.
you can probably remove this suppression now that this is being used!
toluo-stripe
force-pushed
the
tolu/link_dagger_setup
branch
from
edf484e to
56ca299
Compare
| internal var viewModelFactory: ViewModelProvider.Factory = LinkActivityViewModel.Factory() | ||
| private val viewModel: LinkActivityViewModel by viewModels { viewModelFactory } | ||
| internal class LinkActivity : ComponentActivity() { | ||
| internal var viewModel: LinkActivityViewModel? = null |
| try { | ||
| viewModel = ViewModelProvider(this, LinkActivityViewModel.factory())[LinkActivityViewModel::class.java] | ||
| } catch (e: NoArgsException) { | ||
| Log.e(TAG, "Failed to create LinkActivityViewModel", e) |
There was a problem hiding this comment.
can you use our Logger class here instead of Log? Logger helps us avoid creating noisy logs in user apps.
1 build increased size, 2 builds decreased size
Stripe Identity Example 20.52.1-theme1 (20)
|
| Item | Install Size Change | Download Size Change |
|---|---|---|
| androidx.compose.ui.input.key.KeyEvent_androidKt | ⬆️ 28.5 kB | ⬆️ 13.7 kB |
| android.support.v4.media.session.MediaSessionCompat | ⬇️ -18.3 kB | ⬇️ -8.8 kB |
| 🗑 kotlin.coroutines.intrinsics.IntrinsicsKt__IntrinsicsJvmKt | ⬇️ -17.5 kB | ⬇️ -8.4 kB |
| kotlin.math.MathKt | ⬇️ -15.9 kB | ⬇️ -7.6 kB |
| 🗑 androidx.appcompat.app.ActionBar | ⬇️ -15.0 kB | ⬇️ -7.2 kB |
View Treemap
Financial Connections Example 20.52.1 (205201)
com.stripe.android.financialconnections.example
⚖️ Compare build
⏱️ Analyze build performance
Total install size change: ⬇️ 13.9 kB (-0.15%)
Total download size change: ⬇️ 2.2 kB (-0.05%)
Largest size changes
| Item | Install Size Change | Download Size Change |
|---|---|---|
| 📝 com.stripe.android.financialconnections.features.linkaccountpicke... | ⬆️ 53.2 kB | ⬆️ 25.2 kB |
| 🗑 _COROUTINE | ⬇️ -50.4 kB | ⬇️ -23.8 kB |
| 📝 okhttp3.internal.platform.BouncyCastlePlatform$Companion | ⬆️ 47.0 kB | ⬆️ 22.2 kB |
| 🗑 com.stripe.android.model.parsers.SourceJsonParser$KlarnaJsonParse... | ⬇️ -41.1 kB | ⬇️ -19.4 kB |
| com.google.android.material.sidesheet.LeftSheetDelegate | ⬆️ 36.5 kB | ⬆️ 17.2 kB |
View Treemap
PaymentSheet Example 20.52.1 (11)
com.stripe.android.paymentsheet.example
⚖️ Compare build
⏱️ Analyze build performance
Total install size change: ⬆️ 217.6 kB (1.35%)
Total download size change: ⬆️ 107.7 kB (1.26%)
Largest size changes
| Item | Install Size Change | Download Size Change |
|---|---|---|
| androidx.transition.FragmentTransitionSupport$1 | ⬇️ -49.7 kB | ⬇️ -22.7 kB |
| 📝 androidx.camera.core.impl.utils.MainThreadAsyncHandler | ⬆️ 42.8 kB | ⬆️ 19.6 kB |
| 📝 com.stripe.android.paymentsheet.ui.PaymentSheetScreenKt | ⬆️ 41.6 kB | ⬆️ 19.0 kB |
| 📝 com.stripe.android.paymentsheet.ui.PaymentMethodFormKt | ⬆️ 33.3 kB | ⬆️ 15.2 kB |
| kotlin.math.MathKt | ⬇️ -30.9 kB | ⬇️ -14.1 kB |
View Treemap
🛸 Powered by Emerge Tools
Comment trigger: Size diff threshold of 100.00kB exceeded
Summary
native link dagger setup
Motivation
This PR adds the dagger setup necessary to launch the native link flow.
Part of JIRA
Screen.Recording.2024-10-18.at.6.15.15.PM.mov
Testing
Changelog