From 8c6e3ef18aa60858b44c5e9f73859aa1aecbe118 Mon Sep 17 00:00:00 2001
From: Kris Nuttycombe <kris@nutty.land>
Date: Tue, 10 Dec 2024 16:35:21 -0700
Subject: [PATCH] zcash_keys: Make unified key construction APIs consistent
 with one another.

---
 zcash_client_sqlite/src/lib.rs |  8 ++++++++
 zcash_keys/CHANGELOG.md        |  5 +++++
 zcash_keys/src/keys.rs         | 28 +++++++++++++++++-----------
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs
index a6f02d51e1..dabc48f5d4 100644
--- a/zcash_client_sqlite/src/lib.rs
+++ b/zcash_client_sqlite/src/lib.rs
@@ -2066,6 +2066,10 @@ mod tests {
                 ufvk.sapling().cloned(),
                 #[cfg(feature = "orchard")]
                 ufvk.orchard().cloned(),
+                vec![],
+                None,
+                None,
+                vec![],
             )
             .unwrap();
             assert_matches!(
@@ -2090,6 +2094,10 @@ mod tests {
                 ufvk.transparent().cloned(),
                 ufvk.sapling().cloned(),
                 None,
+                vec![],
+                None,
+                None,
+                vec![],
             )
             .unwrap();
             assert_matches!(
diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md
index 749e1ffca1..f493acbb5a 100644
--- a/zcash_keys/CHANGELOG.md
+++ b/zcash_keys/CHANGELOG.md
@@ -56,6 +56,11 @@ and this library adheres to Rust's notion of
   - `zcash_keys::keys::UnifiedFullViewingKey::new`
   - `zcash_keys::keys::UnifiedFullViewingKey::from_orchard_fvk`
   - `zcash_keys::keys::UnifiedFullViewingKey::from_sapling_extended_full_viewing_key`
+- `zcash_keys::keys::UnifiedFullViewingKey::new` has been modified to allow
+  construction that includes unknown data as well as metadata fields.
+- `zcash_keys::keys::UnifiedIncomingViewingKey::new` now returns a
+  `Result<UnifiedIncomingViewingKey, UnifiedKeyError>` instead of a
+  potentially invalid value.
 
 ### Removed
 - `zcash_keys::address::UnifiedAddress::unknown` (use `unknown_data` instead.)
diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs
index 34823bcdde..0959545a5d 100644
--- a/zcash_keys/src/keys.rs
+++ b/zcash_keys/src/keys.rs
@@ -721,7 +721,10 @@ impl UnifiedFullViewingKey {
         #[cfg(feature = "transparent-inputs")] transparent: Option<legacy::AccountPubKey>,
         #[cfg(feature = "sapling")] sapling: Option<sapling::DiversifiableFullViewingKey>,
         #[cfg(feature = "orchard")] orchard: Option<orchard::keys::FullViewingKey>,
-        // TODO: Implement construction of UFVKs with metadata items.
+        unknown_data: Vec<(u32, Vec<u8>)>,
+        expiry_height: Option<BlockHeight>,
+        expiry_time: Option<u64>,
+        unknown_metadata: Vec<(u32, Vec<u8>)>,
     ) -> Result<UnifiedFullViewingKey, UnifiedKeyError> {
         Self::from_checked_parts(
             #[cfg(feature = "transparent-inputs")]
@@ -730,12 +733,10 @@ impl UnifiedFullViewingKey {
             sapling,
             #[cfg(feature = "orchard")]
             orchard,
-            // We don't currently allow constructing new UFVKs with unknown items, but we store
-            // this to allow parsing such UFVKs.
-            vec![],
-            None,
-            None,
-            vec![],
+            unknown_data,
+            expiry_height,
+            expiry_time,
+            unknown_metadata,
         )
     }
 
@@ -1174,8 +1175,8 @@ impl UnifiedIncomingViewingKey {
         expiry_height: Option<BlockHeight>,
         expiry_time: Option<u64>,
         unknown_metadata: Vec<(u32, Vec<u8>)>,
-    ) -> UnifiedIncomingViewingKey {
-        UnifiedIncomingViewingKey {
+    ) -> Result<UnifiedIncomingViewingKey, UnifiedKeyError> {
+        Self::from_checked_parts(
             #[cfg(feature = "transparent-inputs")]
             transparent,
             #[cfg(feature = "sapling")]
@@ -1186,7 +1187,7 @@ impl UnifiedIncomingViewingKey {
             expiry_height,
             expiry_time,
             unknown_metadata,
-        }
+        )
     }
 
     fn from_checked_parts(
@@ -1756,6 +1757,10 @@ mod tests {
             sapling,
             #[cfg(feature = "orchard")]
             orchard,
+            vec![],
+            None,
+            None,
+            vec![],
         );
 
         let ufvk = ufvk.expect("Orchard or Sapling fvk is present.");
@@ -1949,7 +1954,8 @@ mod tests {
             None,
             None,
             vec![],
-        );
+        )
+        .unwrap();
 
         let encoded = uivk.render().encode(&NetworkType::Main);