Multi-account and PCZT support #1517

LukasKorba · 2024-12-04T13:10:09Z

Closes #522
Closes #1513
Closes #1514

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: Did you review your own code in GitHub's web interface? Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs.
Automated tests: Did you add appropriate automated tests for any code changes?
Code coverage: Did you check the code coverage report for the automated tests? While we are not looking for perfect coverage, the tool can point out potential cases that have been missed.
Documentation: Did you update Docs as appropiate? (E.g README.md, etc.)
Run the app: Did you run the app and try the changes?
Did you provide Screenshots of what the App looks like before and after your changes as part of the description of this PR? (only applicable to UI Changes)
Rebase and squash: Did you pull in the latest changes from the main branch and squash your commits before assigning a reviewer? 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.

Reviewer

Checklist review: Did you go through the code with the Code Review Guidelines checklist?
Ad hoc review: Did you perform an ad hoc review? 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.
Automated tests: Did you review the automated tests?
Manual tests: Did you review the manual tests?You will find manual testing guidelines under our manual testing section
How is Code Coverage affected by this PR? We encourage you to compare coverage befor and after your changes and when possible, leave it in a better place. Learn More...
Documentation: Did you review Docs, README.md, LICENSE.md, and Architecture.md as appropriate?
Run the app: Did you run the app and try the changes? 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.

- the FFI has been switched to a preview of import UFVK

- importAccount() WIP

- Some of the Zip32AccountIndex has been refactored to UUID - Public import UFVK method in the SDK added AccountUUID refactor - phase 1 - rebased

- SDK builds again, UUIDs refactored everywhere in the SDK and demo app - tests will follow in the next phase - refactor of documented code in the follow up

nuttycom

This looks like it has a problem to me, which is that by just picking the first account in a lot of cases it may get inconsistent results. The order of accounts is nondeterministic, and it shouldn't be left to chance even if it was deterministic; the account that's in use needs to be tracked explicitly.

...ple/ZcashLightClientSample/ZcashLightClientSample/Get Address/GetAddressViewController.swift

...ple/ZcashLightClientSample/ZcashLightClientSample/Get Balance/GetBalanceViewController.swift

Example/ZcashLightClientSample/ZcashLightClientSample/Get UTXOs/GetUTXOsViewController.swift

nuttycom

Overall looking good; the one thing that I think needs to be changed is that keySource should be optional.

Sources/ZcashLightClientKit/Initializer.swift

Sources/ZcashLightClientKit/Model/WalletTypes.swift

Sources/ZcashLightClientKit/Rust/ZcashRustBackend.swift

Sources/ZcashLightClientKit/Rust/ZcashRustBackendWelding.swift

Sources/ZcashLightClientKit/Synchronizer.swift

nuttycom

Overall looking good; the one thing that I think needs to be changed is that keySource should be optional.

- Comments in the code updated and cleaned up - OfflineTests passes again, those failing has been removed from the bundle and marked to be fixed with a TODO Fixes of build - The SDK builds again

OfflineTests fixes

- updated to build and tests that are obsolete are now taken out of the bundle

- rust backend `getAccount` method implemented, it returns Account with associated data - `listAccount()` has been modified to return an array of Accounts instead of AccountUUIDs

- Account refactored to conform to Equatable, Hashable, Codable, Identifiable

- importAccount(ufvk, purpose, name, keySource) API

UnifiedAddress Hashable conformance

- a new public API for DerivationTool to derive UA from the UFVK added

- also attempt to fix _uuid in views

- Refactored to the proper types

- ufvk removed from the parameters

from_account_uuid

alias reverted Alias logic updated

- The comments were provided - Changelog updated - Code cleaned up - Tests fixed - Error codes for the Pczt

- FFI version bumped to 0.12.0

Package.swift

...e/ZcashLightClientSample.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

Tests/TestUtils/TestsData.swift

str4d

Reviewed ce667eb. I did not review ZcashLightClientSample or most of the test changes.

str4d · 2025-01-06T18:37:18Z

Tests/OfflineTests/ZcashRustBackendTests.swift

@@ -102,11 +104,11 @@ class ZcashRustBackendTests: XCTestCase {
        var uAddresses: [UnifiedAddress] = []
        for i in 0...2 {
            uAddresses.append(
-                try await rustBackend.getCurrentAddress(account: 0)
+                try await rustBackend.getCurrentAddress(accountUUID: TestsData.mockedAccountUUID)


This test is now broken. Previously account: 0 would happen to correctly point to the account created by the rustBackend.createAccount call above. Now, TestData.mockedAccountUUID always causes an error because it will never be a valid UUID. Instead, the test should use the real UUID returned from rustBackend.createAccount, which I believe is returned as a property of usk.

public struct UnifiedSpendingKey: Equatable, Undescribable { let network: NetworkType let bytes: [UInt8] }

doesn't hold the UUID of the Account, that is why I marked the test (and removed it from the suite) to be fixed in #1518 as a follow up

I completely disagree with disabling large swathes of tests in order to get a new feature in. The entire point of tests is to ensure that the changes being made don't break anything.

Tests/OfflineTests/ZcashRustBackendTests.swift

Tests/PerformanceTests/SynchronizerTests.swift

str4d · 2025-01-06T18:47:09Z

Tests/OfflineTests/TransactionRepositoryTests.swift

        let rawID = Data(fromHexEncodedString: "08cb5838ffd2c18ce15e7e8c50174940cd9526fff37601986f5480b7ca07e534")!
        let transaction = ZcashTransaction.Overview(
-            accountId: 0,
+            accountUUID: TestsData.mockedAccountUUID,


This test is using TestDbBuilder.prePopulatedMainnetDataDbURL():

zcash-swift-wallet-sdk/Tests/TestUtils/TestDbBuilder.swift

Lines 49 to 51 in c9fb348

static func prePopulatedMainnetDataDbURL() -> URL? {

Bundle.module.url(forResource: "darkside_data", withExtension: "db")

}

darkside_data.db is currently 2 years old, so is quite outdated. I don't see anywhere in this test that runs an initDataDb, which is what would upgrade it in-test to the latest DB state. That means the test was inherently broken prior to this PR.

After this PR, the same test bug applies as I wrote earlier: we cannot use TestsData.mockedAccountUUID in any call to the Rust backend (or any code that itself calls through to the Rust backend, like TransactionRepository. In the case of this test, the test code should fetch the actual UUID out of the database in setUp(), and then use it here instead of TestsData.mockedAccountUUID.

Yes, same here: The reason why this and other tests is/are broken is very likely not related to multi-account support but for some reasons long living in the code.

Sources/ZcashLightClientKit/Transaction/TransactionEncoder.swift

Sources/ZcashLightClientKit/Transaction/WalletTransactionEncoder.swift

Sources/ZcashLightClientKit/Synchronizer/SDKSynchronizer.swift

Sources/ZcashLightClientKit/Constants/ZcashSDK.swift

CHANGELOG.md

str4d

a8b1524 addresses my non-test comments, and the PR is probably fine. However, I cannot give it an ACK, because large swathes of tests have been disabled.

If @LukasKorba confirms that the now-disabled tests were in a broken state prior to this PR (and thus disabling them has no effect on the correctness checking in this PR), then I'll let the PR be merged as-is, but only if #1518 is made the number 1 priority. We cannot allow this many tests to be disabled in the long term; that is an unacceptable risk. cc @true-jared.

In any case, fixing the code comment is blocking.

str4d · 2025-01-08T19:50:42Z

Sources/ZcashLightClientKit/Transaction/TransactionEncoder.swift

@@ -91,7 +91,9 @@ protocol TransactionEncoder {
    /// - Parameter transaction: a transaction overview
    func submit(transaction: EncodedTransaction) async throws

-    func createTransactionsFromTxIds(_ txIds: [Data]) async throws -> [ZcashTransaction.Overview]
+    /// Tries to fetch the transaction for the given transction ids.


Suggested change

/// Tries to fetch the transaction for the given transction ids.

/// Tries to fetch the transaction for the given transaction ids.

str4d · 2025-01-08T20:00:59Z

Tests/TestUtils/TestsData.swift

@@ -123,3 +123,8 @@ class TestsData {
        self.networkType = networkType
    }
 }
+
+extension TestsData {
+    /// `mockedAccountUUID` is used to mock the rust backend for check only purposes. It's never passed to it.


This is currently false; mockedAccountUUID is passed into the Rust backend repeatedly. AFAICT it only happens in tests that are currently disabled, but that still makes this "never" false.

If the intention is to never pass this into the backend (and given my previous review, it should be), and the tests do not get fixed in this PR (which I strongly think they should be), then update this comment to point to the issues that will fix the code in order to make this statement true.

LukasKorba added 3 commits December 4, 2024 14:05

import-ufvk-ffi

a09f2b1

- the FFI has been switched to a preview of import UFVK

ImportAccount UFVK

3bfe5b3

- importAccount() WIP

AccountUUID refactor - phase 1

06181fb

- Some of the Zip32AccountIndex has been refactored to UUID - Public import UFVK method in the SDK added AccountUUID refactor - phase 1 - rebased

LukasKorba requested review from nuttycom and str4d December 4, 2024 13:10

AccountUUID refactor - phase 2

7a6bd72

- SDK builds again, UUIDs refactored everywhere in the SDK and demo app - tests will follow in the next phase - refactor of documented code in the follow up

nuttycom requested changes Dec 4, 2024

View reviewed changes

LukasKorba requested a review from nuttycom December 4, 2024 18:50

nuttycom requested changes Dec 4, 2024

View reviewed changes

LukasKorba requested a review from nuttycom December 5, 2024 09:02

LukasKorba marked this pull request as ready for review December 5, 2024 09:04

PR Comments addressed and code cleaned up

27af6f1

- Comments in the code updated and cleaned up - OfflineTests passes again, those failing has been removed from the bundle and marked to be fixed with a TODO Fixes of build - The SDK builds again

LukasKorba force-pushed the import-ufvk-ffi-preview branch from 01984b2 to 27af6f1 Compare December 5, 2024 09:31

LukasKorba added 10 commits December 5, 2024 11:59

Update swift.yml

92bd2e6

Generated mocks

05002a8

OfflineTests fixes

Performance, Network and Dark side tests

bb03333

- updated to build and tests that are obsolete are now taken out of the bundle

getAccount for UUID

5179f4e

- rust backend `getAccount` method implemented, it returns Account with associated data - `listAccount()` has been modified to return an array of Accounts instead of AccountUUIDs

Account conformace

e48cd60

- Account refactored to conform to Equatable, Hashable, Codable, Identifiable

hd_account_index wrapped in Zip32AccountIndex

df1b319

account_id -> account_uuid refactor

89773c6

Public importAccount method

f69aa19

- importAccount(ufvk, purpose, name, keySource) API

seedFingerprint and zip32AccountIndex parameters in importAccount added

8c099e3

Codable UnifiedAddress conformance

dcf6cd9

UnifiedAddress Hashable conformance

LukasKorba force-pushed the import-ufvk-ffi-preview branch from 9c239be to dcf6cd9 Compare December 9, 2024 13:28

LukasKorba added 5 commits December 9, 2024 17:31

Optional account_name

7782d6f

FFI bump

32bb9ba

DerivationTool.deriveUnifiedAddressFrom(ufvk)

08e5a73

- a new public API for DerivationTool to derive UA from the UFVK added

PCZT methods

6bab337

- also attempt to fix _uuid in views

PCZT methods updated

0fba7f2

- Refactored to the proper types

LukasKorba added 3 commits December 10, 2024 20:34

FFI bump

5943d63

PCZT API updated

30af9c6

- ufvk removed from the parameters

from_account_id reverted

dd587ac

from_account_uuid

LukasKorba force-pushed the import-ufvk-ffi-preview branch from 985e677 to dd587ac Compare December 10, 2024 21:15

FFI bumped

26ad508

LukasKorba force-pushed the import-ufvk-ffi-preview branch from ee8a88e to 75cfd08 Compare December 11, 2024 18:39

Reset of synchronizer alias

632626a

alias reverted Alias logic updated

LukasKorba force-pushed the import-ufvk-ffi-preview branch from 75cfd08 to 632626a Compare December 11, 2024 18:44

LukasKorba added 4 commits December 11, 2024 21:26

FFI bump

ee32938

Create and submit PCZT

f499898

Alias checker disconnected

fb38ac8

Pczt typealias and clean up

da00a69

- The comments were provided - Changelog updated - Code cleaned up - Tests fixed - Error codes for the Pczt

LukasKorba changed the title ~~Import ufvk ffi preview~~ Multi-account and PCZT support Dec 13, 2024

LukasKorba added 2 commits December 13, 2024 10:22

Code cleanup

f16d098

FFI 0.12.0 bump

ce667eb

- FFI version bumped to 0.12.0

str4d reviewed Jan 6, 2025

View reviewed changes

Package.swift Outdated Show resolved Hide resolved

str4d reviewed Jan 6, 2025

View reviewed changes

...e/ZcashLightClientSample.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved Show resolved Hide resolved

str4d reviewed Jan 6, 2025

View reviewed changes

Tests/TestUtils/TestsData.swift Outdated Show resolved Hide resolved

str4d requested changes Jan 7, 2025

View reviewed changes

Review comments addressed

a8b1524

LukasKorba requested a review from str4d January 7, 2025 09:45

str4d requested changes Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-account and PCZT support #1517

Multi-account and PCZT support #1517

LukasKorba commented Dec 4, 2024 •

edited

Loading

nuttycom left a comment

nuttycom left a comment

nuttycom left a comment

str4d left a comment

str4d Jan 6, 2025

LukasKorba Jan 7, 2025

str4d Jan 8, 2025

str4d Jan 6, 2025

LukasKorba Jan 7, 2025

str4d left a comment

str4d Jan 8, 2025

str4d Jan 8, 2025

	static func prePopulatedMainnetDataDbURL() -> URL? {
	Bundle.module.url(forResource: "darkside_data", withExtension: "db")
	}

	/// Tries to fetch the transaction for the given transction ids.
	/// Tries to fetch the transaction for the given transaction ids.

Multi-account and PCZT support #1517

Are you sure you want to change the base?

Multi-account and PCZT support #1517

Conversation

LukasKorba commented Dec 4, 2024 • edited Loading

Author

Reviewer

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

str4d Jan 6, 2025

Choose a reason for hiding this comment

LukasKorba Jan 7, 2025

Choose a reason for hiding this comment

str4d Jan 8, 2025

Choose a reason for hiding this comment

str4d Jan 6, 2025

Choose a reason for hiding this comment

LukasKorba Jan 7, 2025

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

str4d Jan 8, 2025

Choose a reason for hiding this comment

str4d Jan 8, 2025

Choose a reason for hiding this comment

LukasKorba commented Dec 4, 2024 •

edited

Loading