From 01b0d1c62be67c61f25a8db68840a21d43dfc782 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 13 Nov 2024 13:45:45 -0700 Subject: [PATCH] Apply suggestions from code review. Co-authored by: Jack Grig Co-authored-by: Daira-Emma Hopwood --- components/zcash_protocol/src/value.rs | 6 ++- zcash_client_backend/src/fees.rs | 8 ++-- zcash_client_sqlite/CHANGELOG.md | 2 +- zcash_client_sqlite/src/error.rs | 2 +- zcash_client_sqlite/src/wallet/common.rs | 48 +++++++++++------------- 5 files changed, 33 insertions(+), 33 deletions(-) diff --git a/components/zcash_protocol/src/value.rs b/components/zcash_protocol/src/value.rs index aa4d1709b..e7e5001db 100644 --- a/components/zcash_protocol/src/value.rs +++ b/components/zcash_protocol/src/value.rs @@ -321,7 +321,8 @@ impl Zatoshis { /// Divides this `Zatoshis` value by the given divisor and returns the quotient and remainder. pub fn div_with_remainder(&self, divisor: NonZeroU64) -> QuotRem { let divisor = u64::from(divisor); - // `self` is already bounds-checked, so we don't need to re-check it in division + // `self` is already bounds-checked, and both the quotient and remainder + // are <= self, so we don't need to re-check them in division. QuotRem { quotient: Zatoshis(self.0 / divisor), remainder: Zatoshis(self.0 % divisor), @@ -427,7 +428,8 @@ impl Div for Zatoshis { type Output = Zatoshis; fn div(self, rhs: NonZeroU64) -> Zatoshis { - // `self` is already bounds-checked, so we don't need to re-check it + // `self` is already bounds-checked and the quotient is <= self, so + // we don't need to re-check it Zatoshis(self.0 / u64::from(rhs)) } } diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index eba11c7e6..1950c0509 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -368,6 +368,8 @@ impl SplitPolicy { /// In the case that no other conditions provided by the user are available to fall back on, /// a default value of [`MARGINAL_FEE`] * 100 will be used as the "minimum usable note value" /// when retrieving wallet metadata. + /// + /// [`MARGINAL_FEE`]: zcash_primitives::transaction::fees::zip317::MARGINAL_FEE pub(crate) const MIN_NOTE_VALUE: NonNegativeAmount = NonNegativeAmount::const_from_u64(500000); /// Constructs a new [`SplitPolicy`] that splits change to ensure the given number of spendable @@ -404,9 +406,9 @@ impl SplitPolicy { /// Returns the number of output notes to produce from the given total change value, given the /// total value and number of existing unspent notes in the account and this policy. /// - /// If splitting change to produce [`Self::target_output_count`] would result in notes of value less than - /// [`Self::min_split_output_value`], then this will produce - /// + /// If splitting change to produce [`Self::target_output_count`] would result in notes of value + /// less than [`Self::min_split_output_value`], then this will suggest a smaller number of + /// splits so that each resulting change note has sufficient value. pub fn split_count( &self, existing_notes: Option, diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 0851eb471..fcf0fa286 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -15,7 +15,7 @@ and this library adheres to Rust's notion of - MSRV is now 1.77.0. - Migrated from `schemer` to our fork `schemerz`. - Migrated to `rusqlite 0.32`. -- `error::SqliteClientError` has additional variant `NoteSelectorInvalid` +- `error::SqliteClientError` has additional variant `NoteFilterInvalid` ## [0.12.2] - 2024-10-21 diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 43be98939..f8b2602bb 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -191,7 +191,7 @@ impl fmt::Display for SqliteClientError { SqliteClientError::ChainHeightUnknown => write!(f, "Chain height unknown; please call `update_chain_tip`"), SqliteClientError::UnsupportedPoolType(t) => write!(f, "Pool type is not currently supported: {}", t), SqliteClientError::BalanceError(e) => write!(f, "Balance error: {}", e), - SqliteClientError::NoteFilterInvalid(s) => write!(f, "Could not evaluate selection query: {:?}", s), + SqliteClientError::NoteFilterInvalid(s) => write!(f, "Could not evaluate filter query: {:?}", s), #[cfg(feature = "transparent-inputs")] SqliteClientError::ReachedGapLimit(account_id, bad_index) => write!(f, "The proposal cannot be constructed until transactions with previously reserved ephemeral address outputs have been mined. \ diff --git a/zcash_client_sqlite/src/wallet/common.rs b/zcash_client_sqlite/src/wallet/common.rs index b76b1a2bf..6cfee5217 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -241,7 +241,7 @@ pub(crate) fn spendable_notes_meta( protocol: ShieldedProtocol, chain_tip_height: BlockHeight, account: AccountId, - selector: &NoteFilter, + filter: &NoteFilter, exclude: &[ReceivedNoteId], ) -> Result, SqliteClientError> { let (table_prefix, _, _) = per_protocol_names(protocol); @@ -306,10 +306,10 @@ pub(crate) fn spendable_notes_meta( conn: &rusqlite::Connection, table_prefix: &str, account: AccountId, - selector: &NoteFilter, + filter: &NoteFilter, chain_tip_height: BlockHeight, ) -> Result, SqliteClientError> { - match selector { + match filter { NoteFilter::ExceedsMinValue(v) => Ok(Some(*v)), NoteFilter::ExceedsPriorSendPercentile(n) => { let mut bucket_query = conn.prepare( @@ -351,27 +351,24 @@ pub(crate) fn spendable_notes_meta( } NoteFilter::ExceedsBalancePercentage(p) => { let balance = conn.query_row_and_then::<_, SqliteClientError, _, _>( - &format!( - "SELECT SUM(rn.value) - FROM v_received_outputs rn - INNER JOIN accounts a ON a.id = rn.account_id - INNER JOIN transactions ON transactions.id_tx = rn.transaction_id - WHERE rn.account_id = :account_id - AND a.ufvk IS NOT NULL - AND transactions.mined_height IS NOT NULL - AND rn.pool != :transparent_pool - AND rn.id_within_pool_table NOT IN ( - SELECT rns.received_output_id - FROM v_received_output_spends rns - JOIN transactions stx ON stx.id_tx = rns.transaction_id - WHERE rns.pool == rn.pool - AND ( - stx.block IS NOT NULL -- the spending tx is mined - OR stx.expiry_height IS NULL -- the spending tx will not expire - OR stx.expiry_height > :chain_tip_height -- the spending tx is unexpired - ) - )" - ), + "SELECT SUM(rn.value) + FROM v_received_outputs rn + INNER JOIN accounts a ON a.id = rn.account_id + INNER JOIN transactions ON transactions.id_tx = rn.transaction_id + WHERE rn.account_id = :account_id + AND a.ufvk IS NOT NULL + AND transactions.mined_height IS NOT NULL + AND rn.pool != :transparent_pool + AND (rn.pool, rn.id_within_pool_table) NOT IN ( + SELECT rns.pool, rns.received_output_id + FROM v_received_output_spends rns + JOIN transactions stx ON stx.id_tx = rns.transaction_id + WHERE ( + stx.block IS NOT NULL -- the spending tx is mined + OR stx.expiry_height IS NULL -- the spending tx will not expire + OR stx.expiry_height > :chain_tip_height -- the spending tx is unexpired + ) + )", named_params![ ":account_id": account.0, ":chain_tip_height": u32::from(chain_tip_height), @@ -423,8 +420,7 @@ pub(crate) fn spendable_notes_meta( // TODO: Simplify the query before executing it. Not worrying about this now because queries // will be developer-configured, not end-user defined. - if let Some(min_value) = - min_note_value(conn, table_prefix, account, selector, chain_tip_height)? + if let Some(min_value) = min_note_value(conn, table_prefix, account, filter, chain_tip_height)? { let (note_count, total_value) = run_selection(min_value)?;