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: Assign UUIDs to each account #1631

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ jobs:
run: |
{
echo 'UUIDS<<EOF'
git grep -h "Uuid::from_u128" zcash_client_sqlite/ |
git grep -h "const MIGRATION_ID: Uuid = Uuid::from_u128" zcash_client_sqlite/ |
sed -e "s,.*Uuid::from_u128(0x,," -e "s,_,-,g" -e "s,);,,"
echo EOF
} >> "$GITHUB_OUTPUT"
Expand Down
4 changes: 4 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions zcash_client_backend/src/data_api/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,8 @@ where
<Cache::BlockSource as BlockSource>::Error: fmt::Debug,
ParamsT: consensus::Parameters + Send + 'static,
DbT: InputSource + WalletTest + WalletWrite + WalletCommitmentTrees,
<DbT as WalletRead>::AccountId: ConditionallySelectable + Default + Send + 'static,
<DbT as WalletRead>::AccountId:
std::fmt::Debug + ConditionallySelectable + Default + Send + 'static,
{
/// Invokes [`scan_cached_blocks`] with the given arguments, expecting success.
pub fn scan_cached_blocks(&mut self, from_height: BlockHeight, limit: usize) -> ScanSummary {
Expand Down Expand Up @@ -855,7 +856,7 @@ where
impl<Cache, DbT, ParamsT, AccountIdT, ErrT> TestState<Cache, DbT, ParamsT>
where
ParamsT: consensus::Parameters + Send + 'static,
AccountIdT: std::cmp::Eq + std::hash::Hash,
AccountIdT: std::fmt::Debug + std::cmp::Eq + std::hash::Hash,
ErrT: std::fmt::Debug,
DbT: InputSource<AccountId = AccountIdT, Error = ErrT>
+ WalletTest
Expand Down Expand Up @@ -1286,7 +1287,7 @@ pub struct InitialChainState {
/// Trait representing the ability to construct a new data store for use in a test.
pub trait DataStoreFactory {
type Error: core::fmt::Debug;
type AccountId: ConditionallySelectable + Default + Hash + Eq + Send + 'static;
type AccountId: std::fmt::Debug + ConditionallySelectable + Default + Hash + Eq + Send + 'static;
type Account: Account<AccountId = Self::AccountId> + Clone;
type DsError: core::fmt::Debug;
type DataStore: InputSource<AccountId = Self::AccountId, Error = Self::DsError>
Expand Down
20 changes: 10 additions & 10 deletions zcash_client_backend/src/data_api/testing/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1559,7 +1559,7 @@ pub fn external_address_change_spends_detected_in_restore_from_seed<T: ShieldedP
// Add two accounts to the wallet.
let seed = Secret::new([0u8; 32].to_vec());
let birthday = AccountBirthday::from_sapling_activation(st.network(), BlockHash([0; 32]));
let (account_id, usk) = st.wallet_mut().create_account(&seed, &birthday).unwrap();
let (account1, usk) = st.wallet_mut().create_account(&seed, &birthday).unwrap();
let dfvk = T::sk_to_fvk(T::usk_to_sk(&usk));

let (account2, usk2) = st.wallet_mut().create_account(&seed, &birthday).unwrap();
Expand All @@ -1571,8 +1571,8 @@ pub fn external_address_change_spends_detected_in_restore_from_seed<T: ShieldedP
st.scan_cached_blocks(h, 1);

// Spendable balance matches total balance
assert_eq!(st.get_total_balance(account_id), value);
assert_eq!(st.get_spendable_balance(account_id, 1), value);
assert_eq!(st.get_total_balance(account1), value);
assert_eq!(st.get_spendable_balance(account1, 1), value);
assert_eq!(st.get_total_balance(account2), NonNegativeAmount::ZERO);

let amount_sent = NonNegativeAmount::from_u64(20000).unwrap();
Expand Down Expand Up @@ -1610,35 +1610,35 @@ pub fn external_address_change_spends_detected_in_restore_from_seed<T: ShieldedP
let pending_change = (amount_left - amount_legacy_change).unwrap();

// The "legacy change" is not counted by get_pending_change().
assert_eq!(st.get_pending_change(account_id, 1), pending_change);
assert_eq!(st.get_pending_change(account1, 1), pending_change);
// We spent the only note so we only have pending change.
assert_eq!(st.get_total_balance(account_id), pending_change);
assert_eq!(st.get_total_balance(account1), pending_change);

let (h, _) = st.generate_next_block_including(txid);
st.scan_cached_blocks(h, 1);

assert_eq!(st.get_total_balance(account2), amount_sent,);
assert_eq!(st.get_total_balance(account_id), amount_left);
assert_eq!(st.get_total_balance(account1), amount_left);

st.reset();

// Account creation and DFVK derivation should be deterministic.
let (_, restored_usk) = st.wallet_mut().create_account(&seed, &birthday).unwrap();
let (account1, restored_usk) = st.wallet_mut().create_account(&seed, &birthday).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is necessary because we get new account UUIDs when we regenerate the wallet.

assert!(T::fvks_equal(
&T::sk_to_fvk(T::usk_to_sk(&restored_usk)),
&dfvk,
));

let (_, restored_usk2) = st.wallet_mut().create_account(&seed, &birthday).unwrap();
let (account2, restored_usk2) = st.wallet_mut().create_account(&seed, &birthday).unwrap();
assert!(T::fvks_equal(
&T::sk_to_fvk(T::usk_to_sk(&restored_usk2)),
&dfvk2,
));

st.scan_cached_blocks(st.sapling_activation_height(), 2);

assert_eq!(st.get_total_balance(account2), amount_sent,);
assert_eq!(st.get_total_balance(account_id), amount_left);
assert_eq!(st.get_total_balance(account2), amount_sent);
assert_eq!(st.get_total_balance(account1), amount_left);
}

#[allow(dead_code)]
Expand Down
28 changes: 26 additions & 2 deletions zcash_client_sqlite/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,30 @@ and this library adheres to Rust's notion of

## [Unreleased]

### Added
- `zcash_client_sqlite::AccountUuid`
- `zcash_client_sqlite::Account::uuid`
- `zcash_client_sqlite::WalletDb::get_account_for_uuid`

### Changed
- The `v_transactions` view has been modified:
- The `account_id` column has been replaced with `account_uuid`.
- The `v_tx_outputs` view has been modified:
- The `from_account_id` column has been replaced with `from_account_uuid`.
- The `to_account_id` column has been replaced with `to_account_uuid`.
- The `WalletRead` and `InputSource` impls for `WalletDb` now set the `AccountId`
associated type to `AccountUuid`.
- Variants of `SqliteClientError` have changed:
- The `AccountCollision` and `ReachedGapLimit` now carry `AccountUuid` values
instead of `AccountId`s.
- `SqliteClientError::AccountIdDiscontinuity` has been removed as it is now
unused.
- `SqliteClientError::AccountIdOutOfRange` has been renamed to
`Zip32AccountIndexOutOfRange`.

### Removed
- `zcash_client_sqlite::AccountId` (use `AccountUuid` instead).

## [0.13.0] - 2024-11-14

### Added
Expand All @@ -15,7 +39,7 @@ and this library adheres to Rust's notion of

### Changed
- MSRV is now 1.77.0.
- Migrated to `zcash_primitives 0.20`, `zcash_keys 0.5`,
- Migrated to `zcash_primitives 0.20`, `zcash_keys 0.5`,
`zcash_client_backend 0.15`.
- Migrated from `schemer` to our fork `schemerz`.
- Migrated to `rusqlite 0.32`.
Expand All @@ -35,7 +59,7 @@ and this library adheres to Rust's notion of
summary information is now only returned in the case that some note
commitment tree size information can be determined, either from subtree root
download or from downloaded block data. NOTE: The recovery progress ratio may
be present as `0:0` in the case that the recovery range contains no notes;
be present as `0:0` in the case that the recovery range contains no notes;
this was not adequately documented in the previous release.

## [0.12.0] - 2024-10-04
Expand Down
4 changes: 2 additions & 2 deletions zcash_client_sqlite/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,11 @@ incrementalmerkletree.workspace = true
shardtree = { workspace = true, features = ["legacy-api"] }

# - SQLite databases
rusqlite = { workspace = true, features = ["time", "array"] }
rusqlite = { workspace = true, features = ["time", "array", "uuid"] }
schemerz.workspace = true
schemerz-rusqlite.workspace = true
time.workspace = true
uuid.workspace = true
uuid = { workspace = true, features = ["v4"] }
regex = "1.4"

# Dependencies used internally:
Expand Down
18 changes: 6 additions & 12 deletions zcash_client_sqlite/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@
use zcash_primitives::zip32;
use zcash_primitives::{consensus::BlockHeight, transaction::components::amount::BalanceError};

use crate::wallet::commitment_tree;
use crate::AccountId;
use crate::{wallet::commitment_tree, AccountUuid};

#[cfg(feature = "transparent-inputs")]
use {
Expand Down Expand Up @@ -79,7 +78,7 @@

/// The account being added collides with an existing account in the wallet with the given ID.
/// The collision can be on the seed and ZIP-32 account index, or a shared FVK component.
AccountCollision(AccountId),
AccountCollision(AccountUuid),

/// The account was imported, and ZIP-32 derivation information is not known for it.
UnknownZip32Derivation,
Expand All @@ -90,12 +89,8 @@
/// An error occurred while processing an account due to a failure in deriving the account's keys.
BadAccountData(String),

/// A caller attempted to initialize the accounts table with a discontinuous
/// set of account identifiers.
AccountIdDiscontinuity,

/// A caller attempted to construct a new account with an invalid account identifier.
AccountIdOutOfRange,
/// A caller attempted to construct a new account with an invalid ZIP 32 account identifier.
Zip32AccountIndexOutOfRange,

/// The address associated with a record being inserted was not recognized as
/// belonging to the wallet.
Expand Down Expand Up @@ -129,7 +124,7 @@
/// ephemeral address outputs have been mined. The parameters are the account id and
Copy link
Contributor

@daira daira Nov 28, 2024

Choose a reason for hiding this comment

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

Suggested change
/// ephemeral address outputs have been mined. The parameters are the account id and
/// ephemeral address outputs have been mined. The parameters are the account UUID and

Also s/account_id/account_uuid/ on lines 190 and 192.

/// the index that could not safely be reserved.
#[cfg(feature = "transparent-inputs")]
ReachedGapLimit(AccountId, u32),
ReachedGapLimit(AccountUuid, u32),

/// An ephemeral address would be reused. The parameters are the address in string
/// form, and the txid of the earliest transaction in which it is known to have been
Expand Down Expand Up @@ -181,8 +176,7 @@
SqliteClientError::UnknownZip32Derivation => write!(f, "ZIP-32 derivation information is not known for this account."),
SqliteClientError::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {}", u32::from(*acct_id)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SqliteClientError::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {}", u32::from(*acct_id)),
SqliteClientError::KeyDerivationError(zip32_index) => write!(f, "Key derivation failed for ZIP 32 account index {}", u32::from(*zip32_index)),

SqliteClientError::BadAccountData(e) => write!(f, "Failed to add account: {}", e),
SqliteClientError::AccountIdDiscontinuity => write!(f, "Wallet account identifiers must be sequential."),
SqliteClientError::AccountIdOutOfRange => write!(f, "Wallet account identifiers must be less than 0x7FFFFFFF."),
SqliteClientError::Zip32AccountIndexOutOfRange => write!(f, "ZIP 32 account identifiers must be less than 0x7FFFFFFF."),

Check warning on line 179 in zcash_client_sqlite/src/error.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_sqlite/src/error.rs#L179

Added line #L179 was not covered by tests
SqliteClientError::AccountCollision(id) => write!(f, "An account corresponding to the data provided already exists in the wallet with internal identifier {}.", id.0),
Copy link
Contributor

@daira daira Nov 28, 2024

Choose a reason for hiding this comment

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

Suggested change
SqliteClientError::AccountCollision(id) => write!(f, "An account corresponding to the data provided already exists in the wallet with internal identifier {}.", id.0),
SqliteClientError::AccountCollision(account_uuid) => write!(f, "An account corresponding to the data provided already exists in the wallet with UUID {account_uuid:?}."),

(probably needs a rustfmt)

#[cfg(feature = "transparent-inputs")]
SqliteClientError::AddressNotRecognized(_) => write!(f, "The address associated with a received txo is not identifiable as belonging to the wallet."),
Expand Down
Loading
Loading