Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zcash_client_sqlite: Align handling of transparent UTXOs with that of shielded notes. #1402

Merged
merged 4 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ and this library adheres to Rust's notion of
### Added
- `zcash_client_backend::data_api`:
- `chain::BlockCache` trait, behind the `sync` feature flag.
- `WalletRead::get_spendable_transparent_outputs`.
- `zcash_client_backend::fees`:
- `ChangeValue::{transparent, shielded}`
- `sapling::EmptyBundleView`
- `orchard::EmptyBundleView`
- `zcash_client_backend::scanning`:
- `testing` module
- `zcash_client_backend::sync` module, behind the `sync` feature flag.
- `zcash_client_backend::sync` module, behind the `sync` feature flag

### Changed
- MSRV is now 1.70.0.
Expand Down Expand Up @@ -52,6 +53,12 @@ and this library adheres to Rust's notion of
`zcash_address::ZcashAddress`. This simplifies the process of tracking the
original address to which value was sent.

### Removed
- `zcash_client_backend::data_api`:
- `WalletRead::get_unspent_transparent_outputs` has been removed because its
semantics were unclear and could not be clarified. Use
`WalletRead::get_spendable_transparent_outputs` instead.

## [0.12.1] - 2024-03-27

### Fixed
Expand Down
18 changes: 11 additions & 7 deletions zcash_client_backend/src/data_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,10 +665,10 @@
exclude: &[Self::NoteRef],
) -> Result<SpendableNotes<Self::NoteRef>, Self::Error>;

/// Fetches a spendable transparent output.
/// Fetches the transparent output corresponding to the provided `outpoint`.
///
/// Returns `Ok(None)` if the UTXO is not known to belong to the wallet or is not
/// spendable.
/// spendable as of the chain tip height.
#[cfg(feature = "transparent-inputs")]
fn get_unspent_transparent_output(
&self,
Expand All @@ -677,14 +677,18 @@
Ok(None)
}

/// Returns a list of unspent transparent UTXOs that appear in the chain at heights up to and
/// including `max_height`.
/// Returns the list of transparent outputs received at `address` such that:
/// * The transaction that produced these outputs is mined or mineable as of `max_height`.
/// * Each returned output is unspent as of the current chain tip.
///
/// The caller should filter these outputs to ensure they respect the desired number of
/// confirmations before attempting to spend them.
#[cfg(feature = "transparent-inputs")]
fn get_unspent_transparent_outputs(
fn get_spendable_transparent_outputs(

Check warning on line 687 in zcash_client_backend/src/data_api.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api.rs#L687

Added line #L687 was not covered by tests
&self,
_address: &TransparentAddress,
_max_height: BlockHeight,
_exclude: &[OutPoint],
_target_height: BlockHeight,
_min_confirmations: u32,
) -> Result<Vec<WalletTransparentOutput>, Self::Error> {
Ok(vec![])
}
Expand Down
6 changes: 1 addition & 5 deletions zcash_client_backend/src/data_api/wallet/input_selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -580,11 +580,7 @@ where
let mut transparent_inputs: Vec<WalletTransparentOutput> = source_addrs
.iter()
.map(|taddr| {
wallet_db.get_unspent_transparent_outputs(
taddr,
target_height - min_confirmations,
&[],
)
wallet_db.get_spendable_transparent_outputs(taddr, target_height, min_confirmations)
})
.collect::<Result<Vec<Vec<_>>, _>>()
.map_err(InputSelectorError::DataSource)?
Expand Down
1 change: 1 addition & 0 deletions zcash_client_sqlite/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ and this library adheres to Rust's notion of
- `zcash_client_sqlite::error::SqliteClientError` has changed variants:
- Removed `HdwalletError`.
- Added `TransparentDerivation`.
- The `block` column of the `v_transactions` view has been renamed to `mined_height`.

## [0.10.3] - 2024-04-08

Expand Down
41 changes: 26 additions & 15 deletions zcash_client_sqlite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,22 +272,22 @@
&self,
outpoint: &OutPoint,
) -> Result<Option<WalletTransparentOutput>, Self::Error> {
wallet::get_unspent_transparent_output(self.conn.borrow(), outpoint)
wallet::transparent::get_unspent_transparent_output(self.conn.borrow(), outpoint)
}

#[cfg(feature = "transparent-inputs")]
fn get_unspent_transparent_outputs(
fn get_spendable_transparent_outputs(
&self,
address: &TransparentAddress,
max_height: BlockHeight,
exclude: &[OutPoint],
target_height: BlockHeight,
min_confirmations: u32,
) -> Result<Vec<WalletTransparentOutput>, Self::Error> {
wallet::get_unspent_transparent_outputs(
wallet::transparent::get_spendable_transparent_outputs(
self.conn.borrow(),
&self.params,
address,
max_height,
exclude,
target_height,
min_confirmations,
)
}
}
Expand Down Expand Up @@ -430,9 +430,7 @@
}

fn chain_height(&self) -> Result<Option<BlockHeight>, Self::Error> {
wallet::scan_queue_extrema(self.conn.borrow())
.map(|h| h.map(|range| *range.end()))
.map_err(SqliteClientError::from)
wallet::chain_tip_height(self.conn.borrow()).map_err(SqliteClientError::from)
}

fn get_block_hash(&self, block_height: BlockHeight) -> Result<Option<BlockHash>, Self::Error> {
Expand Down Expand Up @@ -516,7 +514,7 @@
&self,
account: AccountId,
) -> Result<HashMap<TransparentAddress, Option<TransparentAddressMetadata>>, Self::Error> {
wallet::get_transparent_receivers(self.conn.borrow(), &self.params, account)
wallet::transparent::get_transparent_receivers(self.conn.borrow(), &self.params, account)
}

#[cfg(feature = "transparent-inputs")]
Expand All @@ -525,7 +523,12 @@
account: AccountId,
max_height: BlockHeight,
) -> Result<HashMap<TransparentAddress, NonNegativeAmount>, Self::Error> {
wallet::get_transparent_balances(self.conn.borrow(), &self.params, account, max_height)
wallet::transparent::get_transparent_address_balances(
Copy link
Contributor

@daira daira Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this renaming the function in wallet::transparent so that it doesn't match the trait method? I think that for all of the other functions there that correspond to trait methods, the names do match.

(Currently I'm renaming it back in #1257, because I hadn't previously noticed that the change was made here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When moving the methods, I got confused about the meaning of get_transparent_balances vs get_transparent_account_balance and I wanted to clarify. I didn't want to change the top-level trait method, but since this is crate-private I thought that it was a good start.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to undo renaming it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about it.

self.conn.borrow(),
&self.params,
account,
max_height,
)
}
}

Expand Down Expand Up @@ -1034,7 +1037,11 @@
_output: &WalletTransparentOutput,
) -> Result<Self::UtxoRef, Self::Error> {
#[cfg(feature = "transparent-inputs")]
return wallet::put_received_transparent_utxo(&self.conn, &self.params, _output);
return wallet::transparent::put_received_transparent_utxo(
&self.conn,
&self.params,
_output,
);

#[cfg(not(feature = "transparent-inputs"))]
panic!(
Expand Down Expand Up @@ -1228,7 +1235,7 @@
.iter()
.flat_map(|b| b.vin.iter())
{
wallet::mark_transparent_utxo_spent(wdb.conn.0, tx_ref, &txin.prevout)?;
wallet::transparent::mark_transparent_utxo_spent(wdb.conn.0, tx_ref, &txin.prevout)?;

Check warning on line 1238 in zcash_client_sqlite/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/lib.rs#L1238

Added line #L1238 was not covered by tests
}

// If we have some transparent outputs:
Expand Down Expand Up @@ -1337,7 +1344,11 @@

#[cfg(feature = "transparent-inputs")]
for utxo_outpoint in sent_tx.utxos_spent() {
wallet::mark_transparent_utxo_spent(wdb.conn.0, tx_ref, utxo_outpoint)?;
wallet::transparent::mark_transparent_utxo_spent(
wdb.conn.0,
tx_ref,
utxo_outpoint,
)?;
}

for output in sent_tx.outputs() {
Expand Down
Loading
Loading