[#1533] Flexa integration #1588

Milan-Cerovsky · 2024-09-16T13:56:24Z

Author

Self-review your own code in GitHub's web interface¹
Add automated tests as appropriate
Update the manual tests² as appropriate
Check the code coverage³ report for the automated tests
Update documentation as appropriate (e.g README.md, Architecture.md, CHANGELOG.md, etc.)
Run the app and try the changes
Pull in the latest changes from the main branch and squash your commits before assigning a reviewer⁴

Note
It is good practice to provide before and after UI screenshots in the description of this PR. This is only applicable for changes that modify the UI.

Reviewer

Check the code with the Code Review Guidelines checklist
Perform an ad hoc review⁵
Review the automated tests
Review the manual tests
Review the documentation, README.md, Architecture.md, etc. as appropriate
Run the app and try the changes⁶

Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs. ↩
While we aim for automated testing of the application, some aspects require manual testing. If you had to manually test something during development of this pull request, write those steps down. ↩
While we are not looking for perfect coverage, the tool can point out potential cases that have been missed. Code coverage can be generated with: ./gradlew check for Kotlin modules and ./gradlew connectedCheck -PIS_ANDROID_INSTRUMENTATION_TEST_COVERAGE_ENABLED=true for Android modules. ↩
Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit. ↩
In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass. ↩
While the CI server runs the app to look for build failures or crashes, humans running the app are more likely to notice unexpected log messages, UI inconsistencies, or bad output data. Perform this step last, after verifying the code changes are safe to run locally. ↩

Closes #1533

HonzaR

Thank you for these changes! I'm sharing a few first ideas in the comments.

HonzaR · 2024-09-20T07:00:15Z

settings.gradle.kts

@@ -135,6 +135,7 @@ dependencyResolutionManagement {
                }
            }
        }
+        mavenLocal()


Why do we need this change, please?

flexa is not on maven, I am instead building their sdk locally lol

HonzaR · 2024-09-20T07:06:32Z

ui-lib/build.gradle.kts

@@ -122,6 +131,9 @@ dependencies {
    implementation(libs.zcash.bip39)
    implementation(libs.zxing)

+    api("com.flexa:core:1.0.1")
+    api("com.flexa:spend:1.0.1")


Can we use the same dependency approach as we do elsewhere, injecting them via settings.gradle?

HonzaR · 2024-09-20T07:11:30Z

...android-lib/src/main/java/co/electriccoin/zcash/preference/InstallationPreferenceProvider.kt

What's the reason for creating a new preference provider and not using the existing one, e.g., EncryptedPreferenceProvider?

HonzaR · 2024-09-20T07:12:53Z

ui-lib/src/main/java/co/electriccoin/zcash/ui/common/repository/BalanceRepository.kt

+
+interface BalanceRepository {
+    /**
+     * A flow of the wallet balances state used for the UI layer. It's computed form [WalletSnapshot]'s properties


Typo form -> from

HonzaR · 2024-09-20T07:21:43Z

ui-lib/src/main/java/co/electriccoin/zcash/ui/common/repository/FlexaRepository.kt

+    private suspend fun createFlexaAccount(zecBalance: Double) =
+        AppAccount(
+            accountId = getFlexaAccountId(),
+            displayName = "My Wallet",


Is this our preferred displayName value for each user?

I dont know yet :D

HonzaR · 2024-09-20T07:24:43Z

ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/home/view/HomeView.kt

@@ -112,7 +112,6 @@ private fun HomeContent(
            key = { index ->
                subScreens[index].title
            },
-            beyondBoundsPageCount = 1,


What is the intention behind this change, please?

Flexa uses newer material 3 -> and we see it transitively. We need to exclude it probably

HonzaR · 2024-09-20T07:27:58Z

...rc/main/java/co/electriccoin/zcash/ui/screen/integrations/viewmodel/IntegrationsViewModel.kt

+
+            when {
+                appId.isEmpty() && isDebug ->
+                    coinbaseNavigationCommand.emit("https://www.coinbase.com") // fallback debug url


Can we move these hardcoded urls to constants?

HonzaR · 2024-09-20T07:31:27Z

ui-lib/src/main/res/ui/integrations/values/strings.xml

+    <string name="integrations_coinbase_subtitle">A hassle-free way to buy <xliff:g id="currency" example="ZEC">%1$s</xliff:g> and get it directly into your Zashi wallet.</string>
+    <string name="integrations_flexa">Pay with Flexa</string>
+    <string name="integrations_flexa_subtitle">Pay with Flexa payment clips and explore a new way of spending Zcash.</string>
+    <string name="integrations_version">Version %s</string>


Let's use xliff instead here.

# Conflicts: # ui-design-lib/src/main/java/co/electriccoin/zcash/ui/design/component/ZashiSettingsListItem.kt # ui-design-lib/src/main/java/co/electriccoin/zcash/ui/design/theme/ZashiColors.kt # ui-lib/src/main/java/co/electriccoin/zcash/di/RepositoryModule.kt # ui-lib/src/main/java/co/electriccoin/zcash/di/UseCaseModule.kt # ui-lib/src/main/java/co/electriccoin/zcash/di/ViewModelModule.kt # ui-lib/src/main/java/co/electriccoin/zcash/ui/Navigation.kt # ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/advancedsettings/AdvancedSettingsState.kt # ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/advancedsettings/view/AdvancedSettingsView.kt # ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/advancedsettings/viewmodel/AdvancedSettingsViewModel.kt # ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/settings/view/SettingsView.kt

[#1533] Flexa integration

02d8730

Closes #1533

Milan-Cerovsky self-assigned this Sep 16, 2024

[#1533] Design update

aac4b61

Closes #1533

HonzaR reviewed Sep 20, 2024

View reviewed changes

Milan-Cerovsky added 3 commits October 4, 2024 17:41

[#1533] Code cleanup

db63a71

[#1533] Code cleanup

c431674

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1533] Flexa integration #1588

[#1533] Flexa integration #1588

Milan-Cerovsky commented Sep 16, 2024 •

edited by HonzaR

Loading

HonzaR left a comment

HonzaR Sep 20, 2024

Milan-Cerovsky Oct 4, 2024

HonzaR Sep 20, 2024

HonzaR Sep 20, 2024

HonzaR Sep 20, 2024

HonzaR Sep 20, 2024

Milan-Cerovsky Oct 4, 2024

HonzaR Sep 20, 2024

Milan-Cerovsky Oct 4, 2024

HonzaR Sep 20, 2024

HonzaR Sep 20, 2024

[#1533] Flexa integration #1588

Are you sure you want to change the base?

[#1533] Flexa integration #1588

Conversation

Milan-Cerovsky commented Sep 16, 2024 • edited by HonzaR Loading

Author

Reviewer

Footnotes

HonzaR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Milan-Cerovsky commented Sep 16, 2024 •

edited by HonzaR

Loading