Keystone integration #1692

Milan-Cerovsky · 2024-11-22T10:22:53Z

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. ↩

HonzaR

@Milan-Cerovsky please, we need to fix the broken tests first.

HonzaR

I left a few comments inline. Please create separate strings folders and move related texts into them.

HonzaR · 2024-12-01T19:58:17Z

ui-design-lib/src/main/res/ui/common/drawable/ic_app_bar_keystone_icon.png

Can we use vector instead?

it does not export well into a vector :(

HonzaR · 2024-12-01T20:14:45Z

ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/accountlist/view/AccountListView.kt

+    Column {
+        Text(
+            modifier = Modifier.padding(horizontal = 24.dp),
+            text = "Wallets & Hardware",


Let's move all texts into the related strings.xml

HonzaR · 2024-12-01T20:16:42Z

.../src/main/java/co/electriccoin/zcash/ui/screen/accountlist/viewmodel/AccountListViewModel.kt

+                isLoading = isLoading,
+                addWalletButton =
+                    ButtonState(
+                        text = stringRes("Add hardware wallet")


Same comment here.

This button will be removed with upcoming PR

HonzaR · 2024-12-01T20:19:38Z

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

Hardcoded texts here, too.

HonzaR · 2024-12-01T20:21:32Z

ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/keystoneqr/view/KeyStoneQrView.kt

+        ) {
+            Image(
+                bitmap = it,
+                contentDescription = "",


Should we use null instead of empty string for content descriptions that we want to omit from talkback?

made it to null

HonzaR · 2024-12-01T20:23:55Z

...ib/src/main/java/co/electriccoin/zcash/ui/screen/keystoneqr/viewmodel/KeystoneQrViewModel.kt

+        }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(ANDROID_STATE_FLOW_TIMEOUT), null)
+
+    @Suppress("MaxLineLength", "MagicNumber")
+    private fun genSolanaQRCode(): UREncoder {


Let's move this sample code to a fixture instead of the production part so we can easily replace it later.

this is refactored in the next coming PR

HonzaR · 2024-12-01T20:33:57Z

ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/scankeystone/view/ScanKeystoneView.kt

+        var failureText: String? = null
+
+        if (validationResult == ScanValidationState.INVALID) {
+            failureText = "This QR code is not a valid Keystone wallet."


Hardcoded text

HonzaR · 2024-12-01T20:34:25Z

ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/scankeystone/view/ScanKeystoneView.kt

+        Text(
+            modifier = Modifier.fillMaxWidth(),
+            textAlign = TextAlign.Center,
+            text = "Scan your Keystone wallet to connect",


Hardcoded text

HonzaR · 2024-12-01T20:37:51Z

ui-lib/src/main/java/co/electriccoin/zcash/ui/screen/scankeystone/view/ScanKeystoneView.kt

+    if (null == collectedCameraProvider) {
+        // Show loading indicator
+    } else {
+        val contentDescription = stringResource(id = R.string.scan_preview_content_description)


Let's add e.g. scan_keystone resource folder and separate these strings into it.

* Keystone integration (#1692) * Keystone QR generation and reader * Keystone account list UI * Top app bar refactor * Code cleanup * Design update * Keystone version bump * Code cleanup * Test hotfixes * Code cleanup * Keystone business logic implementation & ui elaboration (#1708) * Keystone business logic implementation & ui elaboration * Code cleanup * Code cleanup * Strings code cleanup * Navigation refactor * Navigation update * Select keystone account button hotfix * Navigation hotfix * Wallet restoration hotfixes and optimizations * Confirmation refactor * Design updates * Strings update and design updates * Zip321 keystone integration * Strings update * Spanish translations * New SDK changes integration * New SDK version adoption * Code cleanup * Code cleanup * Transaction history preparation * Sapling address made optional * Code cleanup * Design updates * Transaction hotfix * PCZT preparation * Settings design update * Strings update * Code cleanup * PCZT integration * Design updates * Code cleanup * App hotfixes * Strings update * Strings update * Transparent transaction UI update * Keystone tutorial link commented out * Keystone exception handling * Scan keystone progress bar * Scan keystone performance update * Share PCZT debug button * Optimizations * Add `MockSynchornizer` functions * Try-catch address obtaining Even though this is not the proper way we want to implement it, it fixes #1658 as a temporary solution * Fix `ExchangeRateState` loading state * Peformance optimizations * Account data source exception handling * Spanish strings updated * Transaction progress texts updated * Copy PCZT proposal data * Dialog window dim fix * Proposal proof optimization * Proposal proof optimization * Transaction progress shielding texts update * Code cleanup * Code cleanup * Code cleanup * PCZT progress calculation update * KS promo * KS sign in progress update * KS QR progress update * Adopt `Synchronizer.Status.INITIALIZING` state * Design updates * Propagate exceptions * Updates * Code cleanup * Promo update * Scan fix * Code cleanup * Bump to Zcash SDK 2.2.7-SNAPSHOT * Fix lint warnings * Make tests update With follow-up #1713 for fixing the failing tests. --------- Co-authored-by: Honza <[email protected]> --------- Co-authored-by: Honza <[email protected]>

Milan-Cerovsky added 6 commits November 19, 2024 12:00

Keystone QR generation and reader

389e199

Merge branch 'main' into keystone-integration

a91bfde

Keystone account list UI

7cd597b

Top app bar refactor

222193a

Merge branch 'main' into keystone-integration

dc46e07

Code cleanup

f8c730c

Milan-Cerovsky requested a review from HonzaR November 22, 2024 10:22

Milan-Cerovsky self-assigned this Nov 22, 2024

Milan-Cerovsky added 3 commits November 22, 2024 11:42

Design update

b18fb71

Keystone version bump

46b1cbf

Code cleanup

c3e64f7

HonzaR reviewed Nov 29, 2024

View reviewed changes

Test hotfixes

23afe85

Milan-Cerovsky requested a review from HonzaR November 29, 2024 17:34

HonzaR reviewed Dec 1, 2024

View reviewed changes

Code cleanup

1485244

Milan-Cerovsky requested a review from HonzaR December 2, 2024 09:01

HonzaR approved these changes Dec 2, 2024

View reviewed changes

Milan-Cerovsky merged commit d4805ca into feature/keystone Dec 2, 2024
15 checks passed

Milan-Cerovsky deleted the keystone-integration branch December 2, 2024 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keystone integration #1692

Keystone integration #1692

Milan-Cerovsky commented Nov 22, 2024 •

edited by HonzaR

Loading

HonzaR left a comment

HonzaR left a comment

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

HonzaR Dec 1, 2024

Milan-Cerovsky Dec 2, 2024

Keystone integration #1692

Keystone integration #1692

Conversation

Milan-Cerovsky commented Nov 22, 2024 • edited by HonzaR Loading

Author

Reviewer

Footnotes

HonzaR left a comment

Choose a reason for hiding this comment

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

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 Nov 22, 2024 •

edited by HonzaR

Loading