From 24ebe4c6430e41f27ea5648fa24058de6389edb7 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 5 Jan 2024 13:05:39 -0700 Subject: [PATCH] Address comments from code review. Co-authored-by: Jack Grigg --- zcash_client_backend/CHANGELOG.md | 43 ++++++++++--------- zcash_client_backend/src/fees.rs | 1 + zcash_client_backend/src/fees/common.rs | 11 +++-- zcash_client_backend/src/fees/orchard.rs | 3 -- zcash_client_backend/src/wallet.rs | 11 +---- zcash_client_sqlite/Cargo.toml | 2 +- zcash_primitives/CHANGELOG.md | 2 + .../src/transaction/components/amount.rs | 10 +++++ 8 files changed, 44 insertions(+), 39 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index c1dcddfbd..0a15aca9c 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -17,11 +17,10 @@ and this library adheres to Rust's notion of - `ScannedBlockCommitments` - `Balance::{add_spendable_value, add_pending_change_value, add_pending_spendable_value}` - `AccountBalance::{ - with_sapling_balance_mut, - with_orchard_balance_mut, + with_sapling_balance_mut, + with_orchard_balance_mut, add_unshielded_value }` - - `WalletRead::get_orchard_nullifiers` - `wallet::propose_standard_transfer_to_address` - `wallet::input_selection::Proposal::{from_parts, shielded_inputs}` - `wallet::input_selection::ShieldedInputs` @@ -59,7 +58,7 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api::{NoteId, Recipient}` have been moved into the `zcash_client_backend::wallet` module. - `ScannedBlock::{sapling_tree_size, sapling_nullifier_map, sapling_commitments}` - have been moved to `ScannedBlockSapling` and in that context are now + have been moved to `ScannedBlockSapling` and in that context are now named `{tree_size, nullifier_map, commitments}` respectively. ### Changed @@ -70,9 +69,9 @@ and this library adheres to Rust's notion of - `WalletShieldedOutput` has an additional type parameter which is used for key scope. `WalletShieldedOutput::from_parts` now takes an additional argument of this type. - - `WalletTx` has an additional type parameter as a consequence of the + - `WalletTx` has an additional type parameter as a consequence of the `WalletShieldedOutput` change. - - `ScannedBlock` has an additional type parameter as a consequence of the + - `ScannedBlock` has an additional type parameter as a consequence of the `WalletTx` change. - `ScannedBlock::metadata` has been renamed to `to_block_metadata` and now returns an owned value rather than a reference. @@ -126,9 +125,16 @@ and this library adheres to Rust's notion of argument, instead taking explicit `target_height` and `anchor_height` arguments. This helps to minimize the set of capabilities that the `data_api::InputSource` must expose. - - `WalletRead::get_checkpoint_depth` has been removed without replacement. This - is no longer needed given the change to use the stored anchor height for transaction - proposal execution. + - Changes to the `WalletRead` trait: + - Added `get_orchard_nullifiers` (under the `orchard` feature flag.) + - `get_checkpoint_depth` has been removed without replacement. This + is no longer needed given the change to use the stored anchor height for transaction + proposal execution. + - `is_valid_account_extfvk` has been removed; it was unused in + the ECC mobile wallet SDKs and has been superseded by `get_account_for_ufvk`. + - `get_spendable_sapling_notes`, `select_spendable_sapling_notes`, and + `get_unspent_transparent_outputs` have been removed; use + `data_api::InputSource` instead. - `wallet::{propose_shielding, shield_transparent_funds}` now takes their `min_confirmations` arguments as `u32` rather than a `NonZeroU32` to permit implmentations to enable zero-conf shielding. @@ -146,8 +152,10 @@ and this library adheres to Rust's notion of - `ChangeValue` is now a struct. In addition to the existing change value, it now also provides the output pool to which change should be sent and an optional memo to be associated with the change output. - - `fixed::SingleOutputChangeStrategy::new` and `zip317::SingleOutputChangeStrategy::new` - each now accept an additional `change_memo` argument + - `ChangeError` has a new `BundleError` variant. + - `fixed::SingleOutputChangeStrategy::new` and + `zip317::SingleOutputChangeStrategy::new` each now accept an additional + `change_memo` argument. - `zcash_client_backend::wallet`: - The fields of `ReceivedSaplingNote` are now private. Use `ReceivedSaplingNote::from_parts` for construction instead. Accessor methods @@ -183,7 +191,7 @@ and this library adheres to Rust's notion of - `zcash_client_backend::address`: - `RecipientAddress` has been renamed to `Address` - `Address::Shielded` has been renamed to `Address::Sapling` - - `UnifiedAddress::from_receivers` no longer takes an Orchard receiver + - `UnifiedAddress::from_receivers` no longer takes an Orchard receiver argument unless the `orchard` feature is enabled. - `UnifiedAddress::orchard` is now only available when the `orchard` feature is enabled. @@ -199,16 +207,9 @@ and this library adheres to Rust's notion of ### Removed - `zcash_client_backend::wallet::ReceivedSaplingNote` has been replaced by `zcash_client_backend::ReceivedNote`. +- `zcash_client_backend::wallet::input_selection::Proposal::sapling_inputs` has + been replaced by `Proposal::shielded_inputs` - `zcash_client_backend::data_api` - - `WalletRead::is_valid_account_extfvk` has been removed; it was unused in - the ECC mobile wallet SDKs and has been superseded by `get_account_for_ufvk`. - - `wallet::input_selection::Proposal::sapling_inputs` has been replaced - by `Proposal::shielded_inputs` -- `zcash_client_backend::data_api::WalletRead::{ - get_spendable_sapling_notes - select_spendable_sapling_notes, - get_unspent_transparent_outputs, - }` - use `data_api::InputSource` instead. - `zcash_client_backend::data_api::ScannedBlock::from_parts` has been made crate-private. - `zcash_client_backend::data_api::ScannedBlock::into_sapling_commitments` has been replaced by `into_commitments` which returns both Sapling and Orchard note commitments diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 25f2f9402..a79c216e6 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -16,6 +16,7 @@ use crate::ShieldedProtocol; pub(crate) mod common; pub mod fixed; +#[cfg(feature = "orchard")] pub mod orchard; pub mod sapling; pub mod standard; diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 63a5a1280..2bef424cb 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -87,10 +87,10 @@ where // TODO: implement a less naive strategy for selecting the pool to which change will be sent. #[cfg(feature = "orchard")] let (change_pool, sapling_change, orchard_change) = - if orchard_in > NonNegativeAmount::ZERO || orchard_out > NonNegativeAmount::ZERO { + if orchard_in.is_positive() || orchard_out.is_positive() { // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs (ShieldedProtocol::Orchard, 0, 1) - } else if sapling_in > NonNegativeAmount::ZERO || sapling_out > NonNegativeAmount::ZERO { + } else if sapling_in.is_positive() || sapling_out.is_positive() { // Otherwise, send change to Sapling if we're spending any Sapling inputs or creating any // Sapling outputs, so that we avoid pool-crossing. (ShieldedProtocol::Sapling, 1, 0) @@ -119,7 +119,10 @@ where target_height, transparent_inputs, transparent_outputs, - sapling.inputs().len(), + sapling + .bundle_type() + .num_spends(sapling.inputs().len()) + .map_err(ChangeError::BundleError)?, sapling .bundle_type() .num_outputs( @@ -139,7 +142,7 @@ where required: total_out, })?; - if proposed_change == NonNegativeAmount::ZERO { + if proposed_change.is_zero() { TransactionBalance::new(vec![], fee_amount).map_err(|_| overflow()) } else { let dust_threshold = dust_output_policy diff --git a/zcash_client_backend/src/fees/orchard.rs b/zcash_client_backend/src/fees/orchard.rs index 790bdff78..ac90a0113 100644 --- a/zcash_client_backend/src/fees/orchard.rs +++ b/zcash_client_backend/src/fees/orchard.rs @@ -4,12 +4,10 @@ use std::convert::Infallible; use zcash_primitives::transaction::components::amount::NonNegativeAmount; -#[cfg(feature = "orchard")] use orchard::builder::BundleType; /// A trait that provides a minimized view of Orchard bundle configuration /// suitable for use in fee and change calculation. -#[cfg(feature = "orchard")] pub trait BundleView { /// The type of inputs to the bundle. type In: InputView; @@ -24,7 +22,6 @@ pub trait BundleView { fn outputs(&self) -> &[Self::Out]; } -#[cfg(feature = "orchard")] impl<'a, NoteRef, In: InputView, Out: OutputView> BundleView for (BundleType, &'a [In], &'a [Out]) { diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index 9e4ca4f0c..851bc31ad 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -259,6 +259,7 @@ impl Note { } } + /// Returns the shielded protocol used by this note. pub fn protocol(&self) -> ShieldedProtocol { match self { Note::Sapling(_) => ShieldedProtocol::Sapling, @@ -340,16 +341,6 @@ impl ReceivedNote { } } -impl ReceivedNote { - pub fn protocol(&self) -> ShieldedProtocol { - match self.note() { - Note::Sapling(_) => ShieldedProtocol::Sapling, - #[cfg(feature = "orchard")] - Note::Orchard(_) => ShieldedProtocol::Orchard, - } - } -} - impl sapling_fees::InputView for ReceivedNote { fn note_id(&self) -> &NoteRef { &self.note_id diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 35d906b83..680eec5fc 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/zcash_client_sqlite/Cargo.toml @@ -22,7 +22,6 @@ rustdoc-args = ["--cfg", "docsrs"] zcash_client_backend = { workspace = true, features = ["unstable-serialization", "unstable-spanning-tree"] } zcash_encoding.workspace = true zcash_primitives.workspace = true -orchard.workspace = true # Dependencies exposed in a public API: # (Breaking upgrades to these require a breaking upgrade to this crate.) @@ -43,6 +42,7 @@ jubjub.workspace = true secrecy.workspace = true # - Shielded protocols +orchard.workspace = true sapling.workspace = true # - Note commitment trees diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 84cf87f89..ddb6765ef 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -41,6 +41,8 @@ and this library adheres to Rust's notion of - `NonNegativeAmount::const_from_u64` - `NonNegativeAmount::from_nonnegative_i64_le_bytes` - `NonNegativeAmount::to_i64_le_bytes` + - `NonNegativeAmount::is_zero` + - `NonNegativeAmount::is_positive` - `impl From<&NonNegativeAmount> for Amount` - `impl From for u64` - `impl From for zcash_primitives::sapling::value::NoteValue` diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index 3474e74ea..766368202 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -297,6 +297,16 @@ impl NonNegativeAmount { pub fn to_i64_le_bytes(self) -> [u8; 8] { self.0.to_i64_le_bytes() } + + /// Returns whether or not this `NonNegativeAmount` is the zero value. + pub fn is_zero(&self) -> bool { + self == &NonNegativeAmount::ZERO + } + + /// Returns whether or not this `NonNegativeAmount` is positive. + pub fn is_positive(&self) -> bool { + self > &NonNegativeAmount::ZERO + } } impl From for Amount {