Improve error reporting for address & viewing key decoding.

zcash_client_backend/CHANGELOG.md

@@ -37,6 +37,7 @@ and this library adheres to Rust's notion of
   - `TransactionRequest::new` for constructing a request from `Vec<Payment>`.
   - `TransactionRequest::payments` for accessing the `Payments` that make up a
     request.
+- `zcash_client_backend::encoding::KeyError` 
 - New experimental APIs that should be considered unstable, and are
   likely to be modified and/or moved to a different module in a future
   release:
@@ -113,7 +114,11 @@ and this library adheres to Rust's notion of
   derived from ZIP 316 UFVKs and UIVKs.
 - `welding_rig::scan_block` now uses batching for trial-decryption of
   transaction outputs.
-
+- The return type of the following methods in `zcash_client_backend::encoding`
+  have been changed to improve error reporting:
+  - `decode_extended_spending_key`
+  - `decode_extended_full_viewing_key`
+  - `decode_payment_address`
 
 ### Removed
 - `zcash_client_backend::data_api`:

zcash_client_backend/examples/diversify-address.rs

@@ -9,16 +9,12 @@ use zcash_primitives::{
 
 fn parse_viewing_key(s: &str) -> Result<(ExtendedFullViewingKey, bool), &'static str> {
     decode_extended_full_viewing_key(mainnet::HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY, s)
-        .ok()
-        .flatten()
         .map(|vk| (vk, true))
-        .or_else(|| {
+        .or_else(|_| {
             decode_extended_full_viewing_key(testnet::HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY, s)
-                .ok()
-                .flatten()
                 .map(|vk| (vk, false))
         })
-        .ok_or("Invalid Sapling viewing key")
+        .map_err(|_| "Invalid Sapling viewing key")
 }
 
 fn parse_diversifier_index(s: &str) -> Result<DiversifierIndex, &'static str> {

zcash_client_backend/src/encoding.rs

@@ -26,15 +26,55 @@ where
     bech32::encode(hrp, data.to_base32(), Variant::Bech32).expect("hrp is invalid")
 }
 
-fn bech32_decode<T, F>(hrp: &str, s: &str, read: F) -> Result<Option<T>, Error>
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum Bech32DecodeError {
+    Bech32Error(Error),
+    IncorrectVariant(Variant),
+    ReadError,
+    HrpMismatch { expected: String, actual: String },
+}
+
+impl From<Error> for Bech32DecodeError {
+    fn from(err: Error) -> Self {
+        Bech32DecodeError::Bech32Error(err)
+    }
+}
+
+impl fmt::Display for Bech32DecodeError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match &self {
+            Bech32DecodeError::Bech32Error(e) => write!(f, "{}", e),
+            Bech32DecodeError::IncorrectVariant(variant) => write!(
+                f,
+                "Incorrect bech32 encoding (wrong variant: {:?})",
+                variant
+            ),
+            Bech32DecodeError::ReadError => {
+                write!(f, "Failed to decode key from its binary representation.")
+            }
+            Bech32DecodeError::HrpMismatch { expected, actual } => write!(
+                f,
+                "Key was encoded for a different network: expected {}, got {}.",
+                expected, actual
+            ),
+        }
+    }
+}
+
+fn bech32_decode<T, F>(hrp: &str, s: &str, read: F) -> Result<T, Bech32DecodeError>
 where
     F: Fn(Vec<u8>) -> Option<T>,
 {
-    match bech32::decode(s)? {
+    let (decoded_hrp, data, variant) = bech32::decode(s)?;
-        (decoded_hrp, data, Variant::Bech32) if decoded_hrp == hrp => {
+    if variant != Variant::Bech32 {
-            Vec::<u8>::from_base32(&data).map(read)
+        Err(Bech32DecodeError::IncorrectVariant(variant))
-        }
+    } else if decoded_hrp != hrp {
-        _ => Ok(None),
+        Err(Bech32DecodeError::HrpMismatch {
+            expected: hrp.to_string(),
+            actual: decoded_hrp,
+        })
+    } else {
+        read(Vec::<u8>::from_base32(&data)?).ok_or(Bech32DecodeError::ReadError)
     }
 }
 
@@ -121,7 +161,7 @@ pub fn encode_extended_spending_key(hrp: &str, extsk: &ExtendedSpendingKey) -> S
 pub fn decode_extended_spending_key(
     hrp: &str,
     s: &str,
-) -> Result<Option<ExtendedSpendingKey>, Error> {
+) -> Result<ExtendedSpendingKey, Bech32DecodeError> {
     bech32_decode(hrp, s, |data| ExtendedSpendingKey::read(&data[..]).ok())
 }
 
@@ -155,7 +195,7 @@ pub fn encode_extended_full_viewing_key(hrp: &str, extfvk: &ExtendedFullViewingK
 pub fn decode_extended_full_viewing_key(
     hrp: &str,
     s: &str,
-) -> Result<Option<ExtendedFullViewingKey>, Error> {
+) -> Result<ExtendedFullViewingKey, Bech32DecodeError> {
     bech32_decode(hrp, s, |data| ExtendedFullViewingKey::read(&data[..]).ok())
 }
 
@@ -240,11 +280,11 @@ pub fn encode_payment_address_p<P: consensus::Parameters>(
 ///         HRP_SAPLING_PAYMENT_ADDRESS,
 ///         "ztestsapling1qqqqqqqqqqqqqqqqqqcguyvaw2vjk4sdyeg0lc970u659lvhqq7t0np6hlup5lusxle75ss7jnk",
 ///     ),
-///     Ok(Some(pa)),
+///     Ok(pa),
 /// );
 /// ```
 /// [`PaymentAddress`]: zcash_primitives::sapling::PaymentAddress
-pub fn decode_payment_address(hrp: &str, s: &str) -> Result<Option<PaymentAddress>, Error> {
+pub fn decode_payment_address(hrp: &str, s: &str) -> Result<PaymentAddress, Bech32DecodeError> {
     bech32_decode(hrp, s, |data| {
         if data.len() != 43 {
             return None;
@@ -392,6 +432,7 @@ mod tests {
     use super::{
         decode_extended_full_viewing_key, decode_extended_spending_key, decode_payment_address,
         encode_extended_full_viewing_key, encode_extended_spending_key, encode_payment_address,
+        Bech32DecodeError,
     };
 
     #[test]
@@ -414,7 +455,7 @@ mod tests {
                 encoded_main
             )
             .unwrap(),
-            Some(extsk.clone())
+            extsk
         );
 
         assert_eq!(
@@ -430,7 +471,7 @@ mod tests {
                 encoded_test
             )
             .unwrap(),
-            Some(extsk)
+            extsk
         );
     }
 
@@ -454,7 +495,7 @@ mod tests {
                 encoded_main
             )
             .unwrap(),
-            Some(extfvk.clone())
+            extfvk
         );
 
         assert_eq!(
@@ -470,7 +511,7 @@ mod tests {
                 encoded_test
             )
             .unwrap(),
-            Some(extfvk)
+            extfvk
         );
     }
 
@@ -500,7 +541,7 @@ mod tests {
                 encoded_main
             )
             .unwrap(),
-            Some(addr.clone())
+            addr
         );
 
         assert_eq!(
@@ -513,7 +554,7 @@ mod tests {
                 encoded_test
             )
             .unwrap(),
-            Some(addr)
+            addr
         );
     }
 
@@ -535,9 +576,8 @@ mod tests {
             decode_payment_address(
                 constants::mainnet::HRP_SAPLING_PAYMENT_ADDRESS,
                 &encoded_main
-            )
+            ),
-            .unwrap(),
+            Err(Bech32DecodeError::ReadError)
-            None
         );
     }
 }

zcash_client_backend/src/zip321.rs

@@ -793,7 +793,7 @@ mod tests {
         let expected = TransactionRequest {
             payments: vec![
                 Payment {
-                    recipient_address: RecipientAddress::Shielded(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap().unwrap()),
+                    recipient_address: RecipientAddress::Shielded(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()),
                     amount: Amount::from_u64(376876902796286).unwrap(),
                     memo: None,
                     label: None,
@@ -811,7 +811,7 @@ mod tests {
         let req = TransactionRequest {
             payments: vec![
                 Payment {
-                    recipient_address: RecipientAddress::Shielded(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap().unwrap()),
+                    recipient_address: RecipientAddress::Shielded(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()),
                     amount: Amount::from_u64(0).unwrap(),
                     memo: None,
                     label: None,

zcash_client_sqlite/src/error.rs

@@ -3,7 +3,10 @@
 use std::error;
 use std::fmt;
 
-use zcash_client_backend::{data_api, encoding::TransparentCodecError};
+use zcash_client_backend::{
+    data_api,
+    encoding::{Bech32DecodeError, TransparentCodecError},
+};
 use zcash_primitives::consensus::BlockHeight;
 
 use crate::{NoteId, PRUNING_HEIGHT};
@@ -27,8 +30,8 @@ pub enum SqliteClientError {
     /// Illegal attempt to reinitialize an already-initialized wallet database.
     TableNotEmpty,
 
-    /// Bech32 decoding error
+    /// A Bech32-encoded key or address decoding error
-    Bech32(bech32::Error),
+    Bech32DecodeError(Bech32DecodeError),
 
     /// Base58 decoding error
     Base58(bs58::decode::Error),
@@ -58,7 +61,7 @@ impl error::Error for SqliteClientError {
     fn source(&self) -> Option<&(dyn error::Error + 'static)> {
         match &self {
             SqliteClientError::InvalidMemo(e) => Some(e),
-            SqliteClientError::Bech32(e) => Some(e),
+            SqliteClientError::Bech32DecodeError(Bech32DecodeError::Bech32Error(e)) => Some(e),
             SqliteClientError::DbError(e) => Some(e),
             SqliteClientError::Io(e) => Some(e),
             _ => None,
@@ -78,7 +81,7 @@ impl fmt::Display for SqliteClientError {
                 write!(f, "The note ID associated with an inserted witness must correspond to a received note."),
             SqliteClientError::RequestedRewindInvalid(h, r) =>
                 write!(f, "A rewind must be either of less than {} blocks, or at least back to block {} for your wallet; the requested height was {}.", PRUNING_HEIGHT, h, r),
-            SqliteClientError::Bech32(e) => write!(f, "{}", e),
+            SqliteClientError::Bech32DecodeError(e) => write!(f, "{}", e),
             SqliteClientError::Base58(e) => write!(f, "{}", e),
             SqliteClientError::TransparentAddress(e) => write!(f, "{}", e),
             SqliteClientError::TableNotEmpty => write!(f, "Table is not empty"),
@@ -102,9 +105,9 @@ impl From<std::io::Error> for SqliteClientError {
     }
 }
 
-impl From<bech32::Error> for SqliteClientError {
+impl From<Bech32DecodeError> for SqliteClientError {
-    fn from(e: bech32::Error) -> Self {
+    fn from(e: Bech32DecodeError) -> Self {
-        SqliteClientError::Bech32(e)
+        SqliteClientError::Bech32DecodeError(e)
     }
 }
 

zcash_client_sqlite/src/wallet/init.rs

@@ -197,13 +197,7 @@ impl<P: consensus::Parameters> RusqliteMigration for WalletMigration2<P> {
         for row in rows {
             let (account, address) = row?;
             let decoded_address =
-                decode_payment_address(self.params.hrp_sapling_payment_address(), &address)?
+                decode_payment_address(self.params.hrp_sapling_payment_address(), &address)?;
-                    .ok_or_else(|| {
-                        SqliteClientError::CorruptedData(format!(
-                            "Not a valid Sapling payment address: {}",
-                            address
-                        ))
-                    })?;
 
             let usk =
                 UnifiedSpendingKey::from_seed(&self.params, self.seed.expose_secret(), account)