Only permit transparent-only Unified keys & addresses with associated…

… metadata.
zcash · Nov 7, 2024 · 1fccdf0 · 1fccdf0
1 parent 0462594
commit 1fccdf0
Show file tree

Hide file tree

Showing 4 changed files with 207 additions and 48 deletions.
diff --git a/zcash_client_backend/src/data_api/testing/orchard.rs b/zcash_client_backend/src/data_api/testing/orchard.rs
@@ -75,6 +75,7 @@ impl ShieldedPoolTester for OrchardPoolTester {
             None,
             None,
         )
+        .expect("Address construction is valid")
         .into()
     }
 
@@ -154,7 +155,9 @@ impl ShieldedPoolTester for OrchardPoolTester {
                 return result.map(|(note, addr, memo)| {
                     (
                         Note::Orchard(note),
-                        UnifiedAddress::from_receivers(Some(addr), None, None).into(),
+                        UnifiedAddress::from_receivers(Some(addr), None, None)
+                            .expect("Address construction is valid")
+                            .into(),
                         MemoBytes::from_bytes(&memo).expect("correct length"),
                     )
                 });

diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md
@@ -6,9 +6,15 @@ and this library adheres to Rust's notion of
 
 ## [Unreleased]
 
+### Added
+- `zcash_keys::keys::UnifiedKeyError`
+
 ### Changed
 - MSRV is now 1.77.0.
 
+### Removed
+- `zcash_keys::keys::DerivationError` use `UnifiedKeyError` instead.
+
 ## [0.4.0] - 2024-10-04
 
 ### Added

diff --git a/zcash_keys/src/address.rs b/zcash_keys/src/address.rs
@@ -93,7 +93,7 @@ impl TryFrom<unified::Address> for UnifiedAddress {
             }
         }
 
-        Ok(Self {
+        Self::from_checked_parts(
             #[cfg(feature = "orchard")]
             orchard,
             #[cfg(feature = "sapling")]
@@ -103,7 +103,7 @@ impl TryFrom<unified::Address> for UnifiedAddress {
             expiry_height,
             expiry_time,
             unknown_metadata,
-        })
+        ).ok_or("Transparent-only unified addresses without additional metadata fields are not permitted.")
     }
 }
 
@@ -118,36 +118,54 @@ impl UnifiedAddress {
         #[cfg(feature = "orchard")] orchard: Option<orchard::Address>,
         #[cfg(feature = "sapling")] sapling: Option<PaymentAddress>,
         transparent: Option<TransparentAddress>,
-    ) -> Self {
-        Self::new_internal(
+    ) -> Option<Self> {
+        Self::from_checked_parts(
             #[cfg(feature = "orchard")]
             orchard,
             #[cfg(feature = "sapling")]
             sapling,
             transparent,
+            vec![],
             None,
             None,
+            vec![],
         )
     }
 
-    pub(crate) fn new_internal(
+    pub(crate) fn from_checked_parts(
         #[cfg(feature = "orchard")] orchard: Option<orchard::Address>,
         #[cfg(feature = "sapling")] sapling: Option<PaymentAddress>,
         transparent: Option<TransparentAddress>,
+        unknown_data: Vec<(u32, Vec<u8>)>,
         expiry_height: Option<BlockHeight>,
         expiry_time: Option<u64>,
-    ) -> Self {
-        Self {
+        unknown_metadata: Vec<(u32, Vec<u8>)>,
+    ) -> Option<Self> {
+        #[allow(unused_mut)]
+        let mut has_shielded = false;
+        #[cfg(feature = "sapling")]
+        {
+            has_shielded = has_shielded || sapling.is_some();
+        }
+        #[cfg(feature = "orchard")]
+        {
+            has_shielded = has_shielded || orchard.is_some();
+        }
+
+        let has_metadata =
+            expiry_height.is_some() || expiry_time.is_some() || !unknown_metadata.is_empty();
+
+        (has_shielded || (transparent.is_some() && has_metadata)).then(|| Self {
             #[cfg(feature = "orchard")]
             orchard,
             #[cfg(feature = "sapling")]
             sapling,
             transparent,
-            unknown_data: vec![],
+            unknown_data,
             expiry_height,
             expiry_time,
-            unknown_metadata: vec![],
-        }
+            unknown_metadata,
+        })
     }
 
     /// Returns whether this address has an Orchard receiver.
@@ -592,15 +610,25 @@ mod tests {
         let transparent = None;
 
         #[cfg(all(feature = "orchard", feature = "sapling"))]
-        let ua = UnifiedAddress::new_internal(orchard, sapling, transparent, None, None);
+        let ua = UnifiedAddress::from_checked_parts(
+            orchard,
+            sapling,
+            transparent,
+            vec![],
+            None,
+            None,
+            vec![],
+        );
 
         #[cfg(all(not(feature = "orchard"), feature = "sapling"))]
-        let ua = UnifiedAddress::new_internal(sapling, transparent, None, None);
+        let ua =
+            UnifiedAddress::from_checked_parts(sapling, transparent, vec![], None, None, vec![]);
 
         #[cfg(all(feature = "orchard", not(feature = "sapling")))]
-        let ua = UnifiedAddress::new_internal(orchard, transparent, None, None);
+        let ua =
+            UnifiedAddress::from_checked_parts(orchard, transparent, vec![], None, None, vec![]);
 
-        let addr = Address::from(ua);
+        let addr = Address::from(ua.expect("test UAs are constructed in valid configurations"));
         let addr_str = addr.encode(&MAIN_NETWORK);
         assert_eq!(Address::decode(&MAIN_NETWORK, &addr_str), Some(addr));
     }
@@ -609,7 +637,10 @@ mod tests {
     #[cfg(not(any(feature = "orchard", feature = "sapling")))]
     fn ua_round_trip() {
         let transparent = None;
-        assert_eq!(UnifiedAddress::new_internal(transparent, None, None), None)
+        assert_eq!(
+            UnifiedAddress::from_checked_parts(transparent, vec![], None, None, vec![]),
+            None
+        )
     }
 
     #[test]