Apply suggestions from code review

Co-authored-by: str4d <thestr4d@gmail.com>

zcash_client_backend/src/data_api/error.rs

@@ -24,6 +24,9 @@ pub enum ChainInvalid {
 
 #[derive(Debug)]
 pub enum Error<NoteId> {
+    /// No account with the given identifier was found in the wallet.
+    AccountNotFound(AccountId),
+
     /// The amount specified exceeds the allowed range.
     InvalidAmount,
 
@@ -35,12 +38,6 @@ pub enum Error<NoteId> {
     /// Chain validation detected an error in the block at the specified block height.
     InvalidChain(BlockHeight, ChainInvalid),
 
-    /// A provided extsk is not associated with the specified account.
-    AccountNotFound(AccountId),
-
-    /// No key of the given type was associated with the specified account.
-    KeyNotFound(AccountId, Typecode),
-
     /// A provided extsk is not associated with the specified account.
     InvalidExtSk(AccountId),
 
@@ -55,6 +52,9 @@ pub enum Error<NoteId> {
     /// does not correspond to root of the current commitment tree.
     InvalidWitnessAnchor(NoteId, BlockHeight),
 
+    /// No key of the given type was associated with the specified account.
+    KeyNotFound(AccountId, Typecode),
+
     /// The wallet must first perform a scan of the blockchain before other
     /// operations can be performed.
     ScanRequired,
@@ -86,6 +86,9 @@ impl ChainInvalid {
 impl<N: fmt::Display> fmt::Display for Error<N> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match &self {
+            Error::AccountNotFound(account) => {
+                write!(f, "Wallet does not contain account {}", u32::from(*account))
+            }
             Error::InvalidAmount => write!(
                 f,
                 "The value lies outside the valid range of Zcash amounts."
@@ -98,12 +101,6 @@ impl<N: fmt::Display> fmt::Display for Error<N> {
             Error::InvalidChain(upper_bound, cause) => {
                 write!(f, "Invalid chain (upper bound: {}): {:?}", u32::from(*upper_bound), cause)
             }
-            Error::AccountNotFound(account) => {
-                write!(f, "Unrecognized account {}", u32::from(*account))
-            }
-            Error::KeyNotFound(account, typecode) => {
-                write!(f, "No {:?} key was available for account {}", typecode, u32::from(*account))
-            }
             Error::InvalidExtSk(account) => {
                 write!(f, "Incorrect ExtendedSpendingKey for account {}", u32::from(*account))
             }
@@ -117,6 +114,9 @@ impl<N: fmt::Display> fmt::Display for Error<N> {
                 "Witness for note {} has incorrect anchor after scanning block {}",
                 id_note, last_height
             ),
+            Error::KeyNotFound(account, typecode) => {
+                write!(f, "No {:?} key was available for account {}", typecode, u32::from(*account))
+            }
             Error::ScanRequired => write!(f, "Must scan blocks first"),
             Error::Builder(e) => write!(f, "{:?}", e),
             Error::Protobuf(e) => write!(f, "{}", e),

zcash_client_backend/src/data_api/wallet.rs

@@ -423,8 +423,8 @@ where
 ///   UTXOs.
 /// * `account`: The ZIP32 account identifier for the account to which funds will
 ///   be shielded. Funds will be shielded to the internal (change) address associated with the
-///   Sapling key corresponding to this account, or if a Sapling key is not available for this
-///   account this function will return an error.
+///   most preferred shielded receiver corresponding to this account, or if no shielded
+///   receiver can be used for this account, this function will return an error.
 /// * `memo`: A memo to be included in the output to the (internal) recipient.
 ///   This can be used to take notes about auto-shielding operations internal
 ///   to the wallet that the wallet can use to improve how it represents those
@@ -458,6 +458,8 @@ where
                 .get(&account)
                 .ok_or_else(|| E::from(Error::AccountNotFound(account)))
                 .and_then(|ufvk| {
+                    // TODO: select the most preferred shielded receiver once we have the ability to
+                    // spend Orchard funds.
                     ufvk.sapling()
                         .map(|dfvk| dfvk.change_address().1)
                         .ok_or_else(|| E::from(Error::KeyNotFound(account, Typecode::Sapling)))

zcash_primitives/CHANGELOG.md

@@ -110,7 +110,9 @@ and this library adheres to Rust's notion of
   not child viewing keys.
 - `zcash_primitives::sapling::keys::DiversifiableFullViewingKey::change_address`
   has been added as a convenience method for obtaining the change address
-  at the default diversifier.
+  at the default diversifier. This address **MUST NOT** be encoded and exposed
+  to users. User interfaces should instead mark these notes as "change notes" or
+  "internal wallet operations".
 - A new module `zcash_primitives::legacy::keys` has been added under the
   `transparent-inputs` feature flag to support types related to supporting
   transparent components of unified addresses and derivation of OVKs for

zcash_primitives/src/sapling/keys.rs

@@ -294,6 +294,9 @@ impl DiversifiableFullViewingKey {
 
     /// Returns the internal address corresponding to the smallest valid diversifier index,
     /// along with that index.
+    ///
+    /// This address **MUST NOT** be encoded and exposed to end users. User interfaces
+    /// should instead mark these notes as "change notes" or "internal wallet operations".
     pub fn change_address(&self) -> (zip32::DiversifierIndex, PaymentAddress) {
         let internal_dfvk = self.derive_internal();
         zip32::sapling_default_address(&internal_dfvk.fvk, &internal_dfvk.dk)