[#1632] Refactor `Account.DEFAULT` #1645

HonzaR · 2024-11-29T11:11:41Z

Closes Ensure that the SDK does not assume a default account anywhere #1632
Follow-up filed Refactor Account ZIP 32 index leftovers across SDK #1644
Partly solves Migrate Account type to the new multi-seed-compatible account identifier #1633
These changes as part of the Account logic refactoring add Synchronizer.getAllAccounts to the public API.
It also removes the deprecated Synchronizer.sendToAddress and Synchronizer.shieldFunds to simplify refactoring
Dema app updated:
- The deprecated Fragment-based Demo app refactored just to be buildable, will be removed as part of Eliminate old UI demo-app #973
- The newer Compose-based Demo app refactored to align with the new principles, but still in the most simple way
Changelog update

Note
This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.

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, etc.)
Run the demo app and try the changes
Pull in the latest changes from the main branch and squash your commits before assigning a reviewer⁴

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 demo 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 SDK, 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 demo app to look for build failures or crashes, humans running the demo 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 · 2024-11-29T15:58:36Z

The failing tests need to resolve this comment in the underlying PR first #1640 (review)

Milan-Cerovsky · 2024-12-01T12:29:42Z

crash - DerivationTool.getInstance().deriveUnifiedSpendingKey when deriving spending key
crash - Synchronizer.getAccounts when fast after restore wallet

feature request - getAccounts is fine as a function but please also add a flow

HonzaR · 2024-12-01T17:55:39Z

Thank you. I'll test both crashes.

crash - DerivationTool.getInstance().deriveUnifiedSpendingKey when deriving spending key

This one might be related to my comment above

crash - Synchronizer.getAccounts when fast after restore wallet

No quick thought about this one. I'll come back once reproduced.

feature request - getAccounts is fine as a function but please also add a flow

Yes, we'll need it when adopting in Zashi.

str4d

Reviewed db49a63. I did not review (most of) the changes to the test files.

This PR should be split into the changes that fix #1632 (which all LGTM), and the changes that finish #1640. Whether or not this happens outside of #1640 or by merging the PR into #1640 and then rebasing the branch to fix its commit history, I don't particularly mind; but the #1632 fixes need to merge before #1640 can be merged, because the latter blocks on the former.

sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/Account.kt

sdk-lib/src/test/java/cash/z/ecc/android/sdk/model/AccountTest.kt

sdk-lib/src/androidTest/java/cash/z/ecc/fixture/AccountFixture.kt

sdk-lib/src/androidTest/java/cash/z/ecc/fixture/FakeRustBackend.kt

sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/db/derived/TxOutputsView.kt

str4d · 2024-12-03T08:47:51Z

sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/PendingTransaction.kt

@@ -31,7 +31,7 @@ sealed class TransactionRecipient {
        override fun toString() = "TransactionRecipient.Address"
    }

-    data class Account(val accountValue: cash.z.ecc.android.sdk.model.Account) : TransactionRecipient() {
+    data class Account(val accountZip32Index: Int) : TransactionRecipient() {


This change is incorrect. The value currently returned from the SQL view is the internal account ID, not the ZIP 32 account index. (This is where the SDKs were conflating internal account IDs with ZIP 32 account indices previously; we should not persist with this conflation.)

This one is tricky, as we currently don't have a way to get Account here. I just changed the parameter to val accountId: Int in this PR. And in #1640 I added this small change d9cd693. IIUC the iOS SDK has the same bug. I'll rise it.

backend-lib/src/test/java/cash/z/ecc/android/sdk/internal/fixture/JniAccountBalanceFixture.kt

HonzaR · 2024-12-04T07:46:21Z

@Milan-Cerovsky, we're ready for review here. I moved the #1640 related changes into the PR and left only #1632 related changes in this PR. Please take a look.

We have deprecated the old Fragment-based Demo app. See #973. So, the purpose of these changes is purely the buildability of the new Compose-based Demo app.

This part of the Demo app will be removed as part of #973

+ necessary fixtures changes

As it was, as making it public could bring more requirements on our multi-account support in version 1

- We deduplicated the fixture across the related modules and their tests - It works with UUID instead of String converted to ByteArray - Documentation added

The rest of the changes (tests changes) will be used by 1640 though

str4d

utACK 2f4ac4c. I did not closely review the changes to the demo app.

PR should be squash-merged to get rid of the added-then-removed UUID changes from the history.

demo-app/src/main/java/cash/z/ecc/android/sdk/demoapp/demos/getaddress/GetAddressFragment.kt

demo-app/src/main/java/cash/z/ecc/android/sdk/demoapp/demos/getbalance/GetBalanceFragment.kt

sdk-lib/src/main/java/cash/z/ecc/android/sdk/fixture/AccountFixture.kt

demo-app/src/main/java/cash/z/ecc/android/sdk/demoapp/demos/getbalance/GetBalanceFragment.kt

* [#1632] Refactor `Account.DEFAULT` (#1645) * [#1632] Remove `Account.DEFAULT` * Update Account related APIs * Refactor balances APIs * Adopt `getAccounts` in fragment-based Demo app We have deprecated the old Fragment-based Demo app. See #973. So, the purpose of these changes is purely the buildability of the new Compose-based Demo app. * Add init to `JniUnifiedSpendingKey.kt` * Update deprecated Fragment-based Demo app This part of the Demo app will be removed as part of #973 * Remove deprecated functions from Synchronizer * Update WalletSnapshot and WalletVM APIs in Demo + necessary fixtures changes * Update newer Compose-based Demo app * Hide `Synchronizer.createAccount` form public API As it was, as making it public could bring more requirements on our multi-account support in version 1 * Add all accounts flow API * Refactor AccountFixture - We deduplicated the fixture across the related modules and their tests - Documentation added * Fix `fetchUtxos` error (code 14) - Closes #1648 - This also impacts SDK’s ability to recover from loss of internet connection - Changelog updated * Switch from ZIP 32 account indices to UUID account identifiers (#1640) * Switch from ZIP 32 account indices to UUID account identifiers * Rename TransactionRecipient.Account To emphasize the distinction between Account and TransactionRecipient.Account * Fix `Backend.createAccount` API parameters * Add `importAccountUfvk` to the Rust backend. * Propagate accountName and keySource across SDK * Propagate new importAccount across SDK * Wrap createAccount setup information * Wrap importAccount setup information * Add `Zip32AccountIndex` wrapper * Update key source parameter * Remove account from `UnifiedSpendingKey` * Fix `importAccountByUfvk` API * Refactor Account.accountUuid to wrapper class So we can easily keep it typesafe and compare it to each other * Add `JniAccountUsk` * Add `seedFingerprint` and `zip32AccountIndex` To public API `importAccountUfvk` * Transactions by account UUID * Refactor default account creation * Migrate to Rust crate revision with bugfixes to account UUID migration * Refactor inputs of `importAccountUfvk` * Use `FirsClassByteArray` for `seed` parameter --------- Co-authored-by: Honza <[email protected]> Co-authored-by: Kris Nuttycombe <[email protected]> Co-authored-by: Honza Rychnovský <[email protected]> Co-authored-by: Daira-Emma Hopwood <[email protected]> * Remove unused `TransactionEncoder` functions (#1660) * Resolve [#1640] review comments * `Account` documentation fix * Make `AccountPurpose.Spending` args not null As they now live under `AccountPurpose.Spending` where we need them not null + `AccountImportSetup` doc fix * Replace `bytes` with `usk` in `AccountUsk` * Change test name * Feature/proposal to pczt (#1647) * Migrate to latest revision of Zcash crates * Integrate PCZT APIs into the Android SDK * Fix `Backend.createPcztFromProposal` parameter * Add Synchronizer side of PCZT logic * Add `getAccountForUfvk` method to Rust backend & fix key derivation null issue. (#1656) * Add `getAccountForUfvk` method to the FFI. * Only decode key derivation metadata when creating a count with `Spending` purpose. Also, ensure that a spending account can still be created without providing key derivation metadata. * Update to `librustzcash` revision `205d4c930319b7b6d24aeb4efde69e9b4d1b6f7b` * Add the new `getAccountForUfvk` to `TypesafeBackend` * Update `FakeRustBackend` --------- Co-authored-by: Honza <[email protected]> * Remove unused `TransactionEncoder` functions (#1660) * Resolve [#1640] review comments * `Account` documentation fix * Make `AccountPurpose.Spending` args not null As they now live under `AccountPurpose.Spending` where we need them not null + `AccountImportSetup` doc fix * Replace `bytes` with `usk` in `AccountUsk` * Change test name * Testnet Synchronizer test fix * Detekt warning * Changelog update --------- Co-authored-by: Jack Grigg <[email protected]> Co-authored-by: Honza <[email protected]> Co-authored-by: Honza Rychnovský <[email protected]> * Peformance optimizations * Revert "Peformance optimizations" This reverts commit 68de6ae. * PCZT support follow-ups (#1663) * Rename exception * Add sapling param fetching to new KS-related fun * Add dedicated clone PZCT function * Add `Synchronizer.Status.INITIALIZING` For wallets to know that accounts and other APIs are not ready yet * [#1665] Checkpoints update Closes #1665 * Bump lib version to 2.2.7 * Migrate to the published Zcash crate releases including PCZT v1 (#1664) Co-authored-by: Honza <[email protected]> --------- Co-authored-by: Jack Grigg <[email protected]> Co-authored-by: Kris Nuttycombe <[email protected]> Co-authored-by: Daira-Emma Hopwood <[email protected]> Co-authored-by: Kris Nuttycombe <[email protected]> Co-authored-by: Milan Cerovsky <[email protected]>

HonzaR force-pushed the 1632-remove-default-account branch 2 times, most recently from ae3796b to f3f98c3 Compare November 29, 2024 13:56

HonzaR marked this pull request as ready for review November 29, 2024 13:58

HonzaR requested review from str4d and Milan-Cerovsky November 29, 2024 14:44

HonzaR requested a review from LukasKorba December 2, 2024 11:08

str4d requested changes Dec 3, 2024

View reviewed changes

HonzaR force-pushed the 1632-remove-default-account branch from 3dedb0c to 4409e65 Compare December 3, 2024 20:54

HonzaR changed the base branch from account-uuids to feature-2.2.7 December 3, 2024 21:13

HonzaR requested a review from str4d December 3, 2024 21:14

HonzaR added 16 commits December 4, 2024 14:11

[#1632] Remove Account.DEFAULT

09f4489

Update Account related APIs

edf12e0

Refactor balances APIs

7ba8578

Adopt getAccounts in fragment-based Demo app

e8be36a

We have deprecated the old Fragment-based Demo app. See #973. So, the purpose of these changes is purely the buildability of the new Compose-based Demo app.

Add init to JniUnifiedSpendingKey.kt

15b886f

File and link followup

1ed11c0

Update deprecated Fragment-based Demo app

2a09067

This part of the Demo app will be removed as part of #973

Remove deprecated functions from Synchronizer

1a9662c

Update WalletSnapshot and WalletVM APIs in Demo

12125de

+ necessary fixtures changes

Update newer Compose-based Demo app

5958dd8

Static code analysis warnings

dc59e2c

Changelog update

ed14aa1

Update fixture values + tests

c4a53e1

Fix some Demo app tests

7c1e40a

Fix shield funds sample test

731f2d5

Hide Synchronizer.createAccount form public API

77d80b0

As it was, as making it public could bring more requirements on our multi-account support in version 1

HonzaR added 8 commits December 4, 2024 14:13

Add all accounts flow API

5dd8ec6

Refactor AccountFixture

9f66836

- We deduplicated the fixture across the related modules and their tests - It works with UUID instead of String converted to ByteArray - Documentation added

Move Account changes to account-uuids

9a7dbbd

Move AccountTest changes to account-uuids

496ba8c

Move FakeRustBackend changes to account-uuids

a412b9a

Revert TransactionRecipient.Account parameter change

5f7cac1

Fix static code analysis warnings

152d1ae

Changelog cleanup

6f51c74

HonzaR force-pushed the 1632-remove-default-account branch from 5d8cd90 to 426f9d2 Compare December 4, 2024 13:33

Final 1640 changes stirp out

2f4ac4c

The rest of the changes (tests changes) will be used by 1640 though

HonzaR force-pushed the 1632-remove-default-account branch from 426f9d2 to 2f4ac4c Compare December 4, 2024 13:46

str4d approved these changes Dec 4, 2024

View reviewed changes

str4d merged commit a744a67 into feature-2.2.7 Dec 4, 2024
9 of 12 checks passed

str4d deleted the 1632-remove-default-account branch December 4, 2024 15:55

HonzaR mentioned this pull request Dec 4, 2024

Switch from ZIP 32 account indices to UUID account identifiers #1640

Merged

13 tasks

This was referenced Dec 13, 2024

Remove unused TransactionEncoder functions #1660

Merged

Add getAccountForUfvk method to Rust backend & fix key derivation null issue. #1656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1632] Refactor `Account.DEFAULT` #1645

[#1632] Refactor `Account.DEFAULT` #1645

HonzaR commented Nov 29, 2024 •

edited

Loading

HonzaR commented Nov 29, 2024

Milan-Cerovsky commented Dec 1, 2024

HonzaR commented Dec 1, 2024

str4d left a comment •

edited

Loading

str4d Dec 3, 2024

HonzaR Dec 3, 2024

HonzaR commented Dec 4, 2024

str4d left a comment

[#1632] Refactor Account.DEFAULT #1645

[#1632] Refactor Account.DEFAULT #1645

Conversation

HonzaR commented Nov 29, 2024 • edited Loading

Author

Reviewer

Footnotes

HonzaR commented Nov 29, 2024

Milan-Cerovsky commented Dec 1, 2024

HonzaR commented Dec 1, 2024

str4d left a comment • edited Loading

Choose a reason for hiding this comment

str4d Dec 3, 2024

Choose a reason for hiding this comment

HonzaR Dec 3, 2024

Choose a reason for hiding this comment

HonzaR commented Dec 4, 2024

str4d left a comment

Choose a reason for hiding this comment

[#1632] Refactor `Account.DEFAULT` #1645

[#1632] Refactor `Account.DEFAULT` #1645

HonzaR commented Nov 29, 2024 •

edited

Loading

str4d left a comment •

edited

Loading