Apply suggestions from code review

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

zcash_client_backend/CHANGELOG.md

@@ -86,8 +86,12 @@ and this library adheres to Rust's notion of
   - `ReceivedNote`
   - `Recipient::{map_internal_account, internal_account_transpose_option}`
   - `WalletOutput`
-  - `WalletSaplingOutput::key_source`
+  - `WalletSaplingOutput::{key_source, account_id, recipient_key_scope}`
+  - `WalletSaplingSpend::account_id`
   - `WalletSpend`
+  - `WalletTx::new`
+  - `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`:
@@ -101,8 +105,6 @@ and this library adheres to Rust's notion of
   - `impl TryFrom<&CompactSaplingOutput> for CompactOutputDescription`
 - `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::{
@@ -157,9 +159,6 @@ 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.
-  - `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
     of the `WalletTx` change.
   - `ScannedBlock::metadata` has been renamed to `to_block_metadata` and now
@@ -296,9 +295,16 @@ and this library adheres to Rust's notion of
   - `SentTransactionOutput::recipient` now returns a `Recipient<Note>`.
   - `OvkPolicy::Custom` is now a structured variant that can contain independent
     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`.
+  - `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.
+- `zcash_client_backend::scanning`:
+  - Arguments to `scan_blocks` have changed.
+  - `ScanError` has new variants `TreeSizeInvalid` and `EncodingInvalid`.
+  - `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.
@@ -339,8 +345,12 @@ 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.
+  - `WalletSaplingOutput::cmu` (use `WalletSaplingOutput::note` and
+    `sapling_crypto::Note::cmu` instead).
+  - `WalletSaplingOutput::account` (use `WalletSaplingOutput::account_id` instead)
+  - `WalletSaplingSpend::account` (use `WalletSaplingSpend::account_id` 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

@@ -201,6 +201,11 @@ 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(|_| ())?,
@@ -208,16 +213,21 @@ impl compact_formats::CompactOrchardAction {
         .ok_or(())
     }
 
+    /// Returns the nullifier for the spend of this action.
+    ///
+    /// A convenience method that parses [`CompactOrchardAction.nullifier`].
+    ///
+    /// [`CompactOrchardAction.nullifier`]: #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

@@ -827,10 +827,10 @@ where
         };
 
         // Collect the set of accounts that were spent from in this transaction
-        let spent_from_accounts = sapling_spends.iter().map(|spend| spend.account());
+        let spent_from_accounts = sapling_spends.iter().map(|spend| spend.account_id());
         #[cfg(feature = "orchard")]
         let spent_from_accounts =
-            spent_from_accounts.chain(orchard_spends.iter().map(|spend| spend.account()));
+            spent_from_accounts.chain(orchard_spends.iter().map(|spend| spend.account_id()));
         let spent_from_accounts = spent_from_accounts.copied().collect::<HashSet<_>>();
 
         let (sapling_outputs, mut sapling_nc) = find_received(
@@ -965,8 +965,8 @@ where
     ))
 }
 
-// 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<
     AccountId: ConditionallySelectable + Default,
     Spend,
@@ -1479,7 +1479,7 @@ mod tests {
         assert_eq!(tx.sapling_outputs().len(), 0);
         assert_eq!(tx.sapling_spends()[0].index(), 0);
         assert_eq!(tx.sapling_spends()[0].nf(), &nf);
-        assert_eq!(tx.sapling_spends()[0].account(), &account);
+        assert_eq!(tx.sapling_spends()[0].account_id(), &account);
 
         assert_eq!(
             scanned_block

zcash_client_backend/src/wallet.rs

@@ -109,7 +109,7 @@ pub struct WalletTx<AccountId> {
 }
 
 impl<AccountId> WalletTx<AccountId> {
-    /// Constructs a new [`WalletTx`] from its constituent parts
+    /// Constructs a new [`WalletTx`] from its constituent parts.
     pub fn new(
         txid: TxId,
         block_index: usize,
@@ -132,7 +132,7 @@ impl<AccountId> WalletTx<AccountId> {
         }
     }
 
-    /// Returns the [`TxId`] for the corresponding [`Transaction`]
+    /// Returns the [`TxId`] for the corresponding [`Transaction`].
     ///
     /// [`Transaction`]: zcash_primitives::transaction::Transaction
     pub fn txid(&self) -> TxId {
@@ -150,7 +150,7 @@ impl<AccountId> WalletTx<AccountId> {
         self.sapling_spends.as_ref()
     }
 
-    /// Returns a record for each Sapling note belonging to and/or produced by the wallet in the
+    /// Returns a record for each Sapling note received or produced by the wallet in the
     /// transaction.
     pub fn sapling_outputs(&self) -> &[WalletSaplingOutput<AccountId>] {
         self.sapling_outputs.as_ref()
@@ -163,7 +163,7 @@ impl<AccountId> WalletTx<AccountId> {
         self.orchard_spends.as_ref()
     }
 
-    /// Returns a record for each Orchard note belonging to and/or produced by the wallet in the
+    /// Returns a record for each Orchard note received or produced by the wallet in the
     /// transaction.
     #[cfg(feature = "orchard")]
     pub fn orchard_outputs(&self) -> &[WalletOrchardOutput<AccountId>] {
@@ -229,13 +229,17 @@ impl transparent_fees::InputView for WalletTransparentOutput {
 pub struct WalletSpend<Nf, AccountId> {
     index: usize,
     nf: Nf,
-    account: AccountId,
+    account_id: AccountId,
 }
 
 impl<Nf, AccountId> WalletSpend<Nf, AccountId> {
     /// Constructs a `WalletSpend` from its constituent parts.
-    pub fn from_parts(index: usize, nf: Nf, account: AccountId) -> Self {
-        Self { index, nf, account }
+    pub fn from_parts(index: usize, nf: Nf, account_id: AccountId) -> Self {
+        Self {
+            index,
+            nf,
+            account_id,
+        }
     }
 
     /// Returns the index of the Sapling spend or Orchard action within the transaction that
@@ -247,14 +251,16 @@ impl<Nf, AccountId> WalletSpend<Nf, AccountId> {
     pub fn nf(&self) -> &Nf {
         &self.nf
     }
-    /// Returns the identifier to the account to which the note belonged.
-    pub fn account(&self) -> &AccountId {
-        &self.account
+    /// Returns the identifier to the account_id to which the note belonged.
+    pub fn account_id(&self) -> &AccountId {
+        &self.account_id
     }
 }
 
+/// A type alias for Sapling [`WalletSpend`]s.
 pub type WalletSaplingSpend<AccountId> = WalletSpend<sapling::Nullifier, AccountId>;
 
+/// A type alias for Orchard [`WalletSpend`]s.
 #[cfg(feature = "orchard")]
 pub type WalletOrchardSpend<AccountId> = WalletSpend<orchard::note::Nullifier, AccountId>;
 

zcash_client_sqlite/src/wallet/sapling.rs

@@ -438,14 +438,16 @@ pub(crate) fn put_received_note<T: ReceivedSaplingOutput>(
     let to = output.note().recipient();
     let diversifier = to.diversifier();
 
-    let account = output.account_id();
+    // FIXME: recipient key scope will always be available until IVK import is supported.
+    // Remove this expectation after #1175 merges.
     let scope = output
         .recipient_key_scope()
         .expect("Key import is not yet supported.");
+
     let sql_args = named_params![
         ":tx": &tx_ref,
         ":output_index": i64::try_from(output.index()).expect("output indices are representable as i64"),
-        ":account": u32::from(account),
+        ":account": u32::from(output.account_id()),
         ":diversifier": &diversifier.0.as_ref(),
         ":value": output.note().value().inner(),
         ":rcm": &rcm.as_ref(),