Merge pull request #1423 from daira/pre-zip-320-refactoring

Cleanups, refactoring, and placeholder for TEX addresses

components/zcash_address/src/kind/unified/address.rs

@@ -1,4 +1,4 @@
-use zcash_protocol::{PoolType, ShieldedProtocol};
+use zcash_protocol::PoolType;
 
 use super::{private::SealedItem, ParseError, Typecode};
 
@@ -107,9 +107,9 @@ impl Address {
     /// Returns whether this address has the ability to receive transfers of the given pool type.
     pub fn has_receiver_of_type(&self, pool_type: PoolType) -> bool {
         self.0.iter().any(|r| match r {
-            Receiver::Orchard(_) => pool_type == PoolType::Shielded(ShieldedProtocol::Orchard),
-            Receiver::Sapling(_) => pool_type == PoolType::Shielded(ShieldedProtocol::Sapling),
-            Receiver::P2pkh(_) | Receiver::P2sh(_) => pool_type == PoolType::Transparent,
+            Receiver::Orchard(_) => pool_type == PoolType::ORCHARD,
+            Receiver::Sapling(_) => pool_type == PoolType::SAPLING,
+            Receiver::P2pkh(_) | Receiver::P2sh(_) => pool_type == PoolType::TRANSPARENT,
             Receiver::Unknown { .. } => false,
         })
     }

components/zcash_address/src/lib.rs

@@ -143,7 +143,7 @@ pub use encoding::ParseError;
 pub use kind::unified;
 use kind::unified::Receiver;
 pub use zcash_protocol::consensus::NetworkType as Network;
-use zcash_protocol::{PoolType, ShieldedProtocol};
+use zcash_protocol::PoolType;
 
 /// A Zcash address.
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
@@ -274,9 +274,9 @@ impl ZcashAddress {
         use AddressKind::*;
         match &self.kind {
             Sprout(_) => false,
-            Sapling(_) => pool_type == PoolType::Shielded(ShieldedProtocol::Sapling),
+            Sapling(_) => pool_type == PoolType::SAPLING,
             Unified(addr) => addr.has_receiver_of_type(pool_type),
-            P2pkh(_) | P2sh(_) | Tex(_) => pool_type == PoolType::Transparent,
+            P2pkh(_) | P2sh(_) | Tex(_) => pool_type == PoolType::TRANSPARENT,
         }
     }
 

components/zcash_encoding/CHANGELOG.md

@@ -6,6 +6,9 @@ and this library adheres to Rust's notion of
 [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Added
+- `zcash_encoding::CompactSize::serialized_size`
+- `zcash_encoding::Vector::serialized_size_of_u8_vec`
 
 ## [0.2.0] - 2022-10-19
 ### Changed

components/zcash_encoding/src/lib.rs

@@ -93,6 +93,16 @@ impl CompactSize {
             }
         }
     }
+
+    /// Returns the number of bytes needed to encode the given size in compact form.
+    pub fn serialized_size(size: usize) -> usize {
+        match size {
+            s if s < 253 => 1,
+            s if s <= 0xFFFF => 3,
+            s if s <= 0xFFFFFFFF => 5,
+            _ => 9,
+        }
+    }
 }
 
 /// Namespace for functions that perform encoding of vectors.
@@ -171,6 +181,12 @@ impl Vector {
         CompactSize::write(&mut writer, items.len())?;
         items.try_for_each(|e| func(&mut writer, e))
     }
+
+    /// Returns the serialized size of a vector of `u8` as written by `[Vector::write]`.
+    pub fn serialized_size_of_u8_vec(vec: &[u8]) -> usize {
+        let length = vec.len();
+        CompactSize::serialized_size(length) + length
+    }
 }
 
 /// Namespace for functions that perform encoding of array contents.
@@ -279,8 +295,11 @@ mod tests {
             <T as TryInto<usize>>::Error: Debug,
         {
             let mut data = vec![];
-            CompactSize::write(&mut data, value.try_into().unwrap()).unwrap();
+            let value_usize: usize = value.try_into().unwrap();
+            CompactSize::write(&mut data, value_usize).unwrap();
             assert_eq!(&data[..], expected);
+            let serialized_size = CompactSize::serialized_size(value_usize);
+            assert_eq!(serialized_size, expected.len());
             let result: io::Result<T> = CompactSize::read_t(&data[..]);
             match result {
                 Ok(n) => assert_eq!(n, value),
@@ -308,6 +327,8 @@ mod tests {
             let mut data = vec![];
             CompactSize::write(&mut data, value).unwrap();
             assert_eq!(&data[..], encoded);
+            let serialized_size = CompactSize::serialized_size(value);
+            assert_eq!(serialized_size, encoded.len());
             assert!(CompactSize::read(encoded).is_err());
         }
     }
@@ -320,6 +341,8 @@ mod tests {
                 let mut data = vec![];
                 Vector::write(&mut data, &$value, |w, e| w.write_u8(*e)).unwrap();
                 assert_eq!(&data[..], &$expected[..]);
+                let serialized_size = Vector::serialized_size_of_u8_vec(&$value);
+                assert_eq!(serialized_size, $expected.len());
                 match Vector::read(&data[..], |r| r.read_u8()) {
                     Ok(v) => assert_eq!(v, $value),
                     Err(e) => panic!("Unexpected error: {:?}", e),

zcash_client_backend/CHANGELOG.md

@@ -10,6 +10,10 @@ and this library adheres to Rust's notion of
 ### Added
 - `zcash_client_backend::data_api`:
   - `chain::BlockCache` trait, behind the `sync` feature flag.
+- `zcash_client_backend::fees`:
+  - `ChangeValue::{transparent, shielded}`
+  - `sapling::EmptyBundleView`
+  - `orchard::EmptyBundleView`
 - `zcash_client_backend::scanning`:
   - `testing` module
 - `zcash_client_backend::sync` module, behind the `sync` feature flag.
@@ -26,6 +30,16 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::proto::proposal::Proposal::{from_standard_proposal, 
   try_into_standard_proposal}` each no longer require a `consensus::Parameters` 
   argument.
+- `zcash_client_backend::data_api::fees`
+  - The return type of `ChangeValue::output_pool`, and the type of the
+    `output_pool` argument to `ChangeValue::new`, have changed from
+    `ShieldedProtocol` to `zcash_protocol::PoolType`.
+  - The return type of `ChangeValue::new` is now optional; it returns `None`
+    if a memo is given for the transparent pool. Use `ChangeValue::shielded`
+    to avoid this error case when creating a `ChangeValue` known to be for a
+    shielded pool.
+- `zcash_client_backend::input_selection::GreedyInputSelectorError` has a
+  new variant `UnsupportedTexAddress`.
 - `zcash_client_backend::wallet::Recipient` variants have changed. Instead of
   wrapping protocol-address types, the `Recipient` type now wraps a
   `zcash_address::ZcashAddress`. This simplifies the process of tracking the
@@ -42,7 +56,7 @@ and this library adheres to Rust's notion of
 
 ### Added
 - A new `orchard` feature flag has been added to make it possible to
-  build client code without `orchard` dependendencies. Additions and
+  build client code without `orchard` dependencies. Additions and
   changes related to `Orchard` below are introduced under this feature
   flag.
 - `zcash_client_backend::data_api`:
@@ -232,7 +246,7 @@ and this library adheres to Rust's notion of
 ### Changed
 - Migrated to `zcash_primitives 0.14`, `orchard 0.7`.
 - Several structs and functions now take an `AccountId` type parameter
-  parameter in order to decouple the concept of an account identifier from
+  in order to decouple the concept of an account identifier from
   the ZIP 32 account index. Many APIs that previously referenced
   `zcash_primitives::zip32::AccountId` now reference the generic type.
   Impacted types and functions are:
@@ -518,7 +532,7 @@ and this library adheres to Rust's notion of
   - `chain::CommitmentTreeRoot`
   - `scanning` A new module containing types required for `suggest_scan_ranges`
   - `testing::MockWalletDb::new`
-  - `wallet::input_sellection::Proposal::{min_target_height, min_anchor_height}`
+  - `wallet::input_selection::Proposal::{min_target_height, min_anchor_height}`
   - `SAPLING_SHARD_HEIGHT` constant
 - `zcash_client_backend::proto::compact_formats`:
   - `impl<A: sapling::Authorization> From<&sapling::SpendDescription<A>> for CompactSaplingSpend`
@@ -616,7 +630,7 @@ and this library adheres to Rust's notion of
   - `wallet::input_selection`:
     - `Proposal::target_height` (use `Proposal::min_target_height` instead).
 - `zcash_client_backend::data_api::chain::validate_chain` (logic merged into
-  `chain::scan_cached_blocks`.
+  `chain::scan_cached_blocks`).
 - `zcash_client_backend::data_api::chain::error::{ChainError, Cause}` have been
   replaced by `zcash_client_backend::scanning::ScanError`
 - `zcash_client_backend::proto::compact_formats`:
@@ -771,8 +785,8 @@ and this library adheres to Rust's notion of
   - `WalletWrite::get_next_available_address`
   - `WalletWrite::put_received_transparent_utxo`
   - `impl From<prost::DecodeError> for error::Error`
-  - `chain::error`: a module containing error types type that that can occur only
-    in chain validation and sync have been separated out from errors related to
+  - `chain::error`: a module containing error types that can occur only
+    in chain validation and sync, separated out from errors related to
     other wallet operations.
   - `input_selection`: a module containing types related to the process
     of selecting inputs to be spent, given a transaction request.
@@ -805,7 +819,7 @@ and this library adheres to Rust's notion of
   likely to be modified and/or moved to a different module in a future
   release:
   - `zcash_client_backend::address::UnifiedAddress`
-  - `zcash_client_backend::keys::{UnifiedSpendingKey`, `UnifiedFullViewingKey`, `Era`, `DecodingError`}
+  - `zcash_client_backend::keys::{UnifiedSpendingKey, UnifiedFullViewingKey, Era, DecodingError}`
   - `zcash_client_backend::encoding::AddressCodec`
   - `zcash_client_backend::encoding::encode_payment_address`
   - `zcash_client_backend::encoding::encode_transparent_address`

zcash_client_backend/src/data_api.rs

@@ -1514,8 +1514,11 @@ pub trait WalletWrite: WalletRead {
         received_tx: DecryptedTransaction<Self::AccountId>,
     ) -> Result<(), Self::Error>;
 
-    /// Saves information about a transaction that was constructed and sent by the wallet to the
-    /// persistent wallet store.
+    /// Saves information about a transaction constructed by the wallet to the persistent
+    /// wallet store.
+    ///
+    /// The name `store_sent_tx` is somewhat misleading; this must be called *before* the
+    /// transaction is sent to the network.
     fn store_sent_tx(
         &mut self,
         sent_tx: &SentTransaction<Self::AccountId>,

zcash_client_backend/src/data_api/error.rs

@@ -85,6 +85,8 @@ pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError> {
     /// An error occurred parsing the address from a payment request.
     Address(ConversionError<&'static str>),
 
+    /// The address associated with a record being inserted was not recognized as
+    /// belonging to the wallet.
     #[cfg(feature = "transparent-inputs")]
     AddressNotRecognized(TransparentAddress),
 }

zcash_client_backend/src/data_api/wallet.rs

@@ -289,6 +289,13 @@ where
     )
 }
 
+type ErrorT<DbT, InputsErrT, FeeRuleT> = Error<
+    <DbT as WalletRead>::Error,
+    <DbT as WalletCommitmentTrees>::Error,
+    InputsErrT,
+    <FeeRuleT as FeeRule>::Error,
+>;
+
 /// Constructs a transaction or series of transactions that send funds as specified
 /// by the `request` argument, stores them to the wallet's "sent transactions" data
 /// store, and returns the [`TxId`] for each transaction constructed.
@@ -353,15 +360,7 @@ pub fn spend<DbT, ParamsT, InputsT>(
     request: zip321::TransactionRequest,
     ovk_policy: OvkPolicy,
     min_confirmations: NonZeroU32,
-) -> Result<
-    NonEmpty<TxId>,
-    Error<
-        <DbT as WalletRead>::Error,
-        <DbT as WalletCommitmentTrees>::Error,
-        InputsT::Error,
-        <InputsT::FeeRule as FeeRule>::Error,
-    >,
->
+) -> Result<NonEmpty<TxId>, ErrorT<DbT, InputsT::Error, InputsT::FeeRule>>
 where
     DbT: InputSource,
     DbT: WalletWrite<
@@ -592,15 +591,7 @@ pub fn create_proposed_transactions<DbT, ParamsT, InputsErrT, FeeRuleT, N>(
     usk: &UnifiedSpendingKey,
     ovk_policy: OvkPolicy,
     proposal: &Proposal<FeeRuleT, N>,
-) -> Result<
-    NonEmpty<TxId>,
-    Error<
-        <DbT as WalletRead>::Error,
-        <DbT as WalletCommitmentTrees>::Error,
-        InputsErrT,
-        FeeRuleT::Error,
-    >,
->
+) -> Result<NonEmpty<TxId>, ErrorT<DbT, InputsErrT, FeeRuleT>>
 where
     DbT: WalletWrite + WalletCommitmentTrees,
     ParamsT: consensus::Parameters + Clone,
@@ -645,15 +636,7 @@ fn create_proposed_transaction<DbT, ParamsT, InputsErrT, FeeRuleT, N>(
     min_target_height: BlockHeight,
     prior_step_results: &[(&proposal::Step<N>, BuildResult)],
     proposal_step: &proposal::Step<N>,
-) -> Result<
-    BuildResult,
-    Error<
-        <DbT as WalletRead>::Error,
-        <DbT as WalletCommitmentTrees>::Error,
-        InputsErrT,
-        FeeRuleT::Error,
-    >,
->
+) -> Result<BuildResult, ErrorT<DbT, InputsErrT, FeeRuleT>>
 where
     DbT: WalletWrite + WalletCommitmentTrees,
     ParamsT: consensus::Parameters + Clone,
@@ -980,10 +963,7 @@ where
                             memo.clone(),
                         )?;
                         orchard_output_meta.push((
-                            Recipient::External(
-                                payment.recipient_address().clone(),
-                                PoolType::Shielded(ShieldedProtocol::Orchard),
-                            ),
+                            Recipient::External(payment.recipient_address().clone(), *output_pool),
                             payment.amount(),
                             Some(memo),
                         ));
@@ -997,10 +977,7 @@ where
                             memo.clone(),
                         )?;
                         sapling_output_meta.push((
-                            Recipient::External(
-                                payment.recipient_address().clone(),
-                                PoolType::Shielded(ShieldedProtocol::Sapling),
-                            ),
+                            Recipient::External(payment.recipient_address().clone(), *output_pool),
                             payment.amount(),
                             Some(memo),
                         ));
@@ -1044,15 +1021,19 @@ where
                     payment.amount(),
                 ));
             }
+            Address::Tex(_) => {
+                return Err(Error::ProposalNotSupported);
+            }
         }
     }
 
     for change_value in proposal_step.balance().proposed_change() {
         let memo = change_value
             .memo()
             .map_or_else(MemoBytes::empty, |m| m.clone());
-        match change_value.output_pool() {
-            ShieldedProtocol::Sapling => {
+        let output_pool = change_value.output_pool();
+        match output_pool {
+            PoolType::Shielded(ShieldedProtocol::Sapling) => {
                 builder.add_sapling_output(
                     sapling_internal_ovk(),
                     sapling_dfvk.change_address().1,
@@ -1063,17 +1044,15 @@ where
                     Recipient::InternalAccount {
                         receiving_account: account,
                         external_address: None,
-                        note: PoolType::Shielded(ShieldedProtocol::Sapling),
+                        note: output_pool,
                     },
                     change_value.value(),
                     Some(memo),
                 ))
             }
-            ShieldedProtocol::Orchard => {
+            PoolType::Shielded(ShieldedProtocol::Orchard) => {
                 #[cfg(not(feature = "orchard"))]
-                return Err(Error::UnsupportedChangeType(PoolType::Shielded(
-                    ShieldedProtocol::Orchard,
-                )));
+                return Err(Error::UnsupportedChangeType(output_pool));
 
                 #[cfg(feature = "orchard")]
                 {
@@ -1087,13 +1066,16 @@ where
                         Recipient::InternalAccount {
                             receiving_account: account,
                             external_address: None,
-                            note: PoolType::Shielded(ShieldedProtocol::Orchard),
+                            note: output_pool,
                         },
                         change_value.value(),
                         Some(memo),
                     ))
                 }
             }
+            PoolType::Transparent => {
+                return Err(Error::UnsupportedChangeType(output_pool));
+            }
         }
     }
 
@@ -1115,7 +1097,7 @@ where
 
                 let recipient = recipient
                     .map_internal_account_note(|pool| {
-                        assert!(pool == PoolType::Shielded(ShieldedProtocol::Orchard));
+                        assert!(pool == PoolType::ORCHARD);
                         build_result
                             .transaction()
                             .orchard_bundle()
@@ -1145,7 +1127,7 @@ where
 
                 let recipient = recipient
                     .map_internal_account_note(|pool| {
-                        assert!(pool == PoolType::Shielded(ShieldedProtocol::Sapling));
+                        assert!(pool == PoolType::SAPLING);
                         build_result
                             .transaction()
                             .sapling_bundle()