From 4863d5ae1e2468e89ba9f86db946bf6b60f1e2d6 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 28 Feb 2024 10:37:23 -0700 Subject: [PATCH] Apply suggestions from code review Co-authored-by: str4d --- zcash_client_backend/CHANGELOG.md | 34 +++++++++++++++-------- zcash_client_backend/src/proto.rs | 16 +++++++++-- zcash_client_backend/src/scanning.rs | 10 +++---- zcash_client_backend/src/wallet.rs | 22 +++++++++------ zcash_client_sqlite/src/wallet/sapling.rs | 6 ++-- 5 files changed, 58 insertions(+), 30 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 691fe1a1b..4d3b0051c 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -75,10 +75,15 @@ and this library adheres to Rust's notion of - `ReceivedNote` - `Recipient::{map_internal_account, internal_account_transpose_option}` - `WalletOutput` - - `WalletSaplingOutput::key_source` + - `WalletSaplingOutput::account_id` - `WalletOrchardOutput` behind the `orchard` feature flag. - - `WalletSpend` + - `WalletSpend` + - `WalletSaplingSpend::account_id` + - `WalletOrchardSpend` behind the `orchard` feature flag. + - `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`: @@ -94,8 +99,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::{ @@ -144,9 +147,6 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api`: - `BlockMetadata::sapling_tree_size` now returns an `Option` 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 @@ -281,9 +281,15 @@ and this library adheres to Rust's notion of - `SentTransactionOutput::recipient` now returns a `Recipient`. - `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` + - `WalletTx` is no longer parameterized by the nullifier type; instead, the + nullifier is present as an optional value. +- `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` instead of `&[Payment]` so that parameter indices may be preserved. @@ -324,8 +330,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`). diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index 1ed156916..0e12c981a 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -200,6 +200,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 { Option::from(orchard::note::ExtractedNoteCommitment::from_bytes( &self.cmx[..].try_into().map_err(|_| ())?, @@ -207,16 +212,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 { 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 { self.ephemeral_key[..] .try_into() diff --git a/zcash_client_backend/src/scanning.rs b/zcash_client_backend/src/scanning.rs index 321e0de79..24fe2dfbe 100644 --- a/zcash_client_backend/src/scanning.rs +++ b/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.cloned().collect::>(); 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 diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index 8744ad886..bdf11068c 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/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, @@ -132,7 +132,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 { @@ -229,13 +229,17 @@ impl transparent_fees::InputView for WalletTransparentOutput { pub struct WalletSpend { index: usize, nf: Nf, - account: AccountId, + account_id: AccountId, } impl WalletSpend { /// 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 WalletSpend { 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 = WalletSpend; +/// A type alias for Orchard [`WalletSpend`]s. #[cfg(feature = "orchard")] pub type WalletOrchardSpend = WalletSpend; diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 19fb484d4..dc38b7ecc 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -438,14 +438,16 @@ pub(crate) fn put_received_note( 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(),