Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history 
Co-authored-by: Daira Emma Hopwood <[email protected]>
  • Loading branch information
@nuttycom @daira
nuttycom and daira committed Sep 5, 2023
1 parent 40fef54 commit cb7a497
Show file tree
Hide file tree
Showing 7 changed files with 168 additions and 218 deletions.
10 changes: 5 additions & 5 deletions zcash_client_backend/src/data_api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub enum NullifierQuery {
}

/// Balance information for a value within a single shielded pool in an account.
#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct Balance {
/// The value in the account that may currently be spent; it is possible to compute witnesses
/// for all the notes that comprise this value, and all of this value is confirmed to the
Expand Down Expand Up @@ -86,7 +86,7 @@ impl Balance {

/// Balance information for a single account. The sum of this struct's fields is the total balance
/// of the wallet.
#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct AccountBalance {
/// The value of unspent Sapling outputs belonging to the account.
pub sapling_balance: Balance,
Expand Down Expand Up @@ -144,7 +144,7 @@ impl<T> Ratio<T> {
/// can only be certain to be unspent in the case that [`Self::is_synced`] is true, and even in
/// this circumstance it is possible that a newly created transaction could conflict with a
/// not-yet-mined transaction in the mempool.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct WalletSummary {
account_balances: BTreeMap<AccountId, AccountBalance>,
chain_tip_height: BlockHeight,
Expand Down Expand Up @@ -178,8 +178,8 @@ impl WalletSummary {
self.chain_tip_height
}

/// Returns the height below which all blocks wallet have been scanned, ignoring blocks below
/// the wallet birthday.
/// Returns the height below which all blocks have been scanned by the wallet, ignoring blocks
/// below the wallet birthday.
pub fn fully_scanned_height(&self) -> BlockHeight {
self.fully_scanned_height
}
Expand Down
115 changes: 48 additions & 67 deletions zcash_client_sqlite/src/chain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,10 @@ mod tests {

use zcash_primitives::{
block::BlockHash,
transaction::{components::Amount, fees::zip317::FeeRule},
transaction::{
components::{amount::NonNegativeAmount, Amount},
fees::zip317::FeeRule,
},
zip32::ExtendedSpendingKey,
};

Expand All @@ -344,7 +347,7 @@ mod tests {

use crate::{
testing::{AddressType, TestBuilder},
wallet::{get_balance, truncate_to_height},
wallet::truncate_to_height,
AccountId,
};

Expand Down Expand Up @@ -441,24 +444,21 @@ mod tests {

let dfvk = st.test_account_sapling().unwrap();

// Account balance should be zero
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
Amount::zero()
);
// Wallet summary is not yet available
assert_eq!(st.get_wallet_summary(0), None);

// Create fake CompactBlocks sending value to the address
let value = Amount::from_u64(5).unwrap();
let value2 = Amount::from_u64(7).unwrap();
let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value);
st.generate_next_block(&dfvk, AddressType::DefaultExternal, value2);
let value = NonNegativeAmount::from_u64(5).unwrap();
let value2 = NonNegativeAmount::from_u64(7).unwrap();
let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into());
st.generate_next_block(&dfvk, AddressType::DefaultExternal, value2.into());

// Scan the cache
st.scan_cached_blocks(h, 2);

// Account balance should reflect both received notes
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
st.get_total_balance(AccountId::from(0)),
(value + value2).unwrap()
);

Expand All @@ -469,7 +469,7 @@ mod tests {

// Account balance should be unaltered
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
st.get_total_balance(AccountId::from(0)),
(value + value2).unwrap()
);

Expand All @@ -479,17 +479,14 @@ mod tests {
.unwrap();

// Account balance should only contain the first received note
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
value
);
assert_eq!(st.get_total_balance(AccountId::from(0)), value);

// Scan the cache again
st.scan_cached_blocks(h, 2);

// Account balance should again reflect both received notes
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
st.get_total_balance(AccountId::from(0)),
(value + value2).unwrap()
);
}
Expand All @@ -505,26 +502,23 @@ mod tests {
let dfvk = st.test_account_sapling().unwrap();

// Create a block with height SAPLING_ACTIVATION_HEIGHT
let value = Amount::from_u64(50000).unwrap();
let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value);
let value = NonNegativeAmount::from_u64(50000).unwrap();
let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into());
st.scan_cached_blocks(h1, 1);
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
value
);
assert_eq!(st.get_total_balance(AccountId::from(0)), value);

// Create blocks to reach SAPLING_ACTIVATION_HEIGHT + 2
let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value);
let (h3, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value);
let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into());
let (h3, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into());

// Scan the later block first
st.scan_cached_blocks(h3, 1);

// Now scan the block of height SAPLING_ACTIVATION_HEIGHT + 1
st.scan_cached_blocks(h2, 1);
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
Amount::from_u64(150_000).unwrap()
st.get_total_balance(AccountId::from(0)),
NonNegativeAmount::from_u64(150_000).unwrap()
);

// We can spend the received notes
Expand Down Expand Up @@ -562,35 +556,29 @@ mod tests {

let dfvk = st.test_account_sapling().unwrap();

// Account balance should be zero
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
Amount::zero()
);
// Wallet summary is not yet available
assert_eq!(st.get_wallet_summary(0), None);

// Create a fake CompactBlock sending value to the address
let value = Amount::from_u64(5).unwrap();
let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value);
let value = NonNegativeAmount::from_u64(5).unwrap();
let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into());

// Scan the cache
st.scan_cached_blocks(h1, 1);

// Account balance should reflect the received note
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
value
);
assert_eq!(st.get_total_balance(AccountId::from(0)), value);

// Create a second fake CompactBlock sending more value to the address
let value2 = Amount::from_u64(7).unwrap();
let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value2);
let value2 = NonNegativeAmount::from_u64(7).unwrap();
let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value2.into());

// Scan the cache again
st.scan_cached_blocks(h2, 1);

// Account balance should reflect both received notes
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
st.get_total_balance(AccountId::from(0)),
(value + value2).unwrap()
);
}
Expand All @@ -603,38 +591,33 @@ mod tests {
.build();
let dfvk = st.test_account_sapling().unwrap();

// Account balance should be zero
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
Amount::zero()
);
// Wallet summary is not yet available
assert_eq!(st.get_wallet_summary(0), None);

// Create a fake CompactBlock sending value to the address
let value = Amount::from_u64(5).unwrap();
let value = NonNegativeAmount::from_u64(5).unwrap();
let (received_height, _, nf) =
st.generate_next_block(&dfvk, AddressType::DefaultExternal, value);
st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into());

// Scan the cache
st.scan_cached_blocks(received_height, 1);

// Account balance should reflect the received note
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
value
);
assert_eq!(st.get_total_balance(AccountId::from(0)), value);

// Create a second fake CompactBlock spending value from the address
let extsk2 = ExtendedSpendingKey::master(&[0]);
let to2 = extsk2.default_address().1;
let value2 = Amount::from_u64(2).unwrap();
let (spent_height, _) = st.generate_next_block_spending(&dfvk, (nf, value), to2, value2);
let value2 = NonNegativeAmount::from_u64(2).unwrap();
let (spent_height, _) =
st.generate_next_block_spending(&dfvk, (nf, value.into()), to2, value2.into());

// Scan the cache again
st.scan_cached_blocks(spent_height, 1);

// Account balance should equal the change
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
st.get_total_balance(AccountId::from(0)),
(value - value2).unwrap()
);
}
Expand All @@ -648,29 +631,27 @@ mod tests {

let dfvk = st.test_account_sapling().unwrap();

// Account balance should be zero
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
Amount::zero()
);
// Wallet summary is not yet available
assert_eq!(st.get_wallet_summary(0), None);

// Create a fake CompactBlock sending value to the address
let value = Amount::from_u64(5).unwrap();
let value = NonNegativeAmount::from_u64(5).unwrap();
let (received_height, _, nf) =
st.generate_next_block(&dfvk, AddressType::DefaultExternal, value);
st.generate_next_block(&dfvk, AddressType::DefaultExternal, value.into());

// Create a second fake CompactBlock spending value from the address
let extsk2 = ExtendedSpendingKey::master(&[0]);
let to2 = extsk2.default_address().1;
let value2 = Amount::from_u64(2).unwrap();
let (spent_height, _) = st.generate_next_block_spending(&dfvk, (nf, value), to2, value2);
let value2 = NonNegativeAmount::from_u64(2).unwrap();
let (spent_height, _) =
st.generate_next_block_spending(&dfvk, (nf, value.into()), to2, value2.into());

// Scan the spending block first.
st.scan_cached_blocks(spent_height, 1);

// Account balance should equal the change
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
st.get_total_balance(AccountId::from(0)),
(value - value2).unwrap()
);

Expand All @@ -679,7 +660,7 @@ mod tests {

// Account balance should be the same.
assert_eq!(
get_balance(&st.wallet().conn, AccountId::from(0)).unwrap(),
st.get_total_balance(AccountId::from(0)),
(value - value2).unwrap()
);
}
Expand Down
Loading

0 comments on commit cb7a497

Please sign in to comment.