zec
/
librustzcash
mirror of https://github.com/zcash/librustzcash.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Apply suggestions from code review 

Co-authored-by: Jack Grigg <thestr4d@gmail.com>
Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>
This commit is contained in:
Kris Nuttycombe 2024-10-20 11:52:07 -06:00 committed by Kris Nuttycombe
parent 5bf0f16dd7
commit fde0fc9730
11 changed files with 75 additions and 70 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
zcash_client_backend/CHANGELOG.md
View File

@ -24,12 +24,12 @@ and this library adheres to Rust's notion of
`ChangeErrT` and `NoteRefT`.
- The following methods each now take an additional `change_strategy`
argument, along with an associated `ChangeT` type parameter:
- `zcash_client_backend::data_api::wallet::spend`
- `zcash_client_backend::data_api::wallet::propose_transfer`
- `zcash_client_backend::data_api::wallet::propose_shielding`. This method
also now takes an additional `to_account` argument.
- `zcash_client_backend::data_api::wallet::shield_transparent_funds`. This
method also now takes an additional `to_account` argument.
- `wallet::spend`
- `wallet::propose_transfer`
- `wallet::propose_shielding`. This method also now takes an additional
`to_account` argument.
- `wallet::shield_transparent_funds`. This method also now takes an
additional `to_account` argument.
- `wallet::input_selection::InputSelectionError` now has an additional `Change`
variant. This necessitates the addition of two type parameters.
- `wallet::input_selection::InputSelector::propose_transaction` takes an
@ -49,20 +49,16 @@ and this library adheres to Rust's notion of
has been removed, along with the additional type parameters it necessitated.
- The arguments to `wallet::input_selection::GreedyInputSelector::new` have
changed.
- `zcash_client_backend::fees::ChangeStrategy` has changed. It has two new
associated types, `MetaSource` and `WalletMeta`, and its `FeeRule` associated
type now has an additional `Clone` bound. In addition, it defines a new
`fetch_wallet_meta` method, and the arguments to `compute_balance` have
changed.
- `zcash_client_backend::fees::fixed::SingleOutputChangeStrategy::new`
now takes an additional `DustOutputPolicy` argument. It also now carries
an additional type parameter.
- `zcash_client_backend::fees::standard::SingleOutputChangeStrategy::new`
now takes an additional `DustOutputPolicy` argument. It also now carries
an additional type parameter.
- `zcash_client_backend::fees::zip317::SingleOutputChangeStrategy::new`
now takes an additional `DustOutputPolicy` argument. It also now carries
an additional type parameter.
- `zcash_client_backend::fees`:
- `ChangeStrategy` has changed. It has two new associated types, `MetaSource`
and `WalletMeta`, and its `FeeRule` associated type now has an additional
`Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and
the arguments to `compute_balance` have changed.
- The following methods now take an additional `DustOutputPolicy` argument,
and carry an additional type parameter:
- `fixed::SingleOutputChangeStrategy::new`
- `standard::SingleOutputChangeStrategy::new`
- `zip317::SingleOutputChangeStrategy::new`
### Changed
- MSRV is now 1.77.0.
@ -72,6 +68,8 @@ and this library adheres to Rust's notion of
- `zcash_client_backend::data_api`:
- `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have
been removed. Use `WalletSummary::progress` instead.
- `testing::input_selector` use explicit `InputSelector` constructors
directly instead.
- `zcash_client_backend::fees`:
- `impl From<BalanceError> for ChangeError<...>`

13
zcash_client_backend/src/data_api.rs
View File

@ -848,12 +848,7 @@ impl WalletMeta {
/// Returns the total number of unspent shielded notes belonging to the account for which this
/// was generated.
pub fn total_note_count(&self) -> usize {
#[cfg(feature = "orchard")]
let orchard_note_count = self.orchard_note_count;
#[cfg(not(feature = "orchard"))]
let orchard_note_count = 0;
self.sapling_note_count + orchard_note_count
self.sapling_note_count + self.note_count(ShieldedProtocol::Orchard)
}
}
@ -903,8 +898,10 @@ pub trait InputSource {
/// Returns metadata describing the structure of the wallet for the specified account.
///
/// The returned metadata value must exclude spent notes and unspent notes having value less
/// than the specified minimum value or identified in the given exclude list.
/// The returned metadata value must exclude:
/// - spent notes;
/// - unspent notes having value less than the specified minimum value;
/// - unspent notes identified in the given `exclude` list.
fn get_wallet_metadata(
&self,
account: Self::AccountId,

2
zcash_client_backend/src/data_api/testing/pool.rs
View File

@ -422,7 +422,7 @@ pub fn send_with_multiple_change_outputs<T: ShieldedPoolTester>(
.unwrap();
assert_eq!(sent_note_ids.len(), 3);
// The sent memo should be the empty memo for the sent output, and the
// The sent memo should be the empty memo for the sent output, and each
// change output's memo should be as specified.
let mut change_memo_count = 0;
let mut found_sent_empty_memo = false;

7
zcash_client_backend/src/data_api/wallet.rs
View File

@ -115,6 +115,8 @@ where
Ok(())
}
/// Errors that may be generated in construction of proposals for shielded->shielded or
/// shielded->transparent transfers.
pub type ProposeTransferErrT<DbT, CommitmentTreeErrT, InputsT, ChangeT> = Error<
<DbT as WalletRead>::Error,
CommitmentTreeErrT,
@ -124,6 +126,8 @@ pub type ProposeTransferErrT<DbT, CommitmentTreeErrT, InputsT, ChangeT> = Error<
<<InputsT as InputSelector>::InputSource as InputSource>::NoteRef,
>;
/// Errors that may be generated in construction of proposals for transparent->shielded
/// wallet-internal transfers.
#[cfg(feature = "transparent-inputs")]
pub type ProposeShieldingErrT<DbT, CommitmentTreeErrT, InputsT, ChangeT> = Error<
<DbT as WalletRead>::Error,
@ -134,6 +138,7 @@ pub type ProposeShieldingErrT<DbT, CommitmentTreeErrT, InputsT, ChangeT> = Error
Infallible,
>;
/// Errors that may be generated in combined creation and execution of transaction proposals.
pub type CreateErrT<DbT, InputsErrT, FeeRuleT, ChangeErrT, N> = Error<
<DbT as WalletRead>::Error,
<DbT as WalletCommitmentTrees>::Error,
@ -143,6 +148,7 @@ pub type CreateErrT<DbT, InputsErrT, FeeRuleT, ChangeErrT, N> = Error<
N,
>;
/// Errors that may be generated in the execution of proposals that may send shielded inputs.
pub type TransferErrT<DbT, InputsT, ChangeT> = Error<
<DbT as WalletRead>::Error,
<DbT as WalletCommitmentTrees>::Error,
@ -152,6 +158,7 @@ pub type TransferErrT<DbT, InputsT, ChangeT> = Error<
<<InputsT as InputSelector>::InputSource as InputSource>::NoteRef,
>;
/// Errors that may be generated in the execution of shielding proposals.
#[cfg(feature = "transparent-inputs")]
pub type ShieldErrT<DbT, InputsT, ChangeT> = Error<
<DbT as WalletRead>::Error,

21
zcash_client_backend/src/fees.rs
View File

@ -393,13 +393,13 @@ impl SplitPolicy {
)
.unwrap(),
);
if split_count > NonZeroUsize::MIN
&& *per_output_change.quotient() < self.min_split_output_size
{
split_count = NonZeroUsize::new(usize::from(split_count) - 1)
.expect("split_count checked to be > 1");
} else {
if *per_output_change.quotient() >= self.min_split_output_size {
return split_count;
} else if let Some(new_count) = NonZeroUsize::new(usize::from(split_count) - 1) {
split_count = new_count;
} else {
// We always create at least one change output.
return NonZeroUsize::MIN;
}
}
}
@ -452,7 +452,10 @@ pub trait ChangeStrategy {
/// own trait that descends from [`InputSource`] and adds the required capabilities there, and
/// then implement that trait for their desired database backend.
type MetaSource: InputSource;
type WalletMeta;
/// Tye type of wallet metadata that this change strategy relies upon in order to compute
/// change.
type WalletMetaT;
/// Returns the fee rule that this change strategy will respect when performing
/// balance computations.
@ -465,7 +468,7 @@ pub trait ChangeStrategy {
meta_source: &Self::MetaSource,
account: <Self::MetaSource as InputSource>::AccountId,
exclude: &[<Self::MetaSource as InputSource>::NoteRef],
) -> Result<Self::WalletMeta, <Self::MetaSource as InputSource>::Error>;
) -> Result<Self::WalletMetaT, <Self::MetaSource as InputSource>::Error>;
/// Computes the totals of inputs, suggested change amounts, and fees given the
/// provided inputs and outputs being used to construct a transaction.
@ -502,7 +505,7 @@ pub trait ChangeStrategy {
sapling: &impl sapling::BundleView<NoteRefT>,
#[cfg(feature = "orchard")] orchard: &impl orchard::BundleView<NoteRefT>,
ephemeral_balance: Option<&EphemeralBalance>,
wallet_meta: Option<&Self::WalletMeta>,
wallet_meta: Option<&Self::WalletMetaT>,
) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>>;
}

21
zcash_client_backend/src/fees/common.rs
View File

@ -233,27 +233,20 @@ where
)?;
let change_pool = select_change_pool(&net_flows, cfg.fallback_change_pool);
let target_change_count = wallet_meta.map_or(1, |m| {
usize::from(cfg.split_policy.target_output_count)
.saturating_sub(m.total_note_count())
.max(1)
});
let target_change_counts = OutputManifest {
transparent: 0,
sapling: if change_pool == ShieldedProtocol::Sapling {
wallet_meta.map_or(1, |m| {
std::cmp::max(
usize::from(cfg.split_policy.target_output_count)
.saturating_sub(m.total_note_count()),
1,
)
})
target_change_count
} else {
0
},
orchard: if change_pool == ShieldedProtocol::Orchard {
wallet_meta.map_or(1, |m| {
std::cmp::max(
usize::from(cfg.split_policy.target_output_count)
.saturating_sub(m.total_note_count()),
1,
)
})
target_change_count
} else {
0
},

6
zcash_client_backend/src/fees/fixed.rs
View File

@ -60,7 +60,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
type FeeRule = FixedFeeRule;
type Error = BalanceError;
type MetaSource = I;
type WalletMeta = ();
type WalletMetaT = ();
fn fee_rule(&self) -> &Self::FeeRule {
&self.fee_rule
@ -71,7 +71,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
_meta_source: &Self::MetaSource,
_account: <Self::MetaSource as InputSource>::AccountId,
_exclude: &[<Self::MetaSource as crate::data_api::InputSource>::NoteRef],
) -> Result<Self::WalletMeta, <Self::MetaSource as crate::data_api::InputSource>::Error> {
) -> Result<Self::WalletMetaT, <Self::MetaSource as crate::data_api::InputSource>::Error> {
Ok(())
}
@ -84,7 +84,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
sapling: &impl sapling_fees::BundleView<NoteRefT>,
#[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
ephemeral_balance: Option<&EphemeralBalance>,
_wallet_meta: Option<&Self::WalletMeta>,
_wallet_meta: Option<&Self::WalletMetaT>,
) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
let split_policy = SplitPolicy::single_output();
let cfg = SinglePoolBalanceConfig::new(

6
zcash_client_backend/src/fees/standard.rs
View File

@ -64,7 +64,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
type FeeRule = StandardFeeRule;
type Error = Zip317FeeError;
type MetaSource = I;
type WalletMeta = ();
type WalletMetaT = ();
fn fee_rule(&self) -> &Self::FeeRule {
&self.fee_rule
@ -75,7 +75,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
_meta_source: &Self::MetaSource,
_account: <Self::MetaSource as InputSource>::AccountId,
_exclude: &[<Self::MetaSource as crate::data_api::InputSource>::NoteRef],
) -> Result<Self::WalletMeta, <Self::MetaSource as crate::data_api::InputSource>::Error> {
) -> Result<Self::WalletMetaT, <Self::MetaSource as crate::data_api::InputSource>::Error> {
Ok(())
}
@ -88,7 +88,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
sapling: &impl sapling_fees::BundleView<NoteRefT>,
#[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
ephemeral_balance: Option<&EphemeralBalance>,
wallet_meta: Option<&Self::WalletMeta>,
wallet_meta: Option<&Self::WalletMetaT>,
) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
#[allow(deprecated)]
match self.fee_rule() {

20
zcash_client_backend/src/fees/zip317.rs
View File

@ -67,7 +67,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
type FeeRule = Zip317FeeRule;
type Error = Zip317FeeError;
type MetaSource = I;
type WalletMeta = ();
type WalletMetaT = ();
fn fee_rule(&self) -> &Self::FeeRule {
&self.fee_rule
@ -78,7 +78,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
_meta_source: &Self::MetaSource,
_account: <Self::MetaSource as InputSource>::AccountId,
_exclude: &[<Self::MetaSource as InputSource>::NoteRef],
) -> Result<Self::WalletMeta, <Self::MetaSource as InputSource>::Error> {
) -> Result<Self::WalletMetaT, <Self::MetaSource as InputSource>::Error> {
Ok(())
}
@ -91,7 +91,7 @@ impl<I: InputSource> ChangeStrategy for SingleOutputChangeStrategy<I> {
sapling: &impl sapling_fees::BundleView<NoteRefT>,
#[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
ephemeral_balance: Option<&EphemeralBalance>,
_wallet_meta: Option<&Self::WalletMeta>,
_wallet_meta: Option<&Self::WalletMetaT>,
) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
let split_policy = SplitPolicy::single_output();
let cfg = SinglePoolBalanceConfig::new(
@ -139,10 +139,10 @@ impl<I> MultiOutputChangeStrategy<I> {
/// change value is available to create notes with at least the minimum value dictated by the
/// split policy.
///
/// `fallback_change_pool`: the pool to which change will be sent if when more than one
/// shielded pool is enabled via feature flags, and the transaction has no shielded inputs.
/// `split_policy`: A policy value describing how the change value should be returned as
/// multiple notes.
/// - `fallback_change_pool`: the pool to which change will be sent if when more than one
/// shielded pool is enabled via feature flags, and the transaction has no shielded inputs.
/// - `split_policy`: A policy value describing how the change value should be returned as
/// multiple notes.
pub fn new(
fee_rule: Zip317FeeRule,
change_memo: Option<MemoBytes>,
@ -165,7 +165,7 @@ impl<I: InputSource> ChangeStrategy for MultiOutputChangeStrategy<I> {
type FeeRule = Zip317FeeRule;
type Error = Zip317FeeError;
type MetaSource = I;
type WalletMeta = WalletMeta;
type WalletMetaT = WalletMeta;
fn fee_rule(&self) -> &Self::FeeRule {
&self.fee_rule
@ -176,7 +176,7 @@ impl<I: InputSource> ChangeStrategy for MultiOutputChangeStrategy<I> {
meta_source: &Self::MetaSource,
account: <Self::MetaSource as InputSource>::AccountId,
exclude: &[<Self::MetaSource as InputSource>::NoteRef],
) -> Result<Self::WalletMeta, <Self::MetaSource as InputSource>::Error> {
) -> Result<Self::WalletMetaT, <Self::MetaSource as InputSource>::Error> {
meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude)
}
@ -189,7 +189,7 @@ impl<I: InputSource> ChangeStrategy for MultiOutputChangeStrategy<I> {
sapling: &impl sapling_fees::BundleView<NoteRefT>,
#[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
ephemeral_balance: Option<&EphemeralBalance>,
wallet_meta: Option<&Self::WalletMeta>,
wallet_meta: Option<&Self::WalletMetaT>,
) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
let cfg = SinglePoolBalanceConfig::new(
params,

5
zcash_client_sqlite/src/lib.rs
View File

@ -354,12 +354,16 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> InputSource for
min_value: NonNegativeAmount,
exclude: &[Self::NoteRef],
) -> Result<WalletMeta, Self::Error> {
let chain_tip_height = wallet::chain_tip_height(self.conn.borrow())?
.ok_or(SqliteClientError::ChainHeightUnknown)?;
let sapling_note_count = count_outputs(
self.conn.borrow(),
account_id,
min_value,
exclude,
ShieldedProtocol::Sapling,
chain_tip_height,
)?;
#[cfg(feature = "orchard")]
@ -369,6 +373,7 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> InputSource for
min_value,
exclude,
ShieldedProtocol::Orchard,
chain_tip_height,
)?;
Ok(WalletMeta::new(

6
zcash_client_sqlite/src/wallet/common.rs
View File

@ -232,6 +232,7 @@ pub(crate) fn count_outputs(
min_value: NonNegativeAmount,
exclude: &[ReceivedNoteId],
protocol: ShieldedProtocol,
chain_tip_height: BlockHeight,
) -> Result<usize, rusqlite::Error> {
let (table_prefix, _, _) = per_protocol_names(protocol);
@ -265,13 +266,14 @@ pub(crate) fn count_outputs(
JOIN transactions stx ON stx.id_tx = transaction_id
WHERE stx.block IS NOT NULL -- the spending tx is mined
OR stx.expiry_height IS NULL -- the spending tx will not expire
OR stx.expiry_height > :anchor_height -- the spending tx is unexpired
OR stx.expiry_height > :chain_tip_height -- the spending tx is unexpired
)"
),
named_params![
":account_id": account.0,
":min_value": u64::from(min_value),
":exclude": &excluded_ptr
":exclude": &excluded_ptr,
":chain_tip_height": u32::from(chain_tip_height)
],
|row| row.get(0),
)