Skip to content

Commit

Permalink
zcash_keys: Box Address elements to avoid large variations in enum …
Browse files Browse the repository at this point in the history
…variant sizes
  • Loading branch information
nuttycom committed Feb 15, 2024
1 parent 02e2dba commit e78a5ec
Show file tree
Hide file tree
Showing 16 changed files with 55 additions and 54 deletions.
2 changes: 2 additions & 0 deletions zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,8 @@ and this library adheres to Rust's notion of
- The fields of `ReceivedSaplingNote` are now private. Use
`ReceivedSaplingNote::from_parts` for construction instead. Accessor methods
are provided for each previously public field.
- The address variants of `Recipient` now `Box` their contents to avoid large
discrepancies in enum variant sizing.
- `zcash_client_backend::scanning::ScanError` has a new variant, `TreeSizeInvalid`.
- `zcash_client_backend::zip321::TransactionRequest::payments` now returns a
`BTreeMap<usize, Payment>` instead of `&[Payment]` so that parameter
Expand Down
12 changes: 8 additions & 4 deletions zcash_client_backend/src/data_api/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ where
.expect("Payment step references are checked at construction")
.recipient_address
{
Address::Transparent(t) => Some(t),
Address::Transparent(t) => Some(t.as_ref()),
Address::Unified(uaddr) => uaddr.transparent(),
_ => None,
}
Expand Down Expand Up @@ -849,8 +849,12 @@ where
.memo
.as_ref()
.map_or_else(MemoBytes::empty, |m| m.clone());
builder.add_sapling_output(external_ovk, *addr, payment.amount, memo.clone())?;
sapling_output_meta.push((Recipient::Sapling(*addr), payment.amount, Some(memo)));
builder.add_sapling_output(external_ovk, **addr, payment.amount, memo.clone())?;
sapling_output_meta.push((
Recipient::Sapling(addr.clone()),
payment.amount,
Some(memo),
));
}
Address::Transparent(to) => {
if payment.memo.is_some() {
Expand Down Expand Up @@ -950,7 +954,7 @@ where

SentTransactionOutput::from_parts(
output_index,
Recipient::Transparent(*addr),
Recipient::Transparent(addr.clone()),
value,
None,
None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ where
}

return Err(InputSelectorError::Selection(
GreedyInputSelectorError::UnsupportedAddress(Box::new(addr.clone())),
GreedyInputSelectorError::UnsupportedAddress(addr.clone()),

Check warning on line 384 in zcash_client_backend/src/data_api/wallet/input_selection.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api/wallet/input_selection.rs#L384

Added line #L384 was not covered by tests
));
}
}
Expand Down
6 changes: 3 additions & 3 deletions zcash_client_backend/src/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ impl NoteId {
/// output.
#[derive(Debug, Clone)]
pub enum Recipient {
Transparent(TransparentAddress),
Sapling(sapling::PaymentAddress),
Unified(UnifiedAddress, PoolType),
Transparent(Box<TransparentAddress>),
Sapling(Box<sapling::PaymentAddress>),
Unified(Box<UnifiedAddress>, PoolType),
InternalAccount(AccountId, PoolType),
}

Expand Down
6 changes: 3 additions & 3 deletions zcash_client_backend/src/zip321.rs
Original file line number Diff line number Diff line change
Expand Up @@ -891,7 +891,7 @@ mod tests {
let expected = TransactionRequest::new(
vec![
Payment {
recipient_address: Address::Sapling(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()),
recipient_address: Address::from(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()),
amount: NonNegativeAmount::const_from_u64(376876902796286),
memo: None,
label: None,
Expand All @@ -912,7 +912,7 @@ mod tests {
let expected = TransactionRequest::new(
vec![
Payment {
recipient_address: Address::Sapling(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()),
recipient_address: Address::from(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()),
amount: NonNegativeAmount::ZERO,
memo: None,
label: None,
Expand All @@ -930,7 +930,7 @@ mod tests {
let req = TransactionRequest::new(
vec![
Payment {
recipient_address: Address::Sapling(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()),
recipient_address: Address::from(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()),
amount: NonNegativeAmount::ZERO,
memo: None,
label: None,
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_sqlite/src/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ mod tests {

// We can spend the received notes
let req = TransactionRequest::new(vec![Payment {
recipient_address: Address::Sapling(dfvk.default_address().1),
recipient_address: Address::from(dfvk.default_address().1),
amount: NonNegativeAmount::const_from_u64(110_000),
memo: None,
label: None,
Expand Down
4 changes: 2 additions & 2 deletions zcash_client_sqlite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
match output.transfer_type {
TransferType::Outgoing | TransferType::WalletInternal => {
let recipient = if output.transfer_type == TransferType::Outgoing {
Recipient::Sapling(output.note.recipient())
Recipient::Sapling(Box::new(output.note.recipient()))

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

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/lib.rs#L592

Added line #L592 was not covered by tests
} else {
Recipient::InternalAccount(
output.account,
Expand Down Expand Up @@ -658,7 +658,7 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
*account_id,
tx_ref,
output_index,
&Recipient::Transparent(address),
&Recipient::Transparent(Box::new(address)),

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

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/lib.rs#L661

Added line #L661 was not covered by tests
txout.value,
None
)?;
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_sqlite/src/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ pub(crate) fn get_current_address<P: consensus::Parameters>(
SqliteClientError::CorruptedData("Not a valid Zcash recipient address".to_owned())
})
.and_then(|addr| match addr {
Address::Unified(ua) => Ok(ua),
Address::Unified(ua) => Ok(*ua),
_ => Err(SqliteClientError::CorruptedData(format!(
"Addresses table contains {} which is not a unified address",
addr_str,
Expand Down
4 changes: 2 additions & 2 deletions zcash_client_sqlite/src/wallet/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,7 @@ mod tests {

let ufvk_str = ufvk.encode(&wdb.params);
let address_str =
Address::Unified(ufvk.default_address(DEFAULT_UA_REQUEST).0).encode(&wdb.params);
Address::from(ufvk.default_address(DEFAULT_UA_REQUEST).0).encode(&wdb.params);
wdb.conn.execute(
"INSERT INTO accounts (account, ufvk, address, transparent_address)
VALUES (?, ?, ?, '')",
Expand All @@ -1022,7 +1022,7 @@ mod tests {
// add a transparent "sent note"
#[cfg(feature = "transparent-inputs")]
{
let taddr = Address::Transparent(
let taddr = Address::from(
*ufvk
.default_address(DEFAULT_UA_REQUEST)
.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
))
})?;
let decoded_address = if let Address::Unified(ua) = decoded {
ua
*ua
} else {
return Err(WalletMigrationError::CorruptedData(
"Address in accounts table was not a Unified Address.".to_string(),
Expand All @@ -95,7 +95,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
return Err(WalletMigrationError::CorruptedData(format!(
"Decoded UA {} does not match the UFVK's default address {} at {:?}.",
address,
Address::Unified(expected_address).encode(&self.params),
Address::from(expected_address).encode(&self.params),

Check warning on line 98 in zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet/init/migrations/addresses_table.rs#L98

Added line #L98 was not covered by tests
idx,
)));
}
Expand All @@ -113,7 +113,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
let decoded_transparent_address = if let Address::Transparent(addr) =
decoded_transparent
{
addr
*addr
} else {
return Err(WalletMigrationError::CorruptedData(
"Address in transparent_address column of accounts table was not a transparent address.".to_string(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ mod tests {
match output.transfer_type {
TransferType::Outgoing | TransferType::WalletInternal => {
let recipient = if output.transfer_type == TransferType::Outgoing {
Recipient::Sapling(output.note.recipient())
Recipient::Sapling(Box::new(output.note.recipient()))
} else {
Recipient::InternalAccount(
output.account,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,11 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
"Derivation should have produced a UFVK containing a Sapling component.",
);
let (idx, expected_address) = dfvk.default_address();
if decoded_address != expected_address {
if *decoded_address != expected_address {
return Err(WalletMigrationError::CorruptedData(
format!("Decoded Sapling address {} does not match the ufvk's Sapling address {} at {:?}.",
address,
Address::Sapling(expected_address).encode(&self.params),
Address::from(expected_address).encode(&self.params),

Check warning on line 105 in zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs#L105

Added line #L105 was not covered by tests
idx)));
}
}
Expand All @@ -112,11 +112,11 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
}
Address::Unified(decoded_address) => {
let (expected_address, idx) = ufvk.default_address(ua_request);
if decoded_address != expected_address {
if *decoded_address != expected_address {
return Err(WalletMigrationError::CorruptedData(
format!("Decoded unified address {} does not match the ufvk's default address {} at {:?}.",
address,
Address::Unified(expected_address).encode(&self.params),
Address::from(expected_address).encode(&self.params),

Check warning on line 119 in zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs#L119

Added line #L119 was not covered by tests
idx)));
}
}
Expand Down
14 changes: 7 additions & 7 deletions zcash_client_sqlite/src/wallet/sapling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ pub(crate) mod tests {
// spends the first step's output.

// The first step will deshield to the wallet's default transparent address
let to0 = Address::Transparent(usk.default_transparent_address().0);
let to0 = Address::Transparent(Box::new(usk.default_transparent_address().0));
let request0 = zip321::TransactionRequest::new(vec![Payment {
recipient_address: to0,
amount: NonNegativeAmount::const_from_u64(50000),
Expand Down Expand Up @@ -754,14 +754,14 @@ pub(crate) mod tests {

// We'll use an internal transparent address that hasn't been added to the wallet
// to simulate an external transparent recipient.
let to1 = Address::Transparent(
let to1 = Address::Transparent(Box::new(
usk.transparent()
.to_account_pubkey()
.derive_internal_ivk()
.unwrap()
.default_address()
.0,
);
));
let request1 = zip321::TransactionRequest::new(vec![Payment {
recipient_address: to1,
amount: NonNegativeAmount::const_from_u64(40000),
Expand Down Expand Up @@ -1433,7 +1433,7 @@ pub(crate) mod tests {
let req = TransactionRequest::new(vec![
// payment to an external recipient
Payment {
recipient_address: Address::Sapling(addr2),
recipient_address: Address::from(addr2),
amount: amount_sent,
memo: None,
label: None,
Expand All @@ -1442,7 +1442,7 @@ pub(crate) mod tests {
},
// payment back to the originating wallet, simulating legacy change
Payment {
recipient_address: Address::Sapling(addr),
recipient_address: Address::from(addr),
amount: amount_legacy_change,
memo: None,
label: None,
Expand Down Expand Up @@ -1556,7 +1556,7 @@ pub(crate) mod tests {

// This first request will fail due to insufficient non-dust funds
let req = TransactionRequest::new(vec![Payment {
recipient_address: Address::Sapling(dfvk.default_address().1),
recipient_address: Address::from(dfvk.default_address().1),
amount: NonNegativeAmount::const_from_u64(50000),
memo: None,
label: None,
Expand All @@ -1581,7 +1581,7 @@ pub(crate) mod tests {
// This request will succeed, spending a single dust input to pay the 10000
// ZAT fee in addition to the 41000 ZAT output to the recipient
let req = TransactionRequest::new(vec![Payment {
recipient_address: Address::Sapling(dfvk.default_address().1),
recipient_address: Address::from(dfvk.default_address().1),
amount: NonNegativeAmount::const_from_u64(41000),
memo: None,
label: None,
Expand Down
3 changes: 2 additions & 1 deletion zcash_keys/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ The entries below are relative to the `zcash_client_backend` crate as of

### Changed
- `zcash_keys::address`:
- `RecipientAddress` has been renamed to `Address`
- `RecipientAddress` has been renamed to `Address`. Also, each variant now
`Box`es its contents to avoid large discrepancies in enum variant sizing.
- `Address::Shielded` has been renamed to `Address::Sapling`
- `UnifiedAddress::from_receivers` has been renamed to `UnifiedAddress::new`.
It no longer takes an Orchard receiver argument unless the `orchard` feature
Expand Down
31 changes: 14 additions & 17 deletions zcash_keys/src/address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,7 @@ impl UnifiedAddress {
self.expiry_height
.map(|h| unified::MetadataItem::ExpiryHeight(u32::from(h))),
)
.chain(
self.expiry_time
.map(unified::MetadataItem::ExpiryTime),
)
.chain(self.expiry_time.map(unified::MetadataItem::ExpiryTime))
.map(Item::Metadata);

data_items.chain(meta_items).collect()
Expand All @@ -250,26 +247,26 @@ impl UnifiedAddress {
/// An address that funds can be sent to.
#[derive(Debug, PartialEq, Eq, Clone)]
pub enum Address {
Sapling(PaymentAddress),
Transparent(TransparentAddress),
Unified(UnifiedAddress),
Sapling(Box<PaymentAddress>),
Transparent(Box<TransparentAddress>),
Unified(Box<UnifiedAddress>),
}

impl From<PaymentAddress> for Address {
fn from(addr: PaymentAddress) -> Self {
Address::Sapling(addr)
Address::Sapling(Box::new(addr))
}
}

impl From<TransparentAddress> for Address {
fn from(addr: TransparentAddress) -> Self {
Address::Transparent(addr)
Address::Transparent(Box::new(addr))
}
}

impl From<UnifiedAddress> for Address {
fn from(addr: UnifiedAddress) -> Self {
Address::Unified(addr)
Address::Unified(Box::new(addr))
}
}

Expand Down Expand Up @@ -312,12 +309,12 @@ impl Address {

match self {
Address::Sapling(pa) => ZcashAddress::from_sapling(net, pa.to_bytes()),
Address::Transparent(addr) => match addr {
Address::Transparent(addr) => match **addr {
TransparentAddress::PublicKeyHash(data) => {
ZcashAddress::from_transparent_p2pkh(net, *data)
ZcashAddress::from_transparent_p2pkh(net, data)
}
TransparentAddress::ScriptHash(data) => {
ZcashAddress::from_transparent_p2sh(net, *data)
ZcashAddress::from_transparent_p2sh(net, data)
}

Check warning on line 318 in zcash_keys/src/address.rs

View check run for this annotation

Codecov / codecov/patch

zcash_keys/src/address.rs#L317-L318

Added lines #L317 - L318 were not covered by tests
},
Address::Unified(ua) => ua.to_address(net),
Expand Down Expand Up @@ -345,9 +342,9 @@ pub mod testing {

pub fn arb_addr(request: UnifiedAddressRequest) -> impl Strategy<Value = Address> {
prop_oneof![
arb_payment_address().prop_map(Address::Sapling),
arb_transparent_addr().prop_map(Address::Transparent),
arb_unified_addr(Network::TestNetwork, request).prop_map(Address::Unified),
arb_payment_address().prop_map(Address::from),
arb_transparent_addr().prop_map(Address::from),
arb_unified_addr(Network::TestNetwork, request).prop_map(Address::from),
]
}
}
Expand Down Expand Up @@ -384,7 +381,7 @@ mod tests {
#[cfg(not(feature = "orchard"))]
let ua = UnifiedAddress::new(sapling, transparent, None, None).unwrap();

let addr = Address::Unified(ua);
let addr = Address::from(ua);
let addr_str = addr.encode(&MAIN_NETWORK);
assert_eq!(Address::decode(&MAIN_NETWORK, &addr_str), Some(addr));
}
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 @@ -646,10 +646,7 @@ impl UnifiedFullViewingKey {
self.expiry_height
.map(|h| unified::MetadataItem::ExpiryHeight(u32::from(h))),
)
.chain(
self.expiry_time
.map(unified::MetadataItem::ExpiryTime),
);
.chain(self.expiry_time.map(unified::MetadataItem::ExpiryTime));

let ufvk = unified::Ufvk::try_from_items(
data_items
Expand Down

0 comments on commit e78a5ec

Please sign in to comment.