Skip to content

Commit

Permalink
Address further comments from code review.
Browse files Browse the repository at this point in the history
  • Loading branch information
nuttycom committed Mar 1, 2025
1 parent 1199e60 commit d69e1a8
Show file tree
Hide file tree
Showing 10 changed files with 42 additions and 29 deletions.
2 changes: 1 addition & 1 deletion zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ and this library adheres to Rust's notion of
- Has added method `utxo_query_height` when the `transparent-inputs` feature
flag is active.
- has removed method `get_current_address`. It has been replaced by
added method `WalletWrite::get_last_generated_address`
added method `WalletWrite::get_last_generated_address_matching`
- `zcash_client_backend::data_api::WalletWrite`:
- has added method `get_address_for_index`
- `get_next_available_address` now returns the diversifier index at which the
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_backend/src/data_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,7 @@ pub trait WalletRead {
///
/// This will return `Ok(None)` if no previously generated address conforms to the specified
/// request.
fn get_last_generated_address(
fn get_last_generated_address_matching(
&self,
account: Self::AccountId,
address_filter: UnifiedAddressRequest,
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_backend/src/data_api/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2523,7 +2523,7 @@ impl WalletRead for MockWalletDb {
Ok(None)
}

fn get_last_generated_address(
fn get_last_generated_address_matching(

Check warning on line 2526 in zcash_client_backend/src/data_api/testing.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api/testing.rs#L2526

Added line #L2526 was not covered by tests
&self,
_account: Self::AccountId,
_request: UnifiedAddressRequest,
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_backend/src/data_api/testing/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1768,7 +1768,7 @@ where

let uaddr = st
.wallet()
.get_last_generated_address(account.id(), UnifiedAddressRequest::AllAvailableKeys)
.get_last_generated_address_matching(account.id(), UnifiedAddressRequest::AllAvailableKeys)
.unwrap()
.unwrap();
let taddr = uaddr.transparent().unwrap();
Expand Down
6 changes: 3 additions & 3 deletions zcash_client_backend/src/data_api/testing/transparent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ where
let account_id = st.test_account().unwrap().id();
let uaddr = st
.wallet()
.get_last_generated_address(account_id, UnifiedAddressRequest::AllAvailableKeys)
.get_last_generated_address_matching(account_id, UnifiedAddressRequest::AllAvailableKeys)
.unwrap()
.unwrap();
let taddr = uaddr.transparent().unwrap();
Expand Down Expand Up @@ -193,7 +193,7 @@ where
let account = st.test_account().cloned().unwrap();
let uaddr = st
.wallet()
.get_last_generated_address(account.id(), UnifiedAddressRequest::AllAvailableKeys)
.get_last_generated_address_matching(account.id(), UnifiedAddressRequest::AllAvailableKeys)
.unwrap()
.unwrap();
let taddr = uaddr.transparent().unwrap();
Expand Down Expand Up @@ -301,7 +301,7 @@ where
let account = st.test_account().cloned().unwrap();
let uaddr = st
.wallet()
.get_last_generated_address(account.id(), UnifiedAddressRequest::AllAvailableKeys)
.get_last_generated_address_matching(account.id(), UnifiedAddressRequest::AllAvailableKeys)
.unwrap()
.unwrap();
let taddr = uaddr.transparent().unwrap();
Expand Down
25 changes: 18 additions & 7 deletions zcash_client_sqlite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -679,13 +679,18 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters, CL> WalletRead
wallet::get_account_for_ufvk(self.conn.borrow(), &self.params, ufvk)
}

fn get_last_generated_address(
fn get_last_generated_address_matching(
&self,
account: Self::AccountId,
request: UnifiedAddressRequest,
) -> Result<Option<UnifiedAddress>, Self::Error> {
wallet::get_last_generated_address(self.conn.borrow(), &self.params, account, request)
.map(|res| res.map(|(addr, _)| addr))
wallet::get_last_generated_address_matching(
self.conn.borrow(),
&self.params,
account,
request,
)
.map(|res| res.map(|(addr, _)| addr))
}

fn get_account_birthday(&self, account: Self::AccountId) -> Result<BlockHeight, Self::Error> {
Expand Down Expand Up @@ -2282,7 +2287,10 @@ mod tests {

let current_addr = st
.wallet()
.get_last_generated_address(account.id(), UnifiedAddressRequest::AllAvailableKeys)
.get_last_generated_address_matching(
account.id(),
UnifiedAddressRequest::AllAvailableKeys,
)
.unwrap();
assert!(current_addr.is_some());

Expand All @@ -2296,7 +2304,10 @@ mod tests {

let addr2_cur = st
.wallet()
.get_last_generated_address(account.id(), UnifiedAddressRequest::AllAvailableKeys)
.get_last_generated_address_matching(
account.id(),
UnifiedAddressRequest::AllAvailableKeys,
)
.unwrap();
assert_eq!(addr2, addr2_cur);

Expand All @@ -2310,7 +2321,7 @@ mod tests {

let cur_shielded_only = st
.wallet()
.get_last_generated_address(account.id(), shielded_only_request)
.get_last_generated_address_matching(account.id(), shielded_only_request)
.unwrap();
assert!(cur_shielded_only.is_none());

Expand All @@ -2336,7 +2347,7 @@ mod tests {

let cur_shielded_only = st
.wallet()
.get_last_generated_address(account.id(), shielded_only_request)
.get_last_generated_address_matching(account.id(), shielded_only_request)
.unwrap()
.expect("retrieved the last-generated shielded-only address");
assert_eq!(cur_shielded_only, shielded_only);
Expand Down
10 changes: 6 additions & 4 deletions zcash_client_sqlite/src/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ pub(crate) fn get_next_available_address<P: consensus::Parameters, C: Clock>(
Ok(Some((addr, diversifier_index)))
}

pub(crate) fn get_last_generated_address<P: consensus::Parameters>(
pub(crate) fn get_last_generated_address_matching<P: consensus::Parameters>(
conn: &rusqlite::Connection,
params: &P,
account_uuid: AccountUuid,
Expand Down Expand Up @@ -4145,14 +4145,16 @@ mod tests {

// The default address is set for the test account
assert_matches!(
st.wallet()
.get_last_generated_address(account.id(), UnifiedAddressRequest::AllAvailableKeys),
st.wallet().get_last_generated_address_matching(
account.id(),
UnifiedAddressRequest::AllAvailableKeys
),
Ok(Some(_))
);

// No default address is set for an un-initialized account
assert_matches!(
st.wallet().get_last_generated_address(
st.wallet().get_last_generated_address_matching(
AccountUuid(Uuid::nil()),
UnifiedAddressRequest::AllAvailableKeys
),
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_sqlite/src/wallet/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1135,7 +1135,7 @@ mod tests {
// hardcoded with knowledge of test vectors
let ua_request = UnifiedAddressRequest::unsafe_custom(Omit, Require, Require);

let (ua, di) = wallet::get_last_generated_address(
let (ua, di) = wallet::get_last_generated_address_matching(
&db_data.conn,
&db_data.params,
account_id,
Expand Down
15 changes: 9 additions & 6 deletions zcash_keys/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,25 @@ and this library adheres to Rust's notion of
## [Unreleased]

### Added
- `zcash_keys::keys::UnifiedAddressRequest::{sapling, orchard, p2pkh}`
- `zcash_keys::keys::UnifiedIncomingViewingKey::{has_sapling, has_orchard, has_p2pkh,
receiver_requirements, to_receiver_requirements}`
- `zcash_keys::keys::UnifiedIncomingViewingKey::{has_sapling, has_orchard,
has_transparent, receiver_requirements, to_receiver_requirements}`
- `zcash_keys::keys::ReceiverRequirements`

### Changed
- `zcash_keys::keys::UnifiedAddressRequest` is now an enum instead of a struct.
The `new` and `unsafe_new` methods have been replaced by `custom` and
`unsafe_custom` respectively.
- Arguments to `zcash_keys::keys::UnifiedIncomingViewingKey::address` have been
modified; the `request` argument to this method now has type
`UnifiedAddressRequest` instead of `Option<UnifiedAddressRequest>`. Use
`UnifiedAddressRequest::AllAvailableKeys` where `None` was previously
used to obtain the same semantics.

### Removed
- `UnifiedAddressRequest::{new, unsafe_new}`: use `{custom, unsafe_custom}`
respectively instead.
- `UnifiedAddressRequest::{intersect, orchard, sapling, p2pkh}`: replacements
for these methods are now provided on the newly-added `ReceiverRequirements`
type.
- `UnifiedAddressRequest::intersect`: a replacement for this method is now
provided with the newly-added `ReceiverRequirements` type.

## [0.7.0] - 2025-02-21

Expand Down
5 changes: 1 addition & 4 deletions zcash_keys/src/keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,10 +600,7 @@ pub enum UnifiedAddressRequest {

impl UnifiedAddressRequest {
/// Constructs a new unified address request that allows a receiver of each type.
pub const ALLOW_ALL: Self = {
use ReceiverRequirement::*;
Self::unsafe_custom(Allow, Allow, Allow)
};
pub const ALLOW_ALL: Self = Self::Custom(ReceiverRequirements::ALLOW_ALL);

pub fn custom(

Check warning on line 605 in zcash_keys/src/keys.rs

View check run for this annotation

Codecov / codecov/patch

zcash_keys/src/keys.rs#L605

Added line #L605 was not covered by tests
orchard: ReceiverRequirement,
Expand Down

0 comments on commit d69e1a8

Please sign in to comment.