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.

Sign up for GitHub

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

Jump to bottom

[FC] Crash after authenticating when R8 full-mode is enabled #6813

Merged
merged 3 commits into from
Jun 1, 2023
Merged

[FC] Crash after authenticating when R8 full-mode is enabled #6813

merged 3 commits into from
Jun 1, 2023

Conversation

carlosmuvi-stripe
Copy link
Collaborator

@carlosmuvi-stripe carlosmuvi-stripe commented Jun 1, 2023 •
edited
Loading

Summary

  • Issue: When enabling R8 full-mode on the host app, a class we use reflection on gets obfuscated resulting on the AuthFlow to fail after authenticating with an institution
  • Solution: Add proguard rules to the SDK to avoid obfuscating this class, and preventing similar classes to be obfuscated in the future. See the rules being added here: https://airbnb.io/mavericks/#/proguard?id=kotlin-reflection-with-proguarddexguardr8

Motivation

📔  [Android] Crash after authenticating when R8 full-mode is enabled*
🌐  BANKCON-7040

Testing

  • Added tests
  • Modified tests
  • Manually verified (We'll be adding release mode e2e tests soon!)

@carlosmuvi-stripe carlosmuvi-stripe added the work-cli Added to pull requests created with #work-cli for usage tracking. label Jun 1, 2023
@carlosmuvi-stripe carlosmuvi-stripe self-assigned this Jun 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

Risky Change

This is considered a risky change because it adjusts the sample app build.gradle, please review carefully.
We've seen issues in the past which resulted in failed builds for merchants. Please make sure the build.gradle change is intended.

By adding the label accept-risky-change to this PR, I acknowledge that I'm changing an example app and have verified that the SDK remains in a shippable state.

@carlosmuvi-stripe carlosmuvi-stripe added the accept-risky-change accept-risky-change label Jun 1, 2023
@carlosmuvi-stripe carlosmuvi-stripe marked this pull request as ready for review June 1, 2023 21:39
@carlosmuvi-stripe carlosmuvi-stripe requested review from a team as code owners June 1, 2023 21:39
@carlosmuvi-stripe carlosmuvi-stripe enabled auto-merge (squash) June 1, 2023 21:42
jaynewstrom-stripe
jaynewstrom-stripe reviewed Jun 1, 2023
View reviewed changes
financial-connections-example/build.gradle Outdated
@@ -27,6 +27,10 @@ android {
groups = "financial-connections"
}
}
release {
minifyEnabled true
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't need this, it should already be handled by

stripe-android/build-configuration/android-application.gradle

Line 40 in 9e65354

shrinkResources true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a187372

financial-connections-example/gradle.properties Outdated
STRIPE_ANDROID_NAMESPACE=com.stripe.android.financialconnections.example
android.enableR8.fullMode=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't need this, it should already be handled by

stripe-android/gradle.properties

Line 16 in 000bc24

android.enableR8.fullMode=true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a187372

financial-connections/consumer-rules.txt

# MavericksViewModel loads the Companion class via reflection and thus we need to make sure we keep
# the name of the Companion object.
-keepclassmembers class ** extends com.airbnb.mvrx.MavericksViewModel {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we upstream this? Ideally we wouldn't need to add these ourselves.

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'll open an issue on Mavericks!

@carlosmuvi-stripe carlosmuvi-stripe merged commit 9f0c95b into master Jun 1, 2023
@carlosmuvi-stripe carlosmuvi-stripe deleted the carlosmuvi/BANKCON-7040/fc-crash-after-authenticating-when-r8-full-mode-is branch June 1, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaynewstrom-stripe jaynewstrom-stripe jaynewstrom-stripe approved these changes

@jameswoo-stripe jameswoo-stripe Awaiting requested review from jameswoo-stripe jameswoo-stripe is a code owner automatically assigned from stripe/android-sdk-reviewers

Assignees

@carlosmuvi-stripe carlosmuvi-stripe

Labels
accept-risky-change accept-risky-change work-cli Added to pull requests created with #work-cli for usage tracking.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@carlosmuvi-stripe @jaynewstrom-stripe