Apply suggestions from code review

Co-authored-by: str4d <[email protected]>
Co-authored-by: Daira-Emma Hopwood <[email protected]>

components/zcash_address/CHANGELOG.md

@@ -7,12 +7,6 @@ and this library adheres to Rust's notion of
 
 ## [Unreleased]
 
-### Removed
-- `zcash_address::kind::p2pkh` - use constants from `zcash_protocol` instead.
-- `zcash_address::kind::p2sh` - use constants from `zcash_protocol` instead.
-- `zcash_address::kind::sapling` - use constants from `zcash_protocol` instead.
-- `zcash_address::kind::sprout` - use constants from `zcash_protocol` instead.
-
 ## [0.3.1] - 2024-01-12
 ### Fixed
 - Stubs for `zcash_address::convert` traits that are created by `rust-analyzer`

components/zcash_protocol/CHANGELOG.md

@@ -16,9 +16,13 @@ The entries below are relative to the `zcash_primitives` crate as of the tag
   - `constants`
   - `zcash_protocol::value` replaces `zcash_primitives::transaction::components::amount`
 - `zcash_protocol::consensus`:
-  - `NetworkConstants` has been extracted from the `Parameters` trait.
+  - `NetworkConstants` has been extracted from the `Parameters` trait. Relative to the
+    state prior to the extraction, the bech32 prefixes now return `&'static str` instead
+    of `&str`.
   - `NetworkType`
   - `Parameters::b58_sprout_address_prefix`
+- `zcash_protocol::consensus`:
+  - `impl Hash for LocalNetwork`
 - `zcash_protocol::constants::{mainnet, testnet}::B58_SPROUT_ADDRESS_PREFIX`
 - Added in `zcash_protocol::value`:
   - `Zatoshis`
@@ -40,6 +44,8 @@ The entries below are relative to the `zcash_primitives` crate as of the tag
     - `TryFrom<orchard::ValueSum> for Amount`
     - `From<NonNegativeAmount> for sapling::value::NoteValue>`
     - `TryFrom<sapling::value::NoteValue> for NonNegativeAmount`
+  - `impl AddAssign for NonNegativeAmount`
+  - `impl SubAssign for NonNegativeAmount`
 - `zcash_protocol::consensus::Parameters` has been split into two traits, with
   the `NetworkConstants` trait providing all network constant accessors. Also,
   the `address_network` method has been replaced with a new `network_type`

components/zcash_protocol/Cargo.toml

@@ -37,13 +37,22 @@ proptest.workspace = true
 
 [features]
 ## Exposes APIs that are useful for testing, such as `proptest` strategies.
-test-dependencies = ["dep:proptest", "incrementalmerkletree/test-dependencies"]
+test-dependencies = [
+    "dep:incrementalmerkletree",
+    "dep:proptest",
+    "incrementalmerkletree?/test-dependencies",
+]
+
+## Exposes support for working with a local consensus (e.g. regtest).
+local-consensus = []
+
+#! ### Experimental features
+#!
+#! ⚠️ Enabling these features will likely make your code incompatible with current Zcash
+#! consensus rules!
 
 ## Exposes the in-development NU6 features.
 unstable-nu6 = []
 
 ## Exposes early in-development features that are not yet planned for any network upgrade.
 zfuture = []
-
-## Exposes support for working with a local consensus (e.g. regtest
-local-consensus = []

components/zcash_protocol/LICENSE-MIT

@@ -1,6 +1,6 @@
 The MIT License (MIT)
 
-Copyright (c) 2021 Electric Coin Company
+Copyright (c) 2021-2024 Electric Coin Company
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal

components/zcash_protocol/src/consensus.rs

@@ -8,9 +8,6 @@ use std::ops::{Add, Bound, RangeBounds, Sub};
 
 use crate::constants::{mainnet, regtest, testnet};
 
-#[cfg(feature = "local-consensus")]
-use crate::local_consensus::LocalNetwork;
-
 /// A wrapper type representing blockchain heights.
 ///
 /// Safe conversion from various integer types, as well as addition and subtraction, are
@@ -146,7 +143,7 @@ pub trait NetworkConstants: Clone {
     fn coin_type(&self) -> u32;
 
     /// Returns the human-readable prefix for Bech32-encoded Sapling extended spending keys
-    /// the network to which this NetworkConstants value applies.
+    /// for the network to which this NetworkConstants value applies.
     ///
     /// Defined in [ZIP 32].
     ///
@@ -164,7 +161,7 @@ pub trait NetworkConstants: Clone {
     fn hrp_sapling_extended_full_viewing_key(&self) -> &'static str;
 
     /// Returns the Bech32-encoded human-readable prefix for Sapling payment addresses
-    /// viewing keys for the network to which this NetworkConstants value applies.
+    /// for the network to which this NetworkConstants value applies.
     ///
     /// Defined in section 5.6.4 of the [Zcash Protocol Specification].
     ///
@@ -382,9 +379,6 @@ pub enum Network {
     MainNetwork,
     /// Zcash Testnet.
     TestNetwork,
-    /// Private integration / regression testing, used in `zcashd`.
-    #[cfg(feature = "local-consensus")]
-    Regtest(LocalNetwork),
 }
 
 memuse::impl_no_dynamic_usage!(Network);
@@ -394,17 +388,13 @@ impl Parameters for Network {
         match self {
             Network::MainNetwork => NetworkType::Main,
             Network::TestNetwork => NetworkType::Test,
-            #[cfg(feature = "local-consensus")]
-            Network::Regtest(_) => NetworkType::Regtest,
         }
     }
 
     fn activation_height(&self, nu: NetworkUpgrade) -> Option<BlockHeight> {
         match self {
             Network::MainNetwork => MAIN_NETWORK.activation_height(nu),
             Network::TestNetwork => TEST_NETWORK.activation_height(nu),
-            #[cfg(feature = "local-consensus")]
-            Network::Regtest(network_params) => network_params.activation_height(nu),
         }
     }
 }

components/zcash_protocol/src/constants/mainnet.rs

@@ -17,7 +17,7 @@ pub const HRP_SAPLING_EXTENDED_SPENDING_KEY: &str = "secret-extended-key-main";
 ///
 /// Defined in [ZIP 32].
 ///
-/// [`ExtendedFullViewingKey`]: https://docs.rs/sapling-crypto/latest/sapling_crypto/zip32/struct.ExtendedFullViewingKey.html#
+/// [`ExtendedFullViewingKey`]: https://docs.rs/sapling-crypto/latest/sapling_crypto/zip32/struct.ExtendedFullViewingKey.html
 /// [ZIP 32]: https://github.com/zcash/zips/blob/master/zip-0032.rst
 pub const HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY: &str = "zxviews";
 

components/zcash_protocol/src/constants/regtest.rs

@@ -21,7 +21,7 @@ pub const HRP_SAPLING_EXTENDED_SPENDING_KEY: &str = "secret-extended-key-regtest
 ///
 /// It is defined in [the `zcashd` codebase].
 ///
-/// [`ExtendedFullViewingKey`]: https://docs.rs/sapling-crypto/latest/sapling_crypto/zip32/struct.ExtendedFullViewingKey.html#
+/// [`ExtendedFullViewingKey`]: https://docs.rs/sapling-crypto/latest/sapling_crypto/zip32/struct.ExtendedFullViewingKey.html
 /// [the `zcashd` codebase]: <https://github.com/zcash/zcash/blob/128d863fb8be39ee294fda397c1ce3ba3b889cb2/src/chainparams.cpp#L494>
 pub const HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY: &str = "zxviewregtestsapling";
 
@@ -33,7 +33,7 @@ pub const HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY: &str = "zxviewregtestsapling";
 /// [the `zcashd` codebase]: <https://github.com/zcash/zcash/blob/128d863fb8be39ee294fda397c1ce3ba3b889cb2/src/chainparams.cpp#L493>
 pub const HRP_SAPLING_PAYMENT_ADDRESS: &str = "zregtestsapling";
 
-/// The prefix for a Base58Check-encoded regtest Sprout address
+/// The prefix for a Base58Check-encoded regtest Sprout address.
 ///
 /// Defined in the [Zcash Protocol Specification section 5.6.3][sproutpaymentaddrencoding].
 /// As is specified, this is identical to the testnet prefix.

components/zcash_protocol/src/constants/testnet.rs

@@ -17,7 +17,7 @@ pub const HRP_SAPLING_EXTENDED_SPENDING_KEY: &str = "secret-extended-key-test";
 ///
 /// Defined in [ZIP 32].
 ///
-/// [`ExtendedFullViewingKey`]: https://docs.rs/sapling-crypto/latest/sapling_crypto/zip32/struct.ExtendedFullViewingKey.html#
+/// [`ExtendedFullViewingKey`]: https://docs.rs/sapling-crypto/latest/sapling_crypto/zip32/struct.ExtendedFullViewingKey.html
 /// [ZIP 32]: https://github.com/zcash/zips/blob/master/zip-0032.rst
 pub const HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY: &str = "zxviewtestsapling";
 
@@ -29,7 +29,7 @@ pub const HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY: &str = "zxviewtestsapling";
 /// [Zcash Protocol Specification]: https://github.com/zcash/zips/blob/master/protocol/protocol.pdf
 pub const HRP_SAPLING_PAYMENT_ADDRESS: &str = "ztestsapling";
 
-/// The prefix for a Base58Check-encoded testnet Sprout address
+/// The prefix for a Base58Check-encoded testnet Sprout addresses.
 ///
 /// Defined in the [Zcash Protocol Specification section 5.6.3][sproutpaymentaddrencoding].
 ///

components/zcash_protocol/src/value.rs

@@ -1,7 +1,7 @@
 use std::convert::{Infallible, TryFrom};
 use std::error;
 use std::iter::Sum;
-use std::ops::{Add, AddAssign, Mul, Neg, Sub, SubAssign};
+use std::ops::{Add, Mul, Neg, Sub};
 
 use memuse::DynamicUsage;
 
@@ -12,12 +12,9 @@ pub const MAX_BALANCE: i64 = MAX_MONEY as i64;
 /// A type-safe representation of a Zcash value delta, in zatoshis.
 ///
 /// An ZatBalance can only be constructed from an integer that is within the valid monetary
-/// range of `{-MAX_MONEY..MAX_MONEY}` (where `MAX_MONEY` = 21,000,000 × 10⁸ zatoshis).
-/// However, this range is not preserved as an invariant internally; it is possible to
-/// add two valid ZatBalances together to obtain an invalid ZatBalance. It is the user's
-/// responsibility to handle the result of serializing potentially-invalid ZatBalances. In
-/// particular, a [`Transaction`] containing serialized invalid ZatBalances will be rejected
-/// by the network consensus rules.
+/// range of `{-MAX_MONEY..MAX_MONEY}` (where `MAX_MONEY` = 21,000,000 × 10⁸ zatoshis),
+/// and this is preserved as an invariant internally. (A [`Transaction`] containing serialized
+/// invalid ZatBalances would also be rejected by the network consensus rules.)
 ///
 /// [`Transaction`]: https://docs.rs/zcash_primitives/latest/zcash_primitives/transaction/struct.Transaction.html
 #[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Eq, Ord)]
@@ -174,12 +171,6 @@ impl Add<ZatBalance> for Option<ZatBalance> {
     }
 }
 
-impl AddAssign<ZatBalance> for ZatBalance {
-    fn add_assign(&mut self, rhs: ZatBalance) {
-        *self = (*self + rhs).expect("Addition must produce a valid amount value.")
-    }
-}
-
 impl Sub<ZatBalance> for ZatBalance {
     type Output = Option<ZatBalance>;
 
@@ -196,12 +187,6 @@ impl Sub<ZatBalance> for Option<ZatBalance> {
     }
 }
 
-impl SubAssign<ZatBalance> for ZatBalance {
-    fn sub_assign(&mut self, rhs: ZatBalance) {
-        *self = (*self - rhs).expect("Subtraction must produce a valid amount value.")
-    }
-}
-
 impl Sum<ZatBalance> for Option<ZatBalance> {
     fn sum<I: Iterator<Item = ZatBalance>>(iter: I) -> Self {
         iter.fold(Some(ZatBalance::zero()), |acc, a| acc? + a)
@@ -272,7 +257,7 @@ impl Zatoshis {
     ///
     /// Returns an error if the amount is outside the range `{0..MAX_MONEY}`.
     pub fn from_nonnegative_i64(amount: i64) -> Result<Self, ()> {
-        u64::try_from(amount).map(Zatoshis).map_err(|_| ())
+        Self::from_u64(u64::try_from(amount).map_err(|_| ())?)
     }
 
     /// Reads an Zatoshis from an unsigned 64-bit little-endian integer.
@@ -516,23 +501,9 @@ mod tests {
         assert_eq!(v + ZatBalance(1), None)
     }
 
-    #[test]
-    #[should_panic]
-    fn add_assign_panics_on_overflow() {
-        let mut a = ZatBalance(MAX_BALANCE);
-        a += ZatBalance(1);
-    }
-
     #[test]
     fn sub_underflow() {
         let v = ZatBalance(-MAX_BALANCE);
         assert_eq!(v - ZatBalance(1), None)
     }
-
-    #[test]
-    #[should_panic]
-    fn sub_assign_panics_on_underflow() {
-        let mut a = ZatBalance(-MAX_BALANCE);
-        a -= ZatBalance(1);
-    }
 }

zcash_client_sqlite/src/lib.rs

@@ -264,15 +264,10 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
                 // Keys are not comparable with `Eq`, but addresses are, so we derive what should
                 // be equivalent addresses for each key and use those to check for key equality.
                 UnifiedAddressRequest::all().map_or(Ok(false), |ua_request| {
-                    match (
-                        usk.to_unified_full_viewing_key()
-                            .default_address(ua_request),
-                        ufvk.default_address(ua_request),
-                    ) {
-                        (Ok(a), Ok(b)) => Ok(a == b),
-                        (Err(e), _) => Err(e.into()),
-                        (_, Err(e)) => Err(e.into()),
-                    }
+                    Ok(usk
+                        .to_unified_full_viewing_key()
+                        .default_address(ua_request)?
+                        == ufvk.default_address(ua_request)?)
                 })
             })
         })

zcash_client_sqlite/src/wallet/init.rs

@@ -2,21 +2,18 @@
 
 use std::fmt;
 
-use rusqlite::{self};
 use schemer::{Migrator, MigratorError};
 use schemer_rusqlite::RusqliteAdapter;
 use secrecy::SecretVec;
 use shardtree::error::ShardTreeError;
 use uuid::Uuid;
 
-use zcash_primitives::{
-    consensus::{self},
-    transaction::components::amount::BalanceError,
-};
+use zcash_client_backend::keys::AddressGenerationError;
+use zcash_primitives::{consensus, transaction::components::amount::BalanceError};
 
 use crate::WalletDb;
 
-use super::commitment_tree::{self};
+use super::commitment_tree;
 
 mod migrations;
 
@@ -28,6 +25,9 @@ pub enum WalletMigrationError {
     /// Decoding of an existing value from its serialized form has failed.
     CorruptedData(String),
 
+    /// An error occurred in migrating a Zcash address or key.
+    AddressGeneration(AddressGenerationError),
+
     /// Wrapper for rusqlite errors.
     DbError(rusqlite::Error),
 
@@ -56,6 +56,12 @@ impl From<ShardTreeError<commitment_tree::Error>> for WalletMigrationError {
     }
 }
 
+impl From<AddressGenerationError> for WalletMigrationError {
+    fn from(e: AddressGenerationError) -> Self {
+        WalletMigrationError::AddressGeneration(e)
+    }
+}
+
 impl fmt::Display for WalletMigrationError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match &self {
@@ -71,6 +77,9 @@ impl fmt::Display for WalletMigrationError {
             WalletMigrationError::DbError(e) => write!(f, "{}", e),
             WalletMigrationError::BalanceError(e) => write!(f, "Balance error: {:?}", e),
             WalletMigrationError::CommitmentTree(e) => write!(f, "Commitment tree error: {:?}", e),
+            WalletMigrationError::AddressGeneration(e) => {
+                write!(f, "Address generation error: {:?}", e)
+            }
         }
     }
 }
@@ -79,6 +88,9 @@ impl std::error::Error for WalletMigrationError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match &self {
             WalletMigrationError::DbError(e) => Some(e),
+            WalletMigrationError::BalanceError(e) => Some(e),
+            WalletMigrationError::CommitmentTree(e) => Some(e),
+            WalletMigrationError::AddressGeneration(e) => Some(e),
             _ => None,
         }
     }

zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs

@@ -88,13 +88,9 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                     "Address in accounts table was not a Unified Address.".to_string(),
                 ));
             };
-            let (expected_address, idx) = ufvk
-                .default_address(UnifiedAddressRequest::unsafe_new(
-                    false,
-                    true,
-                    UA_TRANSPARENT,
-                ))
-                .expect("A valid default address exists for the UFVK");
+            let (expected_address, idx) = ufvk.default_address(
+                UnifiedAddressRequest::unsafe_new(false, true, UA_TRANSPARENT),
+            )?;
             if decoded_address != expected_address {
                 return Err(WalletMigrationError::CorruptedData(format!(
                     "Decoded UA {} does not match the UFVK's default address {} at {:?}.",
@@ -164,13 +160,11 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                 ],
             )?;
 
-            let (address, d_idx) = ufvk
-                .default_address(UnifiedAddressRequest::unsafe_new(
-                    false,
-                    true,
-                    UA_TRANSPARENT,
-                ))
-                .expect("A valid default address exists for the UFVK");
+            let (address, d_idx) = ufvk.default_address(UnifiedAddressRequest::unsafe_new(
+                false,
+                true,
+                UA_TRANSPARENT,
+            ))?;
             insert_address(transaction, &self.params, account, d_idx, &address)?;
         }