Apply suggestions from code review

Co-authored-by: str4d <[email protected]>

zcash_client_backend/CHANGELOG.md

@@ -74,8 +74,11 @@ and this library adheres to Rust's notion of
   - `WalletOutput`
   - `WalletSaplingOutput::key_source`
   - `WalletOrchardOutput` behind the `orchard` feature flag.
-  - `WalletSpend` 
+  - `WalletSpend`
+  - `WalletTx::new`
   - `WalletTx::{orchard_spends, orchard_outputs}` behind the `orchard` feature flag.
+  - `WalletTx` getter methods `{txid, block_index, sapling_spends, sapling_outputs}`
+    (replacing what were previously public fields.)
   - `TransparentAddressMetadata` (which replaces `zcash_keys::address::AddressMetadata`).
   - `impl {Debug, Clone} for OvkPolicy`
 - `zcash_client_backend::proposal`:
@@ -91,8 +94,6 @@ and this library adheres to Rust's notion of
   - `CompactOrchardAction::{cmx, nf, ephemeral_key}`
 - `zcash_client_backend::scanning`:
   - `ScanningKeyOps` has replaced the `ScanningKey` trait.
-  - `ScanningKey` is now a concrete type that bundles an incoming viewing key
-    with an optional nullifier key and key source metadata.
   - `ScanningKeys`
   - `Nullifiers`
 - `impl Clone for zcash_client_backend::{
@@ -113,7 +114,8 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::data_api`:
   - `BlockMetadata::sapling_tree_size` now returns an `Option<u32>` instead of
     a `u32` for future consistency with Orchard.
-  - `WalletShieldedOutput::from_parts` now takes an additional key source metadata.
+  - `WalletSaplingOutput::from_parts` arguments have changed.
+  - `WalletSaplingOutput::nf` now returns an `Option<sapling::Nullifier>`
   - `WalletTx` is no longer parameterized by the nullifier type; instead, the
     nullifier is present as an optional value.
   - `ScannedBlock` is no longer parameterized by the nullifier type as a consequence
@@ -250,7 +252,11 @@ and this library adheres to Rust's notion of
     Sapling and Orchard `OutgoingViewingKey`s.
   - `WalletTx::new` now takes additional Orchard spend and output arguments
     when the `orchard` feature is enabled.
-- `zcash_client_backend::scanning::ScanError` has a new variant, `TreeSizeInvalid`.
+- `zcash_client_backend::scanning`:
+  - `ScanError` has a new variant, `TreeSizeInvalid`.
+  - `ScanningKey` is now a concrete type that bundles an incoming viewing key
+    with an optional nullifier key and key source metadata. The trait that
+    provides uniform access to scanning key information is now `ScanningKeyOps`.
 - `zcash_client_backend::zip321`:
   - `TransactionRequest::payments` now returns a `BTreeMap<usize, Payment>`
     instead of `&[Payment]` so that parameter indices may be preserved.
@@ -291,8 +297,13 @@ and this library adheres to Rust's notion of
     `zcash_client_backend::proposal`).
   - `SentTransactionOutput::sapling_change_to` - the note created by an internal
     transfer is now conveyed in the `recipient` field.
-  - `WalletSaplingSpend` use the generic `WalletSpend` instead.
-  - `WalletSaplingOutput::cmu` obtain `cmu` from the `Note` instead.
+  - `WalletSaplingSpend` (use the generic `WalletSpend` instead).
+  - `WalletSaplingOutput::cmu` (use `WalletSaplingOutput::note` and
+    `sapling_crypto::Note::cmu` instead).
+  - `WalletSaplingOutput::cmu` (use `WalletSaplingOutput::note` and
+    `sapling_crypto::Note::cmu` instead).
+  - `WalletTx` fields `{txid, index, sapling_spends, sapling_outputs}` (use
+    the new getters instead.)
 - `zcash_client_backend::data_api`:
   - `{PoolType, ShieldedProtocol}` (moved to `zcash_client_backend`).
   - `{NoteId, Recipient}` (moved to `zcash_client_backend::wallet`).

zcash_client_backend/src/proto.rs

@@ -200,23 +200,33 @@ impl TryFrom<&compact_formats::CompactOrchardAction> for orchard::note_encryptio
 
 #[cfg(feature = "orchard")]
 impl compact_formats::CompactOrchardAction {
+    /// Returns the note commitment for the output of this action.
+    ///
+    /// A convenience method that parses [`CompactOrchardAction.cmx`].
+    ///
+    /// [`CompactOrchardAction.cmx`]: #structfield.cmx
     pub fn cmx(&self) -> Result<orchard::note::ExtractedNoteCommitment, ()> {
         Option::from(orchard::note::ExtractedNoteCommitment::from_bytes(
             &self.cmx[..].try_into().map_err(|_| ())?,
         ))
         .ok_or(())
     }
 
+    /// Returns the nullifier for the spend of this action.
+    ///
+    /// A convenience method that parses [`CompactOrchardAction.nullifier`].
+    ///
+    /// [`CompactOrchardAction.cmx`]: #structfield.nullifier
     pub fn nf(&self) -> Result<orchard::note::Nullifier, ()> {
         let nf_bytes: [u8; 32] = self.nullifier[..].try_into().map_err(|_| ())?;
         Option::from(orchard::note::Nullifier::from_bytes(&nf_bytes)).ok_or(())
     }
 
-    /// Returns the ephemeral public key for this output.
+    /// Returns the ephemeral public key for the output of this action.
     ///
-    /// A convenience method that parses [`CompactOutput.epk`].
+    /// A convenience method that parses [`CompactOrchardAction.ephemeral_key`].
     ///
-    /// [`CompactOutput.epk`]: #structfield.epk
+    /// [`CompactOrchardAction.ephemeral_key`]: #structfield.ephemeral_key
     pub fn ephemeral_key(&self) -> Result<EphemeralKeyBytes, ()> {
         self.ephemeral_key[..]
             .try_into()

zcash_client_backend/src/scanning.rs

@@ -913,8 +913,8 @@ pub(crate) fn scan_block_with_runners<
     ))
 }
 
-// Check for spent notes. The comparison against known-unspent nullifiers is done
-// in constant time.
+/// Check for spent notes. The comparison against known-unspent nullifiers is done
+/// in constant time.
 fn find_spent<Spend, Nf: ConstantTimeEq + Copy, WS>(
     spends: &[Spend],
     nullifiers: &[(AccountId, Nf)],

zcash_client_backend/src/wallet.rs

@@ -109,7 +109,7 @@ pub struct WalletTx {
 }
 
 impl WalletTx {
-    /// Constructs a new [`WalletTx`] from its constituent parts
+    /// Constructs a new [`WalletTx`] from its constituent parts.
     pub fn new(
         txid: TxId,
         block_index: usize,
@@ -130,7 +130,7 @@ impl WalletTx {
         }
     }
 
-    /// Returns the [`TxId`] for the corresponding [`Transaction`]
+    /// Returns the [`TxId`] for the corresponding [`Transaction`].
     ///
     /// [`Transaction`]: zcash_primitives::transaction::Transaction
     pub fn txid(&self) -> TxId {
@@ -251,7 +251,7 @@ impl<Nf> WalletSpend<Nf> {
     }
 }
 
-/// Returns source metadata for a given incoming viewing key.
+/// Source metadata for a given incoming viewing key.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum KeySource {
     /// The ZIP 32 account identifier and key scope for the key, if the key was derived from the