Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add WalletRead::get_seed_account #1259

Merged
merged 3 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ and this library adheres to Rust's notion of
- Arguments to `ScannedBlock::from_parts` have changed.
- Changes to the `WalletRead` trait:
- Added `Account` associated type.
- Added `get_orchard_nullifiers`
- Added `get_orchard_nullifiers` method.
- `get_account_for_ufvk` now returns an `Self::Account` instead of a bare
`AccountId`
- Added `get_seed_account` method.
- Changes to the `InputSource` trait:
- `select_spendable_notes` now takes its `target_value` argument as a
`NonNegativeAmount`. Also, the values of the returned map are also
Expand Down
18 changes: 18 additions & 0 deletions zcash_client_backend/src/data_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
use incrementalmerkletree::{frontier::Frontier, Retention};
use secrecy::SecretVec;
use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree};
use zcash_keys::keys::HdSeedFingerprint;

use self::{chain::CommitmentTreeRoot, scanning::ScanRange};
use crate::{
Expand Down Expand Up @@ -655,6 +656,14 @@
ufvk: &UnifiedFullViewingKey,
) -> Result<Option<Self::Account>, Self::Error>;

/// Returns the account corresponding to a given [`HdSeedFingerprint`] and
/// [`zip32::AccountId`], if any.
fn get_seed_account(
&self,
seed: &HdSeedFingerprint,
account_id: zip32::AccountId,
) -> Result<Option<Self::Account>, Self::Error>;

/// Returns the wallet balances and sync status for an account given the specified minimum
/// number of confirmations, or `Ok(None)` if the wallet has no balance data available.
fn get_wallet_summary(
Expand Down Expand Up @@ -1416,6 +1425,7 @@
use secrecy::{ExposeSecret, SecretVec};
use shardtree::{error::ShardTreeError, store::memory::MemoryShardStore, ShardTree};
use std::{collections::HashMap, convert::Infallible, num::NonZeroU32};
use zcash_keys::keys::HdSeedFingerprint;

use zcash_primitives::{
block::BlockHash,
Expand Down Expand Up @@ -1588,6 +1598,14 @@
Ok(None)
}

fn get_seed_account(

Check warning on line 1601 in zcash_client_backend/src/data_api.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api.rs#L1601

Added line #L1601 was not covered by tests
&self,
_seed: &HdSeedFingerprint,
_account_id: zip32::AccountId,
) -> Result<Option<Self::Account>, Self::Error> {
Ok(None)

Check warning on line 1606 in zcash_client_backend/src/data_api.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api.rs#L1606

Added line #L1606 was not covered by tests
}

fn get_wallet_summary(
&self,
_min_confirmations: u32,
Expand Down
8 changes: 8 additions & 0 deletions zcash_client_sqlite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,14 @@
wallet::get_account_for_ufvk(self.conn.borrow(), &self.params, ufvk)
}

fn get_seed_account(

Check warning on line 408 in zcash_client_sqlite/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/lib.rs#L408

Added line #L408 was not covered by tests
&self,
seed: &HdSeedFingerprint,
account_id: zip32::AccountId,
) -> Result<Option<Self::Account>, Self::Error> {
wallet::get_seed_account(self.conn.borrow(), &self.params, seed, account_id)

Check warning on line 413 in zcash_client_sqlite/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/lib.rs#L413

Added line #L413 was not covered by tests
}

fn get_wallet_summary(
&self,
min_confirmations: u32,
Expand Down
40 changes: 40 additions & 0 deletions zcash_client_sqlite/src/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,46 @@
}
}

/// Returns the account id corresponding to a given [`HdSeedFingerprint`]
/// and [`zip32::AccountId`], if any.
pub(crate) fn get_seed_account<P: consensus::Parameters>(

Check warning on line 784 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L784

Added line #L784 was not covered by tests
conn: &rusqlite::Connection,
params: &P,
seed: &HdSeedFingerprint,
account_id: zip32::AccountId,
) -> Result<Option<(AccountId, Option<UnifiedFullViewingKey>)>, SqliteClientError> {
let mut stmt = conn.prepare(

Check warning on line 790 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L790

Added line #L790 was not covered by tests
"SELECT id, ufvk
FROM accounts
WHERE hd_seed_fingerprint = :hd_seed_fingerprint
AND hd_account_index = :account_id",
nuttycom marked this conversation as resolved.
Show resolved Hide resolved
)?;

let mut accounts = stmt.query_and_then::<_, SqliteClientError, _, _>(
named_params![
":hd_seed_fingerprint": seed.as_bytes(),
":hd_account_index": u32::from(account_id),

Check warning on line 800 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L797-L800

Added lines #L797 - L800 were not covered by tests
],
|row| {
let account_id = row.get::<_, u32>(0).map(AccountId)?;
Ok((
account_id,
row.get::<_, Option<String>>(1)?
.map(|ufvk_str| UnifiedFullViewingKey::decode(params, &ufvk_str))
.transpose()
.map_err(|e| {
SqliteClientError::CorruptedData(format!(
"Could not decode unified full viewing key for account {:?}: {}",
account_id, e

Check warning on line 812 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L802-L812

Added lines #L802 - L812 were not covered by tests
))
})?,
))
},
)?;

accounts.next().transpose()

Check warning on line 819 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L819

Added line #L819 was not covered by tests
}

pub(crate) trait ScanProgress {
fn sapling_scan_progress(
&self,
Expand Down
48 changes: 48 additions & 0 deletions zcash_client_sqlite/src/wallet/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,54 @@ mod tests {
expected_idx += 1;
}

let expected_indices = vec![
r#"CREATE UNIQUE INDEX accounts_ufvk ON "accounts" (ufvk)"#,
r#"CREATE UNIQUE INDEX accounts_uivk ON "accounts" (uivk)"#,
r#"CREATE UNIQUE INDEX hd_account ON "accounts" (hd_seed_fingerprint, hd_account_index)"#,
r#"CREATE INDEX "addresses_accounts" ON "addresses" (
Comment on lines +439 to +443
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this; I was unaware that uniqueness constraints were starting to be embedded in the indices (which prior to now I only thought were present for performance). I rely heavily on changes to init.rs to track changes to the database structure and determine what constraints are applied.

"account_id" ASC
)"#,
r#"CREATE INDEX nf_map_locator_idx ON nullifier_map(block_height, tx_index)"#,
r#"CREATE INDEX orchard_received_notes_account ON orchard_received_notes (
account_id ASC
)"#,
r#"CREATE INDEX orchard_received_notes_spent ON orchard_received_notes (
spent ASC
)"#,
r#"CREATE INDEX orchard_received_notes_tx ON orchard_received_notes (
tx ASC
)"#,
r#"CREATE INDEX "sapling_received_notes_account" ON "sapling_received_notes" (
"account_id" ASC
)"#,
r#"CREATE INDEX "sapling_received_notes_spent" ON "sapling_received_notes" (
"spent" ASC
)"#,
r#"CREATE INDEX "sapling_received_notes_tx" ON "sapling_received_notes" (
"tx" ASC
)"#,
r#"CREATE INDEX sent_notes_from_account ON "sent_notes" (from_account_id)"#,
r#"CREATE INDEX sent_notes_to_account ON "sent_notes" (to_account_id)"#,
r#"CREATE INDEX sent_notes_tx ON "sent_notes" (tx)"#,
r#"CREATE INDEX utxos_received_by_account ON "utxos" (received_by_account_id)"#,
r#"CREATE INDEX utxos_spent_in_tx ON "utxos" (spent_in_tx)"#,
];
let mut indices_query = st
.wallet()
.conn
.prepare("SELECT sql FROM sqlite_master WHERE type = 'index' AND sql != '' ORDER BY tbl_name, name")
.unwrap();
let mut rows = indices_query.query([]).unwrap();
let mut expected_idx = 0;
while let Some(row) = rows.next().unwrap() {
let sql: String = row.get(0).unwrap();
assert_eq!(
re.replace_all(&sql, " "),
re.replace_all(expected_indices[expected_idx], " ")
);
expected_idx += 1;
}

let expected_views = vec![
// v_orchard_shard_scan_ranges
format!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
(account_type = {account_type_imported} AND hd_seed_fingerprint IS NULL AND hd_account_index IS NULL)
)
);
CREATE UNIQUE INDEX accounts_uivk ON accounts_new ("uivk");
CREATE UNIQUE INDEX accounts_ufvk ON accounts_new ("ufvk");
CREATE UNIQUE INDEX hd_account ON accounts_new (hd_seed_fingerprint, hd_account_index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the full subtleties of UNIQUE INDEX vs CONSTRAIN UNIQUE and why you'd choose one over the other. AFAICT:

  • They both produce an index.
  • UNIQUE INDEX can be removed and added without altering the table (and I guess CREATE UNIQUE INDEX fails if the table contains non-unique data?)
  • UNIQUE INDEX can include expressions
  • CONSTRAIN UNIQUE has greater support in ON CONFLICT logic (whereas UNIQUE INDEX only supports INSERT ... ON CONFLICT I think).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was looking at this today. I think that the ON CONFLICT capabilities are actually the same, from what I was reading, at least for recent releases? Given that the separate indices can be dropped and added independently, I think I have a slight preference for them, but I don't think it makes much difference one way or the other.

CREATE UNIQUE INDEX accounts_uivk ON accounts_new (uivk);
CREATE UNIQUE INDEX accounts_ufvk ON accounts_new (ufvk);
"#),
)?;

Expand Down
Loading