zcash_client_sqlite: Assign UUIDs to each account #1631

str4d · 2024-11-21T11:14:44Z

This provides equivalent uniqueness to the accounts table primary key, but avoids collisions across wallet recreation events (to defend against downstream crate users who don't flush any persisted account IDs at those events).

Closes #1629.

codecov · 2024-11-21T12:36:48Z

Codecov Report

Attention: Patch coverage is 69.56522% with 28 lines in your changes missing coverage. Please review.

Project coverage is 56.46%. Comparing base (2df4497) to head (bf42ec2).

Files with missing lines	Patch %	Lines
zcash_client_sqlite/src/wallet.rs	18.18%	18 Missing ⚠️
zcash_client_sqlite/src/lib.rs	0.00%	5 Missing ⚠️
...te/src/wallet/init/migrations/add_account_uuids.rs	92.18%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1631      +/-   ##
==========================================
+ Coverage   56.39%   56.46%   +0.07%     
==========================================
  Files         148      149       +1     
  Lines       19232    19319      +87     
==========================================
+ Hits        10845    10908      +63     
- Misses       8387     8411      +24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

str4d · 2024-11-22T09:45:04Z

I discovered that the Android SDK is misusing v_tx_outputs by putting to_account_id into a type that actually stores a ZIP 32 account index. These previously were indeed the same type, until #1235 was merged. I think we need to alter the views to expose the account UUIDs instead of the account IDs, particularly as after this PR the account IDs cannot be extracted from AccountId and are intended to be entirely internal.

daira · 2024-11-22T19:38:59Z

zcash_client_sqlite/src/lib.rs

+/// This is an ephemeral value for efficiently and generically working with accounts in a
+/// [`WalletDb`]. To reference accounts in external contexts, use [`AccountUuid`].
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Default)]
+pub struct AccountId(u32);


Would it be useful to rename this to LocalAccountId? Doing so is obviously a more disruptive change, but it would force all code currently using AccountId [directly as a type] to explicitly choose between AccountUuid and LocalAccountId. It would also, I think, reduce the chance of confusing these with ZIP 32 account indices (which is something I've found to be a hazard on several occasions).

Such a rename serves little purpose, because it is named directly for the WalletRead::AccountId associated type that it gets set to, and the associated type is what actually gets used in most places.

I'm somewhat annoyed that we now have this split where our local account ID that needs to be in WalletRead is infallible concretely but fallible in the trait API, which was in part due to #1235 getting rushed into a crate release (something I expressed disagreement with at the time). As it is, we'd need Yet Another Large Refactor to fix it, and I explicitly do not want to do that at the same time as these changes; our changelogs get large enough as it is.

Confusion with the associated type is one reason I want to do this change, actually. It makes no sense to me to have the associated type be named the same as one particular possible instantiation of it. They are conceptually not the same thing. (See also https://github.com/Electric-Coin-Company/zcash-swift-wallet-sdk/pull/1515/files#r1855209493 .)

zcash_client_sqlite/src/wallet/init/migrations.rs

daira

utACK with comments.

str4d · 2024-11-23T05:51:02Z

Force-pushed to replace account_id in the views with account_uuid (because account_id is now intended to be an internal-only value). cc @AArnott for awareness (though I've also documented it in the changelogs).

zcash_client_sqlite/CHANGELOG.md

daira · 2024-11-23T22:10:51Z

zcash_client_sqlite/src/wallet.rs

@@ -779,8 +794,9 @@ pub(crate) fn get_derived_account<P: consensus::Parameters>(
            ":hd_account_index": u32::from(account_index),


I'm confused: stmt uses :account_id as a named parameter, but the name specified here is :hd_account_index. How was this working?

Hmm. https://docs.rs/rusqlite/latest/rusqlite/trait.Params.html#named-parameters :

Note: Unbound named parameters will be left to the value they previously were bound with, falling back to NULL for parameters which have never been bound.

That's a bug, and we've had that kind of bug before. It would be nice to figure out a way to lint for this.

I'd settle for a runtime error. Substituting NULL is just terrible. 😭

I guess you could have a macro to generate a struct and an impl rusqlite::Params for that struct from the query string. Then you could pass an instance of the struct rather than named_params!, and the compiler would check whether you'd missed out fields or included undefined fields. It could be made completely typesafe; you would have the statement type returned from the macro be parameterized by the struct type, so that it will only accept the right parameters. Sounds like a lot of effort to get working, though, unless someone has already done something like it.

daira · 2024-11-24T00:43:23Z

zcash_client_sqlite/src/wallet.rs

@@ -767,7 +782,7 @@ pub(crate) fn get_derived_account<P: consensus::Parameters>(
    account_index: zip32::AccountId,
 ) -> Result<Option<Account>, SqliteClientError> {
    let mut stmt = conn.prepare(
-        "SELECT id, ufvk
+        "SELECT id, uuid, ufvk
        FROM accounts
        WHERE hd_seed_fingerprint = :hd_seed_fingerprint
          AND hd_account_index = :account_id",


Suggested change

AND hd_account_index = :account_id",

AND hd_account_index = :hd_account_index",

daira

utACK with this suggestion: https://github.com/zcash/librustzcash/pull/1631/files#r1855319372

zcash_client_sqlite: Add missing leaf migration states for tests

45392cf

str4d marked this pull request as ready for review November 22, 2024 02:48

str4d mentioned this pull request Nov 22, 2024

Support for multiple accounts #1335

Open

daira reviewed Nov 22, 2024

View reviewed changes

zcash_client_sqlite/src/wallet/init/migrations.rs Outdated Show resolved Hide resolved

daira previously approved these changes Nov 22, 2024

View reviewed changes

str4d added 2 commits November 23, 2024 05:49

zcash_client_sqlite: Add a uuid column to the accounts table

fd5bb46

zcash_client_sqlite: Add AccountUuid to expose the uuid column

bf42ec2

str4d dismissed daira’s stale review via bf42ec2 November 23, 2024 05:49

str4d force-pushed the 1629-zcs-account-id branch from c8aff5b to bf42ec2 Compare November 23, 2024 05:49

daira mentioned this pull request Nov 23, 2024

[#1511] Refactor account representation from int to a dedicated account structure Electric-Coin-Company/zcash-swift-wallet-sdk#1515

Open

14 tasks

daira reviewed Nov 23, 2024

View reviewed changes

zcash_client_sqlite/CHANGELOG.md Show resolved Hide resolved

daira reviewed Nov 23, 2024

View reviewed changes

daira reviewed Nov 24, 2024

View reviewed changes

daira approved these changes Nov 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zcash_client_sqlite: Assign UUIDs to each account #1631

zcash_client_sqlite: Assign UUIDs to each account #1631

str4d commented Nov 21, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

str4d commented Nov 22, 2024

daira Nov 22, 2024 •

edited

Loading

str4d Nov 22, 2024

daira Nov 23, 2024 •

edited

Loading

daira left a comment

str4d commented Nov 23, 2024

daira Nov 23, 2024 •

edited

Loading

daira Nov 23, 2024

str4d Nov 23, 2024

daira Nov 24, 2024

daira Nov 24, 2024 •

edited

Loading

daira Nov 24, 2024

daira left a comment

		@@ -779,8 +794,9 @@ pub(crate) fn get_derived_account<P: consensus::Parameters>(
		":hd_account_index": u32::from(account_index),

	AND hd_account_index = :account_id",
	AND hd_account_index = :hd_account_index",

zcash_client_sqlite: Assign UUIDs to each account #1631

Are you sure you want to change the base?

zcash_client_sqlite: Assign UUIDs to each account #1631

Conversation

str4d commented Nov 21, 2024

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

str4d commented Nov 22, 2024

daira Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

str4d Nov 22, 2024

Choose a reason for hiding this comment

daira Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d commented Nov 23, 2024

daira Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

daira Nov 23, 2024

Choose a reason for hiding this comment

str4d Nov 23, 2024

Choose a reason for hiding this comment

daira Nov 24, 2024

Choose a reason for hiding this comment

daira Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

daira Nov 24, 2024

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2024 •

edited

Loading

daira Nov 22, 2024 •

edited

Loading

daira Nov 23, 2024 •

edited

Loading

daira Nov 23, 2024 •

edited

Loading

daira Nov 24, 2024 •

edited

Loading