Apply suggestions from code review

Co-authored-by: str4d <thestr4d@gmail.com>
Co-authored-by: Daira Emma Hopwood <daira@jacaranda.org>

zcash_client_backend/CHANGELOG.md

@@ -28,8 +28,7 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::proto::`
   - `PROPOSAL_SER_V1`
   - `ProposalError`
-  - `proposal::Proposal::{from_standard_proposal, try_into_standard_proposal}`
+  - `proposal` module, for parsing and serializing transaction proposals.
-  - `proposal::ProposedInput::parse_txid`
 - `impl Clone for zcash_client_backend::{
      zip321::{Payment, TransactionRequest, Zip321Error, parse::Param, parse::IndexedParam},
      wallet::{ReceivedSaplingNote, WalletTransparentOutput},

zcash_client_backend/proto/proposal.proto

@@ -31,7 +31,7 @@ message Proposal {
 }
 
 message SaplingInputs {
-    // Returns the Sapling anchor height to be used in creating the transaction.
+    // The Sapling anchor height to be used in creating the transaction
     uint32 anchorHeight = 1;
     // The unique identifier and amount for each proposed Sapling input
     repeated ProposedInput inputs = 2;
@@ -46,7 +46,7 @@ message ProposedInput {
 
 // The fee rule used in constructing a Proposal
 enum FeeRule {
-    // Protobuf requires that enums have a zero descriminant as the default
+    // Protobuf requires that enums have a zero discriminant as the default
     // value. However, we need to require that a known fee rule is selected,
     // and we do not want to fall back to any default, so sending the
     // FeeRuleNotSpecified value will be treated as an error.

zcash_client_backend/src/data_api.rs

@@ -222,8 +222,11 @@ pub trait SaplingInputSource {
     /// or a UUID.
     type NoteRef: Copy + Debug + Eq + Ord;
 
-    /// Returns a received Sapling note, or Ok(None) if the note is not known to belong to the
+    /// Fetches a spendable Sapling note by indexing into the specified transaction's
-    /// wallet or if the note is not spendable.
+    /// [`sapling::Bundle::shielded_outputs`].
+    ///
+    /// Returns `Ok(None)` if the note is not known to belong to the wallet or if the note
+    /// is not spendable.
     fn get_spendable_sapling_note(
         &self,
         txid: &TxId,
@@ -247,8 +250,10 @@ pub trait TransparentInputSource {
     /// The type of errors produced by a wallet backend.
     type Error;
 
-    /// Returns a received transparent UTXO, or Ok(None) if the UTXO is not known to belong to the
+    /// Fetches a spendable transparent output.
-    /// wallet or is not spendable.
+    ///
+    /// Returns `Ok(None)` if the UTXO is not known to belong to the wallet or is not
+    /// spendable.
     fn get_unspent_transparent_output(
         &self,
         outpoint: &OutPoint,

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -90,10 +90,25 @@ pub struct Proposal<FeeRuleT, NoteRef> {
     is_shielding: bool,
 }
 
+/// Errors that
+#[derive(Debug, Clone)]
+pub enum ProposalError {
+    RequestInvalid,
+    Overflow,
+    BalanceError {
+        input_total: NonNegativeAmount,
+        output_total: NonNegativeAmount,
+    },
+    ShieldingFlagInvalid,
+}
+
 impl<FeeRuleT, NoteRef> Proposal<FeeRuleT, NoteRef> {
-    /// Constructs a [`Proposal`] from its constituent parts.
+    /// Constructs a validated [`Proposal`] from its constituent parts.
+    ///
+    /// This operation validaes the proposal for balance consistency and agreement between
+    /// the `is_shielding` flag and the structure of the proposal.
     #[allow(clippy::too_many_arguments)]
-    pub(crate) fn from_parts(
+    pub fn from_parts(
         transaction_request: TransactionRequest,
         transparent_inputs: Vec<WalletTransparentOutput>,
         sapling_inputs: Option<SaplingInputs<NoteRef>>,
@@ -101,19 +116,35 @@ impl<FeeRuleT, NoteRef> Proposal<FeeRuleT, NoteRef> {
         fee_rule: FeeRuleT,
         min_target_height: BlockHeight,
         is_shielding: bool,
-    ) -> Result<Self, ()> {
+    ) -> Result<Self, ProposalError> {
-        let transparent_total = transparent_inputs
+        let transparent_input_total = transparent_inputs
             .iter()
             .map(|out| out.txout().value)
-            .fold(Ok(NonNegativeAmount::ZERO), |acc, a| (acc? + a).ok_or(()))?;
+            .fold(Ok(NonNegativeAmount::ZERO), |acc, a| {
-        let sapling_total = sapling_inputs
+                (acc? + a).ok_or(ProposalError::Overflow)
+            })?;
+        let sapling_input_total = sapling_inputs
             .iter()
             .flat_map(|s_in| s_in.notes().iter())
             .map(|out| out.value())
-            .fold(Ok(NonNegativeAmount::ZERO), |acc, a| (acc? + a).ok_or(()))?;
+            .fold(Ok(NonNegativeAmount::ZERO), |acc, a| {
-        let input_total = (transparent_total + sapling_total).ok_or(())?;
+                (acc? + a).ok_or(ProposalError::Overflow)
+            })?;
+        let input_total =
+            (transparent_input_total + sapling_input_total).ok_or(ProposalError::Overflow)?;
 
-        let output_total = (transaction_request.total()? + balance.total()).ok_or(())?;
+        let request_total = transaction_request
+            .total()
+            .map_err(|_| ProposalError::RequestInvalid)?;
+        let output_total = (request_total + balance.total()).ok_or(ProposalError::Overflow)?;
+
+        if is_shielding
+            && (transparent_input_total == NonNegativeAmount::ZERO
+                || sapling_input_total > NonNegativeAmount::ZERO
+                || request_total > NonNegativeAmount::ZERO)
+        {
+            return Err(ProposalError::ShieldingFlagInvalid);
+        }
 
         if input_total == output_total {
             Ok(Self {
@@ -126,7 +157,10 @@ impl<FeeRuleT, NoteRef> Proposal<FeeRuleT, NoteRef> {
                 is_shielding,
             })
         } else {
-            Err(())
+            Err(ProposalError::BalanceError {
+                input_total,
+                output_total,
+            })
         }
     }
 

zcash_client_backend/src/proto.rs

@@ -1,6 +1,6 @@
 //! Generated code for handling light client protobuf structs.
 
-use std::io;
+use std::{array::TryFromSliceError, io};
 
 use incrementalmerkletree::frontier::CommitmentTree;
 
@@ -22,7 +22,7 @@ use zcash_note_encryption::{EphemeralKeyBytes, COMPACT_NOTE_SIZE};
 
 use crate::{
     data_api::{
-        wallet::input_selection::{Proposal, SaplingInputs},
+        wallet::input_selection::{Proposal, ProposalError, SaplingInputs},
         PoolType, SaplingInputSource, ShieldedProtocol, TransparentInputSource,
     },
     fees::{ChangeValue, TransactionBalance},
@@ -207,10 +207,10 @@ impl service::TreeState {
 
 pub const PROPOSAL_SER_V1: u32 = 1;
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone)]
-pub enum ProposalError<DbError> {
+pub enum ProposalDecodingError<DbError> {
     Zip321(Zip321Error),
-    TxIdInvalid(Vec<u8>),
+    TxIdInvalid(TryFromSliceError),
     InputRetrieval(DbError),
     InputNotFound(TxId, PoolType, u32),
     BalanceInvalid,
@@ -218,17 +218,18 @@ pub enum ProposalError<DbError> {
     VersionInvalid(u32),
     ZeroMinConf,
     FeeRuleNotSpecified,
+    ProposalInvalid(ProposalError),
 }
 
-impl<E> From<Zip321Error> for ProposalError<E> {
+impl<E> From<Zip321Error> for ProposalDecodingError<E> {
     fn from(value: Zip321Error) -> Self {
         Self::Zip321(value)
     }
 }
 
 impl proposal::ProposedInput {
-    pub fn parse_txid(&self) -> Result<TxId, Vec<u8>> {
+    pub fn parse_txid(&self) -> Result<TxId, TryFromSliceError> {
-        Ok(TxId::from_bytes(self.txid.clone().try_into()?))
+        Ok(TxId::from_bytes(self.txid[..].try_into()?))
     }
 }
 
@@ -311,7 +312,7 @@ impl proposal::Proposal {
         &self,
         params: &P,
         wallet_db: &DbT,
-    ) -> Result<Proposal<StandardFeeRule, DbT::NoteRef>, ProposalError<DbError>>
+    ) -> Result<Proposal<StandardFeeRule, DbT::NoteRef>, ProposalDecodingError<DbError>>
     where
         DbT: TransparentInputSource<Error = DbError> + SaplingInputSource<Error = DbError>,
     {
@@ -323,7 +324,7 @@ impl proposal::Proposal {
                     proposal::FeeRule::Zip313 => StandardFeeRule::Zip313,
                     proposal::FeeRule::Zip317 => StandardFeeRule::Zip317,
                     proposal::FeeRule::NotSpecified => {
-                        return Err(ProposalError::FeeRuleNotSpecified);
+                        return Err(ProposalDecodingError::FeeRuleNotSpecified);
                     }
                 };
 
@@ -333,14 +334,16 @@ impl proposal::Proposal {
                     .transparent_inputs
                     .iter()
                     .map(|t_in| {
-                        let txid = t_in.parse_txid().map_err(ProposalError::TxIdInvalid)?;
+                        let txid = t_in
+                            .parse_txid()
+                            .map_err(ProposalDecodingError::TxIdInvalid)?;
                         let outpoint = OutPoint::new(txid.into(), t_in.index);
 
                         wallet_db
                             .get_unspent_transparent_output(&outpoint)
-                            .map_err(ProposalError::InputRetrieval)?
+                            .map_err(ProposalDecodingError::InputRetrieval)?
                             .ok_or_else(|| {
-                                ProposalError::InputNotFound(
+                                ProposalDecodingError::InputNotFound(
                                     txid,
                                     PoolType::Transparent,
                                     t_in.index,
@@ -353,14 +356,16 @@ impl proposal::Proposal {
                     s_in.inputs
                         .iter()
                         .map(|s_in| {
-                            let txid = s_in.parse_txid().map_err(ProposalError::TxIdInvalid)?;
+                            let txid = s_in
+                                .parse_txid()
+                                .map_err(ProposalDecodingError::TxIdInvalid)?;
 
                             wallet_db
                                 .get_spendable_sapling_note(&txid, s_in.index)
-                                .map_err(ProposalError::InputRetrieval)
+                                .map_err(ProposalDecodingError::InputRetrieval)
                                 .and_then(|opt| {
                                     opt.ok_or_else(|| {
-                                        ProposalError::InputNotFound(
+                                        ProposalDecodingError::InputNotFound(
                                             txid,
                                             PoolType::Shielded(ShieldedProtocol::Sapling),
                                             s_in.index,
@@ -377,37 +382,42 @@ impl proposal::Proposal {
                         .transpose()
                 });
 
-                let balance = self.balance.as_ref().ok_or(ProposalError::BalanceInvalid)?;
+                let proto_balance = self
+                    .balance
+                    .as_ref()
+                    .ok_or(ProposalDecodingError::BalanceInvalid)?;
                 let balance = TransactionBalance::new(
-                    balance
+                    proto_balance
                         .proposed_change
                         .iter()
                         .filter_map(|cv| {
-                            cv.value
+                            cv.value.as_ref().map(
-                                .as_ref()
+                                |cv| -> Result<ChangeValue, ProposalDecodingError<_>> {
-                                .map(|cv| -> Result<ChangeValue, ProposalError<_>> {
                                     match cv {
                                         proposal::change_value::Value::SaplingValue(sc) => {
                                             Ok(ChangeValue::sapling(
-                                                NonNegativeAmount::from_u64(sc.amount)
+                                                NonNegativeAmount::from_u64(sc.amount).map_err(
-                                                    .map_err(|_| ProposalError::BalanceInvalid)?,
+                                                    |_| ProposalDecodingError::BalanceInvalid,
+                                                )?,
                                                 sc.memo
                                                     .as_ref()
                                                     .map(|bytes| {
-                                                        MemoBytes::from_bytes(&bytes.value)
+                                                        MemoBytes::from_bytes(&bytes.value).map_err(
-                                                            .map_err(ProposalError::MemoInvalid)
+                                                            ProposalDecodingError::MemoInvalid,
+                                                        )
                                                     })
                                                     .transpose()?,
                                             ))
                                         }
                                     }
-                                })
+                                },
+                            )
                         })
                         .collect::<Result<Vec<_>, _>>()?,
-                    NonNegativeAmount::from_u64(balance.fee_required)
+                    NonNegativeAmount::from_u64(proto_balance.fee_required)
-                        .map_err(|_| ProposalError::BalanceInvalid)?,
+                        .map_err(|_| ProposalDecodingError::BalanceInvalid)?,
                 )
-                .map_err(|_| ProposalError::BalanceInvalid)?;
+                .map_err(|_| ProposalDecodingError::BalanceInvalid)?;
 
                 Proposal::from_parts(
                     transaction_request,
@@ -418,9 +428,9 @@ impl proposal::Proposal {
                     self.min_target_height.into(),
                     self.is_shielding,
                 )
-                .map_err(|_| ProposalError::BalanceInvalid)
+                .map_err(ProposalDecodingError::ProposalInvalid)
             }
-            other => Err(ProposalError::VersionInvalid(other)),
+            other => Err(ProposalDecodingError::VersionInvalid(other)),
         }
     }
 }

zcash_client_backend/src/zip321.rs

@@ -154,8 +154,8 @@ impl TransactionRequest {
 
     /// Returns the total value of payments to be made.
     ///
-    /// Returns `Err` in the case of overflow, if payment values are negative, or the value is
+    /// Returns `Err` in the case of overflow, or the value is
-    /// outside the valid range of Zcash values. .
+    /// outside the range `0..=MAX_MONEY` zatoshis.
     pub fn total(&self) -> Result<NonNegativeAmount, ()> {
         if self.payments.is_empty() {
             Ok(NonNegativeAmount::ZERO)

zcash_client_sqlite/src/lib.rs

@@ -206,15 +206,9 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> TransparentInput
 
     fn get_unspent_transparent_output(
         &self,
-        _outpoint: &OutPoint,
+        outpoint: &OutPoint,
     ) -> Result<Option<WalletTransparentOutput>, Self::Error> {
-        #[cfg(feature = "transparent-inputs")]
+        wallet::get_unspent_transparent_output(self.conn.borrow(), outpoint)
-        return wallet::get_unspent_transparent_output(self.conn.borrow(), _outpoint);
-
-        #[cfg(not(feature = "transparent-inputs"))]
-        panic!(
-            "The wallet must be compiled with the transparent-inputs feature to use this method."
-        );
     }
 
     fn get_unspent_transparent_outputs(

zcash_client_sqlite/src/wallet/sapling.rs

@@ -135,8 +135,8 @@ fn to_spendable_note(row: &Row) -> Result<ReceivedSaplingNote<ReceivedNoteId>, S
     ))
 }
 
-// The `clippy::let_and_return` lint is explicitly allowed here because a bug in the Rust compiler
+// The `clippy::let_and_return` lint is explicitly allowed here because a bug in Clippy
-// (https://github.com/rust-lang/rust/issues/114633) fails to identify that the `result` temporary
+// (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_sapling_note(