Merge pull request #1139 from nuttycom/keys/no_default_address_request

zcash_keys: Remove `UnifiedAddressRequest::DEFAULT`

zcash_client_backend/src/zip321.rs

@@ -743,40 +743,16 @@ pub mod testing {
     use proptest::collection::btree_map;
     use proptest::collection::vec;
     use proptest::option;
-    use proptest::prelude::{any, prop_compose, prop_oneof};
+    use proptest::prelude::{any, prop_compose};
-    use proptest::strategy::Strategy;
+    use zcash_keys::address::testing::arb_addr;
-    use sapling::testing::arb_payment_address;
+    use zcash_keys::keys::UnifiedAddressRequest;
     use zcash_primitives::{
-        consensus::TEST_NETWORK, legacy::testing::arb_transparent_addr,
+        consensus::TEST_NETWORK, transaction::components::amount::testing::arb_nonnegative_amount,
-        transaction::components::amount::testing::arb_nonnegative_amount,
     };
 
-    use crate::address::{Address, UnifiedAddress};
+    use crate::address::Address;
 
     use super::{MemoBytes, Payment, TransactionRequest};
-
-    prop_compose! {
-        fn arb_unified_addr()(
-            sapling in arb_payment_address(),
-            transparent in option::of(arb_transparent_addr()),
-        ) -> UnifiedAddress {
-            UnifiedAddress::from_receivers(
-                #[cfg(feature = "orchard")]
-                None,
-                Some(sapling),
-                transparent
-            ).unwrap()
-        }
-    }
-
-    pub fn arb_addr() -> impl Strategy<Value = Address> {
-        prop_oneof![
-            arb_payment_address().prop_map(Address::Sapling),
-            arb_transparent_addr().prop_map(Address::Transparent),
-            arb_unified_addr().prop_map(Address::Unified),
-        ]
-    }
-
     pub const VALID_PARAMNAME: &str = "[a-zA-Z][a-zA-Z0-9+-]*";
 
     prop_compose! {
@@ -787,7 +763,7 @@ pub mod testing {
 
     prop_compose! {
         pub fn arb_zip321_payment()(
-            recipient_address in arb_addr(),
+            recipient_address in arb_addr(UnifiedAddressRequest::unsafe_new(false, true, true)),
             amount in arb_nonnegative_amount(),
             memo in option::of(arb_valid_memo()),
             message in option::of(any::<String>()),
@@ -826,8 +802,10 @@ pub mod testing {
     }
 
     prop_compose! {
-        pub fn arb_addr_str()(addr in arb_addr()) -> String {
+        pub fn arb_addr_str()(
-            addr.encode(&TEST_NETWORK)
+            recipient_address in arb_addr(UnifiedAddressRequest::unsafe_new(false, true, true))
+        ) -> String {
+            recipient_address.encode(&TEST_NETWORK)
         }
     }
 }
@@ -835,6 +813,7 @@ pub mod testing {
 #[cfg(test)]
 mod tests {
     use std::str::FromStr;
+    use zcash_keys::{address::testing::arb_addr, keys::UnifiedAddressRequest};
     use zcash_primitives::{
         consensus::{Parameters, TEST_NETWORK},
         memo::Memo,
@@ -863,7 +842,7 @@ mod tests {
     #[cfg(all(test, feature = "test-dependencies"))]
     use super::{
         render::{memo_param, str_param},
-        testing::{arb_addr, arb_addr_str, arb_valid_memo, arb_zip321_request, arb_zip321_uri},
+        testing::{arb_addr_str, arb_valid_memo, arb_zip321_request, arb_zip321_uri},
     };
 
     fn check_roundtrip(req: TransactionRequest) {
@@ -1101,7 +1080,7 @@ mod tests {
     #[cfg(all(test, feature = "test-dependencies"))]
     proptest! {
         #[test]
-        fn prop_zip321_roundtrip_address(addr in arb_addr()) {
+        fn prop_zip321_roundtrip_address(addr in arb_addr(UnifiedAddressRequest::unsafe_new(false, true, true))) {
             let a = addr.encode(&TEST_NETWORK);
             assert_eq!(Address::decode(&TEST_NETWORK, &a), Some(addr));
         }

zcash_client_sqlite/Cargo.toml

@@ -22,6 +22,7 @@ rustdoc-args = ["--cfg", "docsrs"]
 zcash_address.workspace = true
 zcash_client_backend = { workspace = true, features = ["unstable-serialization", "unstable-spanning-tree"] }
 zcash_encoding.workspace = true
+zcash_keys = { workspace = true, features = ["orchard"] }
 zcash_primitives.workspace = true
 
 # Dependencies exposed in a public API:

zcash_client_sqlite/src/lib.rs

@@ -107,6 +107,9 @@ pub(crate) const VERIFY_LOOKAHEAD: u32 = 10;
 
 pub(crate) const SAPLING_TABLES_PREFIX: &str = "sapling";
 
+pub const DEFAULT_UA_REQUEST: UnifiedAddressRequest =
+    UnifiedAddressRequest::unsafe_new(false, true, true);
+
 /// A newtype wrapper for received note identifiers.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub struct ReceivedNoteId(pub(crate) i64);
@@ -1116,12 +1119,9 @@ extern crate assert_matches;
 
 #[cfg(test)]
 mod tests {
-    use zcash_client_backend::{
+    use zcash_client_backend::data_api::{AccountBirthday, WalletRead, WalletWrite};
-        data_api::{AccountBirthday, WalletRead, WalletWrite},
-        keys::UnifiedAddressRequest,
-    };
 
-    use crate::{testing::TestBuilder, AccountId};
+    use crate::{testing::TestBuilder, AccountId, DEFAULT_UA_REQUEST};
 
     #[cfg(feature = "unstable")]
     use {
@@ -1145,7 +1145,7 @@ mod tests {
         // TODO: Add Orchard
         let addr2 = st
             .wallet_mut()
-            .get_next_available_address(account, UnifiedAddressRequest::DEFAULT)
+            .get_next_available_address(account, DEFAULT_UA_REQUEST)
             .unwrap();
         assert!(addr2.is_some());
         assert_ne!(current_addr, addr2);
@@ -1173,7 +1173,12 @@ mod tests {
             .unwrap();
 
         // The receiver for the default UA should be in the set.
-        assert!(receivers.contains_key(ufvk.default_address().0.transparent().unwrap()));
+        assert!(receivers.contains_key(
+            ufvk.default_address(DEFAULT_UA_REQUEST)
+                .0
+                .transparent()
+                .unwrap()
+        ));
 
         // The default t-addr should be in the set.
         assert!(receivers.contains_key(&taddr));

zcash_client_sqlite/src/wallet.rs

@@ -99,6 +99,7 @@ use zcash_client_backend::{
 };
 
 use crate::wallet::commitment_tree::{get_max_checkpointed_height, SqliteShardStore};
+use crate::DEFAULT_UA_REQUEST;
 use crate::{
     error::SqliteClientError, SqlTransaction, WalletCommitmentTrees, WalletDb, PRUNING_DEPTH,
     SAPLING_TABLES_PREFIX,
@@ -260,7 +261,7 @@ pub(crate) fn add_account<P: consensus::Parameters>(
     }
 
     // Always derive the default Unified Address for the account.
-    let (address, d_idx) = key.default_address();
+    let (address, d_idx) = key.default_address(DEFAULT_UA_REQUEST);
     insert_address(conn, params, account, d_idx, &address)?;
 
     Ok(())

zcash_client_sqlite/src/wallet/init.rs

@@ -181,7 +181,9 @@ mod tests {
         zip32::AccountId,
     };
 
-    use crate::{testing::TestBuilder, wallet::scanning::priority_code, WalletDb};
+    use crate::{
+        testing::TestBuilder, wallet::scanning::priority_code, WalletDb, DEFAULT_UA_REQUEST,
+    };
 
     use super::init_wallet_db;
 
@@ -1005,7 +1007,8 @@ mod tests {
             )?;
 
             let ufvk_str = ufvk.encode(&wdb.params);
-            let address_str = Address::Unified(ufvk.default_address().0).encode(&wdb.params);
+            let address_str =
+                Address::Unified(ufvk.default_address(DEFAULT_UA_REQUEST).0).encode(&wdb.params);
             wdb.conn.execute(
                 "INSERT INTO accounts (account, ufvk, address, transparent_address)
                 VALUES (?, ?, ?, '')",
@@ -1019,8 +1022,14 @@ mod tests {
             // add a transparent "sent note"
             #[cfg(feature = "transparent-inputs")]
             {
-                let taddr = Address::Transparent(*ufvk.default_address().0.transparent().unwrap())
+                let taddr = Address::Transparent(
-                    .encode(&wdb.params);
+                    *ufvk
+                        .default_address(DEFAULT_UA_REQUEST)
+                        .0
+                        .transparent()
+                        .unwrap(),
+                )
+                .encode(&wdb.params);
                 wdb.conn.execute(
                     "INSERT INTO blocks (height, hash, time, sapling_tree) VALUES (0, 0, 0, x'000000')",
                     [],
@@ -1060,7 +1069,7 @@ mod tests {
     #[test]
     #[cfg(feature = "transparent-inputs")]
     fn account_produces_expected_ua_sequence() {
-        use zcash_client_backend::{data_api::AccountBirthday, keys::UnifiedAddressRequest};
+        use zcash_client_backend::data_api::AccountBirthday;
 
         let network = Network::MainNetwork;
         let data_file = NamedTempFile::new().unwrap();
@@ -1090,7 +1099,7 @@ mod tests {
                 assert_eq!(tv.unified_addr, ua.encode(&Network::MainNetwork));
 
                 db_data
-                    .get_next_available_address(account, UnifiedAddressRequest::DEFAULT)
+                    .get_next_available_address(account, DEFAULT_UA_REQUEST)
                     .unwrap()
                     .expect("get_next_available_address generated an address");
             } else {

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

@@ -395,6 +395,7 @@ mod tests {
     #[test]
     #[cfg(feature = "transparent-inputs")]
     fn migrate_from_wm2() {
+        use zcash_client_backend::keys::UnifiedAddressRequest;
         use zcash_primitives::transaction::components::amount::NonNegativeAmount;
 
         let network = Network::TestNetwork;
@@ -437,7 +438,7 @@ mod tests {
 
         let usk = UnifiedSpendingKey::from_seed(&network, &[0u8; 32][..], AccountId::ZERO).unwrap();
         let ufvk = usk.to_unified_full_viewing_key();
-        let (ua, _) = ufvk.default_address();
+        let (ua, _) = ufvk.default_address(UnifiedAddressRequest::unsafe_new(false, true, true));
         let taddr = ufvk
             .transparent()
             .and_then(|k| {

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

@@ -5,6 +5,7 @@ use schemer;
 use schemer_rusqlite::RusqliteMigration;
 use uuid::Uuid;
 use zcash_client_backend::{address::Address, keys::UnifiedFullViewingKey};
+use zcash_keys::keys::UnifiedAddressRequest;
 use zcash_primitives::{consensus, zip32::AccountId};
 
 use crate::wallet::{init::WalletMigrationError, insert_address};
@@ -84,7 +85,8 @@ 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();
+            let (expected_address, idx) =
+                ufvk.default_address(UnifiedAddressRequest::unsafe_new(false, true, true));
             if decoded_address != expected_address {
                 return Err(WalletMigrationError::CorruptedData(format!(
                     "Decoded UA {} does not match the UFVK's default address {} at {:?}.",
@@ -154,7 +156,8 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                 ],
             )?;
 
-            let (address, d_idx) = ufvk.default_address();
+            let (address, d_idx) =
+                ufvk.default_address(UnifiedAddressRequest::unsafe_new(false, true, true));
             insert_address(transaction, &self.params, account, d_idx, &address)?;
         }
 

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

@@ -10,6 +10,7 @@ use uuid::Uuid;
 use zcash_client_backend::{
     address::Address, keys::UnifiedSpendingKey, PoolType, ShieldedProtocol,
 };
+use zcash_keys::keys::UnifiedAddressRequest;
 use zcash_primitives::{consensus, zip32::AccountId};
 
 #[cfg(feature = "transparent-inputs")]
@@ -64,6 +65,8 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
         let mut stmt_fetch_accounts =
             transaction.prepare("SELECT account, address FROM accounts")?;
 
+        let ua_request = UnifiedAddressRequest::new(false, true, true)
+            .expect("A shielded receiver type is requested.");
         let mut rows = stmt_fetch_accounts.query([])?;
         while let Some(row) = rows.next()? {
             // We only need to check for the presence of the seed if we have keys that
@@ -105,7 +108,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                             "Address field value decoded to a transparent address; should have been Sapling or unified.".to_string()));
                     }
                     Address::Unified(decoded_address) => {
-                        let (expected_address, idx) = ufvk.default_address();
+                        let (expected_address, idx) = ufvk.default_address(ua_request);
                         if decoded_address != expected_address {
                             return Err(WalletMigrationError::CorruptedData(
                                 format!("Decoded unified address {} does not match the ufvk's default address {} at {:?}.",
@@ -117,7 +120,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                 }
 
                 let ufvk_str: String = ufvk.encode(&self.params);
-                let address_str: String = ufvk.default_address().0.encode(&self.params);
+                let address_str: String = ufvk.default_address(ua_request).0.encode(&self.params);
 
                 // This migration, and the wallet behaviour before it, stored the default
                 // transparent address in the `accounts` table. This does not necessarily

zcash_keys/src/address.rs

@@ -273,6 +273,32 @@ impl Address {
     }
 }
 
+#[cfg(any(test, feature = "test-dependencies"))]
+pub mod testing {
+    use proptest::prelude::*;
+    use sapling::testing::arb_payment_address;
+    use zcash_primitives::{consensus::Network, legacy::testing::arb_transparent_addr};
+
+    use crate::keys::{testing::arb_unified_spending_key, UnifiedAddressRequest};
+
+    use super::{Address, UnifiedAddress};
+
+    pub fn arb_unified_addr(
+        params: Network,
+        request: UnifiedAddressRequest,
+    ) -> impl Strategy<Value = UnifiedAddress> {
+        arb_unified_spending_key(params).prop_map(move |k| k.default_address(request).0)
+    }
+
+    pub fn arb_addr(request: UnifiedAddressRequest) -> impl Strategy<Value = Address> {
+        prop_oneof![
+            arb_payment_address().prop_map(Address::Sapling),
+            arb_transparent_addr().prop_map(Address::Transparent),
+            arb_unified_addr(Network::TestNetwork, request).prop_map(Address::Unified),
+        ]
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use zcash_address::test_vectors;

zcash_keys/src/keys.rs

@@ -182,7 +182,7 @@ impl UnifiedSpendingKey {
 
         #[cfg(feature = "orchard")]
         let orchard =
-            orchard::keys::SpendingKey::from_zip32_seed(seed, params.coin_type(), account.into())
+            orchard::keys::SpendingKey::from_zip32_seed(seed, params.coin_type(), account)
                 .map_err(DerivationError::Orchard)?;
 
         #[cfg(feature = "transparent-inputs")]
@@ -380,8 +380,11 @@ impl UnifiedSpendingKey {
     }
 
     #[cfg(feature = "test-dependencies")]
-    pub fn default_address(&self) -> (UnifiedAddress, DiversifierIndex) {
+    pub fn default_address(
-        self.to_unified_full_viewing_key().default_address()
+        &self,
+        request: UnifiedAddressRequest,
+    ) -> (UnifiedAddress, DiversifierIndex) {
+        self.to_unified_full_viewing_key().default_address(request)
     }
 
     #[cfg(all(feature = "test-dependencies", feature = "transparent-inputs"))]
@@ -405,6 +408,9 @@ pub enum AddressGenerationError {
     /// A requested address typecode was not recognized, so we are unable to generate the address
     /// as requested.
     ReceiverTypeNotSupported(Typecode),
+    /// A requested address typecode was recognized, but the unified key being used to generate the
+    /// address lacks an item of the requested type.
+    KeyNotAvailable(Typecode),
     /// A Unified address cannot be generated without at least one shielded receiver being
     /// included.
     ShieldedReceiverRequired,
@@ -413,44 +419,41 @@ pub enum AddressGenerationError {
 /// Specification for how a unified address should be generated from a unified viewing key.
 #[derive(Clone, Copy, Debug)]
 pub struct UnifiedAddressRequest {
-    #[cfg(feature = "orchard")]
+    _has_orchard: bool,
-    has_orchard: bool,
     has_sapling: bool,
-    #[cfg(feature = "transparent-inputs")]
     has_p2pkh: bool,
 }
 
 impl UnifiedAddressRequest {
-    pub const DEFAULT: UnifiedAddressRequest = Self {
+    /// Construct a new unified address request from its constituent parts
-        #[cfg(feature = "orchard")]
+    pub fn new(has_orchard: bool, has_sapling: bool, has_p2pkh: bool) -> Option<Self> {
-        has_orchard: false, // FIXME: Always request Orchard receivers once we can receive Orchard funds
-        has_sapling: true,
-        #[cfg(feature = "transparent-inputs")]
-        has_p2pkh: true,
-    };
-
-    pub fn new(
-        #[cfg(feature = "orchard")] has_orchard: bool,
-        has_sapling: bool,
-        #[cfg(feature = "transparent-inputs")] has_p2pkh: bool,
-    ) -> Option<Self> {
-        #[cfg(feature = "orchard")]
         let has_shielded_receiver = has_orchard || has_sapling;
-        #[cfg(not(feature = "orchard"))]
-        let has_shielded_receiver = has_sapling;
 
         if !has_shielded_receiver {
             None
         } else {
             Some(Self {
-                #[cfg(feature = "orchard")]
+                _has_orchard: has_orchard,
-                has_orchard,
                 has_sapling,
-                #[cfg(feature = "transparent-inputs")]
                 has_p2pkh,
             })
         }
     }
+
+    /// Construct a new unified address request from its constituent parts.
+    ///
+    /// Panics: at least one of `has_orchard` or `has_sapling` must be `true`.
+    pub const fn unsafe_new(has_orchard: bool, has_sapling: bool, has_p2pkh: bool) -> Self {
+        if !(has_orchard || has_sapling) {
+            panic!("At least one shielded receiver must be requested.")
+        }
+
+        Self {
+            _has_orchard: has_orchard,
+            has_sapling,
+            has_p2pkh,
+        }
+    }
 }
 
 /// A [ZIP 316](https://zips.z.cash/zip-0316) unified full viewing key.
@@ -626,51 +629,65 @@ impl UnifiedFullViewingKey {
         request: UnifiedAddressRequest,
     ) -> Result<UnifiedAddress, AddressGenerationError> {
         #[cfg(feature = "orchard")]
-        let orchard = {
+        let orchard = if request._has_orchard {
-            let orchard_j = orchard::keys::DiversifierIndex::from(*j.as_bytes());
+            if let Some(ofvk) = &self.orchard {
-            self.orchard
+                let orchard_j = orchard::keys::DiversifierIndex::from(*j.as_bytes());
-                .as_ref()
+                Some(ofvk.address_at(orchard_j, Scope::External))
-                .filter(|_| request.has_orchard)
+            } else {
-                .map(|ofvk| ofvk.address_at(orchard_j, Scope::External))
+                return Err(AddressGenerationError::KeyNotAvailable(Typecode::Orchard));
-        };
+            }
-
+        } else {
-        let sapling = if let Some(extfvk) = self.sapling.as_ref().filter(|_| request.has_sapling) {
+            None
-            // If a Sapling receiver type is requested, we must be able to construct an
+        };
-            // address; if we're unable to do so, then no Unified Address exists at this
+
-            // diversifier and we use `?` to early-return from this method.
+        let sapling = if request.has_sapling {
-            Some(
+            if let Some(extfvk) = &self.sapling {
-                extfvk
+                // If a Sapling receiver type is requested, we must be able to construct an
-                    .address(j)
+                // address; if we're unable to do so, then no Unified Address exists at this
-                    .ok_or(AddressGenerationError::InvalidSaplingDiversifierIndex(j))?,
+                // diversifier and we use `?` to early-return from this method.
-            )
+                Some(
-        } else {
+                    extfvk
-            None
+                        .address(j)
-        };
+                        .ok_or(AddressGenerationError::InvalidSaplingDiversifierIndex(j))?,
-
+                )
-        #[cfg(feature = "transparent-inputs")]
+            } else {
-        let transparent = if let Some(tfvk) =
+                return Err(AddressGenerationError::KeyNotAvailable(Typecode::Sapling));
-            self.transparent.as_ref().filter(|_| request.has_p2pkh)
+            }
-        {
+        } else {
-            // If a transparent receiver type is requested, we must be able to construct an
+            None
-            // address; if we're unable to do so, then no Unified Address exists at this
+        };
-            // diversifier.
+
-            match to_transparent_child_index(j) {
+        let transparent = if request.has_p2pkh {
-                Some(transparent_j) => match tfvk
+            #[cfg(not(feature = "transparent-inputs"))]
-                    .derive_external_ivk()
+            return Err(AddressGenerationError::ReceiverTypeNotSupported(
-                    .and_then(|tivk| tivk.derive_address(transparent_j))
+                Typecode::P2pkh,
-                {
+            ));
-                    Ok(taddr) => Some(taddr),
+
-                    Err(_) => return Err(AddressGenerationError::InvalidTransparentChildIndex(j)),
+            #[cfg(feature = "transparent-inputs")]
-                },
+            if let Some(tfvk) = self.transparent.as_ref() {
-                // Diversifier doesn't generate a valid transparent child index, so we eagerly
+                // If a transparent receiver type is requested, we must be able to construct an
-                // return `None`.
+                // address; if we're unable to do so, then no Unified Address exists at this
-                None => return Err(AddressGenerationError::InvalidTransparentChildIndex(j)),
+                // diversifier.
+                match to_transparent_child_index(j) {
+                    Some(transparent_j) => match tfvk
+                        .derive_external_ivk()
+                        .and_then(|tivk| tivk.derive_address(transparent_j))
+                    {
+                        Ok(taddr) => Some(taddr),
+                        Err(_) => {
+                            return Err(AddressGenerationError::InvalidTransparentChildIndex(j))
+                        }
+                    },
+                    // Diversifier doesn't generate a valid transparent child index, so we eagerly
+                    // return `None`.
+                    None => return Err(AddressGenerationError::InvalidTransparentChildIndex(j)),
+                }
+            } else {
+                return Err(AddressGenerationError::KeyNotAvailable(Typecode::P2pkh));
             }
         } else {
             None
         };
-        #[cfg(not(feature = "transparent-inputs"))]
-        let transparent = None;
 
         UnifiedAddress::from_receivers(
             #[cfg(feature = "orchard")]
@@ -720,9 +737,11 @@ impl UnifiedFullViewingKey {
 
     /// Returns the Unified Address corresponding to the smallest valid diversifier index,
     /// along with that index.
-    pub fn default_address(&self) -> (UnifiedAddress, DiversifierIndex) {
+    pub fn default_address(
-        // FIXME: Enable Orchard keys
+        &self,
-        self.find_address(DiversifierIndex::new(), UnifiedAddressRequest::DEFAULT)
+        request: UnifiedAddressRequest,
+    ) -> (UnifiedAddress, DiversifierIndex) {
+        self.find_address(DiversifierIndex::new(), request)
             .expect("UFVK should have at least one valid diversifier")
     }
 }
@@ -913,7 +932,7 @@ mod tests {
             }
 
             let ua = ufvk
-                .address(d_idx, UnifiedAddressRequest::DEFAULT)
+                .address(d_idx, UnifiedAddressRequest::unsafe_new(false, true, true))
                 .unwrap_or_else(|err| {
                     panic!(
                         "unified address generation failed for account {}: {:?}",