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

Add UnifiedIncomingViewingKey struct #1245

Merged
merged 11 commits into from
Mar 18, 2024
54 changes: 12 additions & 42 deletions zcash_client_sqlite/src/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@
use std::num::NonZeroU32;
use std::ops::RangeInclusive;
use tracing::debug;
use zcash_address::unified::{Encoding, Ivk, Uivk};
use zcash_keys::keys::{AddressGenerationError, UnifiedAddressRequest};
use zcash_keys::keys::{AddressGenerationError, UnifiedAddressRequest, UnifiedIncomingViewingKey};

use zcash_client_backend::{
address::{Address, UnifiedAddress},
Expand Down Expand Up @@ -119,6 +118,7 @@
crate::UtxoId,
rusqlite::Row,
std::collections::BTreeSet,
zcash_address::unified::{Encoding, Ivk, Uivk},
zcash_client_backend::wallet::{TransparentAddressMetadata, WalletTransparentOutput},
zcash_primitives::{
legacy::{
Expand Down Expand Up @@ -183,7 +183,7 @@
///
/// Accounts that have this kind of viewing key cannot be used in wallet contexts,
/// because they are unable to maintain an accurate balance.
Incoming(Uivk),
Incoming(Box<UnifiedIncomingViewingKey>),
}

/// An account stored in a `zcash_client_sqlite` database.
Expand All @@ -206,7 +206,7 @@
) -> Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError> {
Copy link
Contributor

@daira daira Mar 15, 2024

Choose a reason for hiding this comment

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

This method can be public. (It will need a changelog entry.)

Copy link
Contributor

Choose a reason for hiding this comment

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

In general we would like to avoid encouraging users to use the default_address method. In any case, this is not a change that should be made in this PR.

match &self.viewing_key {
ViewingKey::Full(ufvk) => ufvk.default_address(request),
ViewingKey::Incoming(_uivk) => todo!(),
ViewingKey::Incoming(uivk) => uivk.default_address(request),

Check warning on line 209 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L209

Added line #L209 was not covered by tests
}
}
}
Expand All @@ -233,10 +233,10 @@
}
}
AArnott marked this conversation as resolved.
Show resolved Hide resolved

fn uivk_str(&self, params: &impl Parameters) -> Result<String, SqliteClientError> {
fn uivk(&self) -> UnifiedIncomingViewingKey {
match self {
ViewingKey::Full(ufvk) => ufvk_to_uivk(ufvk, params),
ViewingKey::Incoming(uivk) => Ok(uivk.encode(&params.network_type())),
ViewingKey::Full(ufvk) => ufvk.as_ref().to_unified_incoming_viewing_key(),
ViewingKey::Incoming(uivk) => uivk.as_ref().clone(),

Check warning on line 239 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L239

Added line #L239 was not covered by tests
}
}
}
Expand Down Expand Up @@ -296,31 +296,6 @@
)
}

pub(crate) fn ufvk_to_uivk<P: consensus::Parameters>(
ufvk: &UnifiedFullViewingKey,
params: &P,
) -> Result<String, SqliteClientError> {
let mut ivks: Vec<Ivk> = Vec::new();
if let Some(orchard) = ufvk.orchard() {
ivks.push(Ivk::Orchard(orchard.to_ivk(Scope::External).to_bytes()));
}
if let Some(sapling) = ufvk.sapling() {
let ivk = sapling.to_external_ivk();
ivks.push(Ivk::Sapling(ivk.to_bytes()));
}
#[cfg(feature = "transparent-inputs")]
if let Some(tfvk) = ufvk.transparent() {
let tivk = tfvk.derive_external_ivk()?;
ivks.push(Ivk::P2pkh(tivk.serialize().try_into().map_err(|_| {
SqliteClientError::BadAccountData("Unable to serialize transparent IVK.".to_string())
})?));
}

let uivk = zcash_address::unified::Uivk::try_from_items(ivks)
.map_err(|e| SqliteClientError::BadAccountData(format!("Unable to derive UIVK: {}", e)))?;
Ok(uivk.encode(&params.network_type()))
}

pub(crate) fn add_account<P: consensus::Parameters>(
conn: &rusqlite::Transaction,
params: &P,
Expand Down Expand Up @@ -370,7 +345,7 @@
":hd_seed_fingerprint": hd_seed_fingerprint.as_ref().map(|fp| fp.to_bytes()),
":hd_account_index": hd_account_index.map(u32::from),
":ufvk": viewing_key.ufvk().map(|ufvk| ufvk.encode(params)),
":uivk": viewing_key.uivk_str(params)?,
":uivk": viewing_key.uivk().encode(params),
":orchard_fvk_item_cache": orchard_item,
":sapling_fvk_item_cache": sapling_item,
":p2pkh_fvk_item_cache": transparent_item,
Expand Down Expand Up @@ -1525,15 +1500,10 @@
))
} else {
let uivk_str: String = row.get("uivk")?;
let (network, uivk) = Uivk::decode(&uivk_str).map_err(|e| {
SqliteClientError::CorruptedData(format!("Failure to decode UIVK: {e}"))
})?;
if network != params.network_type() {
return Err(SqliteClientError::CorruptedData(
"UIVK network type does not match wallet network type".to_string(),
));
}
ViewingKey::Incoming(uivk)
ViewingKey::Incoming(Box::new(
UnifiedIncomingViewingKey::decode(params, &uivk_str[..])
.map_err(SqliteClientError::BadAccountData)?,

Check warning on line 1505 in zcash_client_sqlite/src/wallet.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet.rs#L1503-L1505

Added lines #L1503 - L1505 were not covered by tests
))
};

Ok(Some(Account {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{collections::HashSet, rc::Rc};

use crate::wallet::{account_kind_code, init::WalletMigrationError, ufvk_to_uivk};
use crate::wallet::{account_kind_code, init::WalletMigrationError};
use rusqlite::{named_params, Transaction};
use schemer_rusqlite::RusqliteMigration;
use secrecy::{ExposeSecret, SecretVec};
Expand Down Expand Up @@ -121,8 +121,9 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
));
}

let uivk = ufvk_to_uivk(&ufvk_parsed, &self.params)
.map_err(|e| WalletMigrationError::CorruptedData(e.to_string()))?;
let uivk = ufvk_parsed
.to_unified_incoming_viewing_key()
.encode(&self.params);

#[cfg(feature = "transparent-inputs")]
let transparent_item = ufvk_parsed.transparent().map(|k| k.serialize());
Expand Down
18 changes: 14 additions & 4 deletions zcash_keys/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,23 @@ and this library adheres to Rust's notion of
- `zcash_keys::address::Address::has_receiver`
- `impl Display for zcash_keys::keys::AddressGenerationError`
- `impl std::error::Error for zcash_keys::keys::AddressGenerationError`
- `zcash_keys::keys::DecodingError`
- `zcash_keys::keys::UnifiedFullViewingKey::to_unified_incoming_viewing_key`
- `zcash_keys::keys::UnifiedIncomingViewingKey`

### Changed
- `zcash_keys::keys::AddressGenerationError` has a new variant
`DiversifierSpaceExhausted`.
- `zcash_keys::keys::UnifiedFullViewingKey::{find_address, default_address}`
- `zcash_keys::keys::UnifiedFullViewingKey::{find_address, default_address}`
now return `Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError>`
(instead of `Option<(UnifiedAddress, DiversifierIndex)>` for `find_address`).
- `zcash_keys::keys::AddressGenerationError`
- Added `DiversifierSpaceExhausted` variant.
- At least one of the `orchard`, `sapling`, or `transparent-inputs` features
must be enabled for the `keys` module to be accessible.

### Removed
- `UnifiedFullViewingKey::new` has been placed behind the `test-dependencies`
feature flag. UFVKs should only be produced by derivation from the USK, or
parsed from their string representation.

### Fixed
- `UnifiedFullViewingKey::find_address` can now find an address for a diversifier
Expand All @@ -29,7 +39,7 @@ and this library adheres to Rust's notion of
- `zcash_keys::keys::UnifiedAddressRequest::all`

### Fixed
- A missing application of the `sapling` feature flag was remedied;
- A missing application of the `sapling` feature flag was remedied;
prior to this fix it was not possible to use this crate without the
`sapling` feature enabled.

Expand Down
9 changes: 8 additions & 1 deletion zcash_keys/src/address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,14 @@ impl Address {
}
}

#[cfg(any(test, feature = "test-dependencies"))]
#[cfg(all(
any(
feature = "orchard",
feature = "sapling",
feature = "transparent-inputs"
),
any(test, feature = "test-dependencies")
))]
nuttycom marked this conversation as resolved.
Show resolved Hide resolved
pub mod testing {
use proptest::prelude::*;
use zcash_primitives::consensus::Network;
Expand Down
Loading
Loading