zcash_client_backend: Restrict use of backend-specific note identifiers.

In general, it is preferable to use globally relevant identifiers where
possible. This PR removes the `WalletRead::TxRef` associated type in
favor of using `TxId` directly for the transaction identifier, and
restricts the use of the `NoteRef` type to those scenarios where the
result of one query is intended to be used directly as the input to
another query.

Closes #834

zcash_client_backend/CHANGELOG.md

@@ -12,6 +12,7 @@ and this library adheres to Rust's notion of
 - `impl Debug` for `zcash_client_backend::{data_api::wallet::input_selection::Proposal, wallet::ReceivedSaplingNote}`
 - `zcash_client_backend::data_api`:
   - `BlockMetadata`
+  - `NoteId`
   - `NullifierQuery` for use with `WalletRead::get_sapling_nullifiers`
   - `ScannedBlock`
   - `ShieldedProtocol`
@@ -33,9 +34,14 @@ and this library adheres to Rust's notion of
 - Bumped dependencies to `hdwallet 0.4`, `zcash_primitives 0.12`, `zcash_note_encryption 0.4`,
   `incrementalmerkletree 0.4`, `orchard 0.5`, `bs58 0.5`
 - `zcash_client_backend::data_api`:
-  - `WalletRead::get_memo` now returns `Result<Option<Memo>, Self::Error>`
-    instead of `Result<Memo, Self::Error>` in order to make representable
-    wallet states where the full note plaintext is not available.
+  - `WalletRead::TxRef` has been removed in favor of consistently using `TxId` instead. 
+  - `WalletRead::get_transaction` now takes a `TxId` as its argument.
+  - `WalletWrite::{store_decrypted_tx, store_sent_tx}` now return `Result<(), Self::Error>` 
+    as the `WalletRead::TxRef` associated type has been removed. Use `TxId` instead.
+  - `WalletRead::get_memo` now takes a `NoteId` as its argument instead of `Self::NoteRef`
+    and returns `Result<Option<Memo>, Self::Error>` instead of `Result<Memo,
+    Self::Error>` in order to make representable wallet states where the full
+    note plaintext is not available.
   - `WalletRead::get_nullifiers` has been renamed to `WalletRead::get_sapling_nullifiers`
     and its signature has changed; it now subsumes the removed `WalletRead::get_all_nullifiers`.
   - `WalletRead::get_target_and_anchor_heights` now takes its argument as a `NonZeroU32`
@@ -45,15 +51,15 @@ and this library adheres to Rust's notion of
   - `chain::BlockSource::with_blocks` now takes its limit as an `Option<usize>`
     instead of `Option<u32>`.
   - A new `CommitmentTree` variant has been added to `data_api::error::Error`
-  - `data_api::wallet::{create_spend_to_address, create_proposed_transaction,
+  - `wallet::{create_spend_to_address, create_proposed_transaction,
     shield_transparent_funds}` all now require that `WalletCommitmentTrees` be
     implemented for the type passed to them for the `wallet_db` parameter.
-  - `data_api::wallet::create_proposed_transaction` now takes an additional 
+  - `wallet::create_proposed_transaction` now takes an additional 
     `min_confirmations` argument.
-  - `data_api::wallet::{spend, create_spend_to_address, shield_transparent_funds, 
+  - `wallet::{spend, create_spend_to_address, shield_transparent_funds, 
     propose_transfer, propose_shielding, create_proposed_transaction}` now take their
     respective `min_confirmations` arguments as `NonZeroU32`
-  - `data_api::wallet::input_selection::InputSelector::{propose_transaction, propose_shielding}`
+  - `wallet::input_selection::InputSelector::{propose_transaction, propose_shielding}`
     now take their respective `min_confirmations` arguments as `NonZeroU32`
   - A new `Scan` variant has been added to `data_api::chain::error::Error`.
   - A new `SyncRequired` variant has been added to `data_api::wallet::input_selection::InputSelectorError`.
@@ -64,6 +70,9 @@ and this library adheres to Rust's notion of
   - Arguments to `WalletSaplingOutput::from_parts` have changed.
 - `zcash_client_backend::data_api::wallet::input_selection::InputSelector`:
   - Arguments to `{propose_transaction, propose_shielding}` have changed. 
+- `zcash_client_backend::data_api::wallet::{create_spend_to_address, spend,
+  create_proposed_transaction, shield_transparent_funds}` now return the `TxId`
+  for the newly created transaction instead an internal database identifier.
 - `zcash_client_backend::wallet::ReceivedSaplingNote::note_commitment_tree_position`
   has replaced the `witness` field in the same struct.
 - `zcash_client_backend::welding_rig` has been renamed to `zcash_client_backend::scanning`

zcash_client_backend/src/data_api.rs

@@ -38,6 +38,8 @@ pub mod wallet;
 
 pub const SAPLING_SHARD_HEIGHT: u8 = sapling::NOTE_COMMITMENT_TREE_DEPTH / 2;
 
+/// An enumeration of constraints that can be applied when querying for nullifiers for notes
+/// belonging to the wallet.
 pub enum NullifierQuery {
     Unspent,
     All,
@@ -58,13 +60,6 @@ pub trait WalletRead {
     /// or a UUID.
     type NoteRef: Copy + Debug + Eq + Ord;
 
-    /// Backend-specific transaction identifier.
-    ///
-    /// For example, this might be a database identifier type
-    /// or a TxId if the backend is able to support that type
-    /// directly.
-    type TxRef: Copy + Debug + Eq + Ord;
-
     /// Returns the minimum and maximum block heights for stored blocks.
     ///
     /// This will return `Ok(None)` if no block data is present in the database.
@@ -189,14 +184,13 @@ pub trait WalletRead {
 
     /// Returns the memo for a note.
     ///
-    /// Implementations of this method must return an error if the note identifier
-    /// does not appear in the backing data store. Returns `Ok(None)` if the note
-    /// is known to the wallet but memo data has not yet been populated for that
-    /// note.
-    fn get_memo(&self, id_note: Self::NoteRef) -> Result<Option<Memo>, Self::Error>;
+    /// Returns `Ok(None)` if the note is known to the wallet but memo data has not yet been
+    /// populated for that note, or if the note identifier does not correspond to a note
+    /// that is known to the wallet.
+    fn get_memo(&self, note_id: NoteId) -> Result<Option<Memo>, Self::Error>;
 
     /// Returns a transaction.
-    fn get_transaction(&self, id_tx: Self::TxRef) -> Result<Transaction, Self::Error>;
+    fn get_transaction(&self, txid: TxId) -> Result<Transaction, Self::Error>;
 
     /// Returns the nullifiers for notes that the wallet is tracking, along with their associated
     /// account IDs, that are either unspent or have not yet been confirmed as spent (in that a
@@ -206,7 +200,7 @@ pub trait WalletRead {
         query: NullifierQuery,
     ) -> Result<Vec<(AccountId, sapling::Nullifier)>, Self::Error>;
 
-    /// Return all unspent Sapling notes.
+    /// Return all unspent Sapling notes, excluding the specified note IDs.
     fn get_spendable_sapling_notes(
         &self,
         account: AccountId,
@@ -380,13 +374,43 @@ pub struct SentTransaction<'a> {
 }
 
 /// A shielded transfer protocol supported by the wallet.
-#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub enum ShieldedProtocol {
     /// The Sapling protocol
     Sapling,
     // TODO: Orchard
 }
 
+/// A unique identifier for a shielded transaction output
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
+pub struct NoteId {
+    txid: TxId,
+    protocol: ShieldedProtocol,
+    output_index: u32,
+}
+
+impl NoteId {
+    pub fn new(txid: TxId, protocol: ShieldedProtocol, output_index: u32) -> Self {
+        Self {
+            txid,
+            protocol,
+            output_index,
+        }
+    }
+
+    pub fn txid(&self) -> &TxId {
+        &self.txid
+    }
+
+    pub fn protocol(&self) -> ShieldedProtocol {
+        self.protocol
+    }
+
+    pub fn output_index(&self) -> u32 {
+        self.output_index
+    }
+}
+
 /// A value pool to which the wallet supports sending transaction outputs.
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum PoolType {
@@ -506,7 +530,7 @@ pub trait WalletWrite: WalletRead {
     fn put_blocks(
         &mut self,
         blocks: Vec<ScannedBlock<sapling::Nullifier>>,
-    ) -> Result<Vec<Self::NoteRef>, Self::Error>;
+    ) -> Result<(), Self::Error>;
 
     /// Updates the wallet's view of the blockchain.
     ///
@@ -518,14 +542,11 @@ pub trait WalletWrite: WalletRead {
     fn update_chain_tip(&mut self, tip_height: BlockHeight) -> Result<(), Self::Error>;
 
     /// Caches a decrypted transaction in the persistent wallet store.
-    fn store_decrypted_tx(
-        &mut self,
-        received_tx: DecryptedTransaction,
-    ) -> Result<Self::TxRef, Self::Error>;
+    fn store_decrypted_tx(&mut self, received_tx: DecryptedTransaction) -> Result<(), Self::Error>;
 
     /// Saves information about a transaction that was constructed and sent by the wallet to the
     /// persistent wallet store.
-    fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result<Self::TxRef, Self::Error>;
+    fn store_sent_tx(&mut self, sent_tx: &SentTransaction) -> Result<(), Self::Error>;
 
     /// Truncates the wallet database to the specified height.
     ///
@@ -608,7 +629,7 @@ pub mod testing {
 
     use super::{
         chain::CommitmentTreeRoot, scanning::ScanRange, BlockMetadata, DecryptedTransaction,
-        NullifierQuery, ScannedBlock, SentTransaction, WalletCommitmentTrees, WalletRead,
+        NoteId, NullifierQuery, ScannedBlock, SentTransaction, WalletCommitmentTrees, WalletRead,
         WalletWrite, SAPLING_SHARD_HEIGHT,
     };
 
@@ -633,7 +654,6 @@ pub mod testing {
     impl WalletRead for MockWalletDb {
         type Error = ();
         type NoteRef = u32;
-        type TxRef = TxId;
 
         fn block_height_extrema(&self) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error> {
             Ok(None)
@@ -705,11 +725,11 @@ pub mod testing {
             Ok(Amount::zero())
         }
 
-        fn get_memo(&self, _id_note: Self::NoteRef) -> Result<Option<Memo>, Self::Error> {
+        fn get_memo(&self, _id_note: NoteId) -> Result<Option<Memo>, Self::Error> {
             Ok(None)
         }
 
-        fn get_transaction(&self, _id_tx: Self::TxRef) -> Result<Transaction, Self::Error> {
+        fn get_transaction(&self, _txid: TxId) -> Result<Transaction, Self::Error> {
             Err(())
         }
 
@@ -788,8 +808,8 @@ pub mod testing {
         fn put_blocks(
             &mut self,
             _blocks: Vec<ScannedBlock<sapling::Nullifier>>,
-        ) -> Result<Vec<Self::NoteRef>, Self::Error> {
-            Ok(vec![])
+        ) -> Result<(), Self::Error> {
+            Ok(())
         }
 
         fn update_chain_tip(&mut self, _tip_height: BlockHeight) -> Result<(), Self::Error> {
@@ -799,15 +819,12 @@ pub mod testing {
         fn store_decrypted_tx(
             &mut self,
             _received_tx: DecryptedTransaction,
-        ) -> Result<Self::TxRef, Self::Error> {
-            Ok(TxId::from_bytes([0u8; 32]))
+        ) -> Result<(), Self::Error> {
+            Ok(())
         }
 
-        fn store_sent_tx(
-            &mut self,
-            _sent_tx: &SentTransaction,
-        ) -> Result<Self::TxRef, Self::Error> {
-            Ok(TxId::from_bytes([0u8; 32]))
+        fn store_sent_tx(&mut self, _sent_tx: &SentTransaction) -> Result<(), Self::Error> {
+            Ok(())
         }
 
         fn truncate_to_height(&mut self, _block_height: BlockHeight) -> Result<(), Self::Error> {

zcash_client_backend/src/data_api/wallet.rs

@@ -1,7 +1,7 @@
-use std::fmt::Debug;
 use std::{convert::Infallible, num::NonZeroU32};
 
 use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree};
+use zcash_primitives::transaction::TxId;
 use zcash_primitives::{
     consensus::{self, BlockHeight, NetworkUpgrade},
     memo::MemoBytes,
@@ -203,7 +203,7 @@ pub fn create_spend_to_address<DbT, ParamsT>(
     ovk_policy: OvkPolicy,
     min_confirmations: NonZeroU32,
 ) -> Result<
-    DbT::TxRef,
+    TxId,
     Error<
         <DbT as WalletRead>::Error,
         <DbT as WalletCommitmentTrees>::Error,
@@ -306,7 +306,7 @@ pub fn spend<DbT, ParamsT, InputsT>(
     ovk_policy: OvkPolicy,
     min_confirmations: NonZeroU32,
 ) -> Result<
-    DbT::TxRef,
+    TxId,
     Error<
         <DbT as WalletRead>::Error,
         <DbT as WalletCommitmentTrees>::Error,
@@ -317,7 +317,6 @@ pub fn spend<DbT, ParamsT, InputsT>(
 >
 where
     DbT: WalletWrite + WalletCommitmentTrees,
-    DbT::TxRef: Copy + Debug,
     DbT::NoteRef: Copy + Eq + Ord,
     ParamsT: consensus::Parameters + Clone,
     InputsT: InputSelector<DataSource = DbT>,
@@ -441,7 +440,7 @@ pub fn create_proposed_transaction<DbT, ParamsT, InputsErrT, FeeRuleT>(
     min_confirmations: NonZeroU32,
     change_memo: Option<MemoBytes>,
 ) -> Result<
-    DbT::TxRef,
+    TxId,
     Error<
         <DbT as WalletRead>::Error,
         <DbT as WalletCommitmentTrees>::Error,
@@ -452,7 +451,6 @@ pub fn create_proposed_transaction<DbT, ParamsT, InputsErrT, FeeRuleT>(
 >
 where
     DbT: WalletWrite + WalletCommitmentTrees,
-    DbT::TxRef: Copy + Debug,
     DbT::NoteRef: Copy + Eq + Ord,
     ParamsT: consensus::Parameters + Clone,
     FeeRuleT: FeeRule,
@@ -667,7 +665,9 @@ where
             #[cfg(feature = "transparent-inputs")]
             utxos_spent: utxos.iter().map(|utxo| utxo.outpoint().clone()).collect(),
         })
-        .map_err(Error::DataSource)
+        .map_err(Error::DataSource)?;
+
+    Ok(tx.txid())
 }
 
 /// Constructs a transaction that consumes available transparent UTXOs belonging to
@@ -717,7 +717,7 @@ pub fn shield_transparent_funds<DbT, ParamsT, InputsT>(
     memo: &MemoBytes,
     min_confirmations: NonZeroU32,
 ) -> Result<
-    DbT::TxRef,
+    TxId,
     Error<
         <DbT as WalletRead>::Error,
         <DbT as WalletCommitmentTrees>::Error,

zcash_client_sqlite/CHANGELOG.md

@@ -12,6 +12,7 @@ and this library adheres to Rust's notion of
 - A new default-enabled feature flag `multicore`. This allows users to disable
   multicore support by setting `default_features = false` on their
   `zcash_primitives`, `zcash_proofs`, and `zcash_client_sqlite` dependencies.
+- `zcash_client_sqlite::ReceivedNoteId`
 
 ### Changed
 - MSRV is now 1.65.0.
@@ -28,6 +29,9 @@ and this library adheres to Rust's notion of
 
 ### Removed
 - The empty `wallet::transact` module has been removed.
+- `zcash_client_sqlite::NoteId` has been replaced with `zcash_client_sqlite::ReceivedNoteId`
+  as the `SentNoteId` variant of is now unused following changes to 
+  `zcash_client_backend::data_api::WalletRead`.
 
 ### Fixed
 - Fixed an off-by-one error in the `BlockSource` implementation for the SQLite-backed