From 5a2897061ca1fd31966b79666e7139d694e9832a Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 12 Mar 2024 10:12:42 -0600 Subject: [PATCH] Apply suggestions from code review Co-authored-by: str4d --- zcash_client_sqlite/src/wallet/common.rs | 4 +--- zcash_client_sqlite/src/wallet/orchard.rs | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/common.rs b/zcash_client_sqlite/src/wallet/common.rs index 31f9b23ef..1a47fc404 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -31,7 +31,7 @@ fn per_protocol_names(protocol: ShieldedProtocol) -> (&'static str, &'static str fn unscanned_tip_exists( conn: &Connection, anchor_height: BlockHeight, - table_prefix: &str, + table_prefix: &'static str, ) -> Result { // v_sapling_shard_unscanned_ranges only returns ranges ending on or after wallet birthday, so // we don't need to refer to the birthday in this query. @@ -140,8 +140,6 @@ where // 3) Select all notes for which the running sum was less than the required value, as // well as a single note for which the sum was greater than or equal to the // required value, bringing the sum of all selected notes across the threshold. - // - // 4) Match the selected notes against the witnesses at the desired height. let mut stmt_select_notes = conn.prepare_cached( &format!( "WITH eligible AS ( diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index a4b1f83b9..e3eacdfe8 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -140,6 +140,13 @@ fn to_spendable_note( let ufvk_str: Option = row.get(8)?; let scope_code: Option = row.get(9)?; + // If we don't have information about the recipient key scope or the ufvk we can't determine + // which spending key to use. This may be because the received note was associated with an + // imported viewing key, so we treat such notes as not spendable. Although this method is + // presently only called using the results of queries where both the ufvk and + // recipient_key_scope columns are checked to be non-null, this is method is written + // defensively to account for the fact that both of these are nullable columns in case it + // is used elsewhere in the future. ufvk_str .zip(scope_code) .map(|(ufvk_str, scope_code)| { @@ -176,10 +183,6 @@ fn to_spendable_note( .transpose() } -// The `clippy::let_and_return` lint is explicitly allowed here because a bug in Clippy -// (https://github.com/rust-lang/rust-clippy/issues/11308) means it fails to identify that the `result` temporary -// is required in order to resolve the borrows involved in the `query_and_then` call. -#[allow(clippy::let_and_return)] pub(crate) fn get_spendable_orchard_note( conn: &Connection, params: &P, @@ -299,7 +302,7 @@ pub(crate) fn get_orchard_nullifiers( // Get the nullifiers for the notes we are tracking let mut stmt_fetch_nullifiers = match query { NullifierQuery::Unspent => conn.prepare( - "SELECT rn.id, rn.account_id, rn.nf + "SELECT rn.account_id, rn.nf FROM orchard_received_notes rn LEFT OUTER JOIN transactions tx ON tx.id_tx = rn.spent @@ -307,15 +310,15 @@ pub(crate) fn get_orchard_nullifiers( AND nf IS NOT NULL", )?, NullifierQuery::All => conn.prepare( - "SELECT rn.id, rn.account_id, rn.nf + "SELECT rn.account_id, rn.nf FROM orchard_received_notes rn WHERE nf IS NOT NULL", )?, }; let nullifiers = stmt_fetch_nullifiers.query_and_then([], |row| { - let account = AccountId(row.get(1)?); - let nf_bytes: [u8; 32] = row.get(2)?; + let account = AccountId(row.get(0)?); + let nf_bytes: [u8; 32] = row.get(1)?; Ok::<_, rusqlite::Error>((account, Nullifier::from_bytes(&nf_bytes).unwrap())) })?;