Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
zcash_client_backend: Move min_confirmations into Proposal
Browse files Browse the repository at this point in the history
This fixes an API issue whereby it was possible to execute a `Proposal`
with a different value of `min_confirmations` than that with which the
`Proposal` was constructed.
nuttycom committed Oct 25, 2023
1 parent 25b74a2 commit c2bb2df
Showing 5 changed files with 68 additions and 81 deletions.
8 changes: 6 additions & 2 deletions zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -15,6 +15,8 @@ and this library adheres to Rust's notion of
- `zcash_client_backend::data_api::error::Error` has new error variant:
- `Error::UnsupportedPoolType(zcash_client_backend::data_api::PoolType)`
- Added module `zcash_client_backend::fees::standard`
- Added methods to `zcash_client_backend::wallet::input_selection::Proposal`:
`{from_parts, min_confirmations}`
- Added methods to `zcash_client_backend::wallet::ReceivedSaplingNote`:
`{from_parts, txid, output_index, diversifier, rseed, note_commitment_tree_position}`.

@@ -45,10 +47,12 @@ and this library adheres to Rust's notion of
`change_memo` argument; instead, change memos are represented in the
individual values of the `proposed_change` field of the `Proposal`'s
`TransactionBalance`.
- `wallet::create_proposed_transaction` now takes its `proposal` argument
by reference instead of as an owned value.
- `wallet::create_proposed_transaction` no longer takes a `min_confirmations`
argument. Instead, `min_confirmations` is stored in the `Proposal`
- `wallet::create_spend_to_address` now takes an additional
`change_memo` argument.
- `wallet::create_proposed_transaction` now takes its `proposal` argument
by reference instead of as an owned value.
- `zcash_client_backend::fees::ChangeValue::Sapling` is now a structured variant.
In addition to the existing change value, it now also carries an optional memo
to be associated with the change output.
33 changes: 4 additions & 29 deletions zcash_client_backend/src/data_api/wallet.rs
Original file line number Diff line number Diff line change
@@ -231,15 +231,7 @@ where
change_memo,
)?;

create_proposed_transaction(
wallet_db,
params,
prover,
usk,
ovk_policy,
&proposal,
min_confirmations,
)
create_proposed_transaction(wallet_db, params, prover, usk, ovk_policy, &proposal)
}

/// Constructs a transaction that sends funds as specified by the `request` argument
@@ -332,15 +324,7 @@ where
min_confirmations,
)?;

create_proposed_transaction(
wallet_db,
params,
prover,
usk,
ovk_policy,
&proposal,
min_confirmations,
)
create_proposed_transaction(wallet_db, params, prover, usk, ovk_policy, &proposal)
}

/// Select transaction inputs, compute fees, and construct a proposal for a transaction
@@ -495,7 +479,6 @@ pub fn create_proposed_transaction<DbT, ParamsT, InputsErrT, FeeRuleT>(
usk: &UnifiedSpendingKey,
ovk_policy: OvkPolicy,
proposal: &Proposal<FeeRuleT, DbT::NoteRef>,
min_confirmations: NonZeroU32,
) -> Result<
TxId,
Error<
@@ -546,7 +529,7 @@ where
// are no possible transparent inputs, so we ignore those
let mut builder = Builder::new(params.clone(), proposal.min_target_height(), None);

let checkpoint_depth = wallet_db.get_checkpoint_depth(min_confirmations)?;
let checkpoint_depth = wallet_db.get_checkpoint_depth(proposal.min_confirmations())?;
wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _>>(|sapling_tree| {
for selected in proposal.sapling_inputs() {
let (note, key, merkle_path) = select_key_for_note(
@@ -815,15 +798,7 @@ where
min_confirmations,
)?;

create_proposed_transaction(
wallet_db,
params,
prover,
usk,
OvkPolicy::Sender,
&proposal,
min_confirmations,
)
create_proposed_transaction(wallet_db, params, prover, usk, OvkPolicy::Sender, &proposal)
}

#[allow(clippy::type_complexity)]
33 changes: 33 additions & 0 deletions zcash_client_backend/src/data_api/wallet/input_selection.rs
Original file line number Diff line number Diff line change
@@ -81,12 +81,39 @@ pub struct Proposal<FeeRuleT, NoteRef> {
sapling_inputs: Vec<ReceivedSaplingNote<NoteRef>>,
balance: TransactionBalance,
fee_rule: FeeRuleT,
min_confirmations: NonZeroU32,
min_target_height: BlockHeight,
min_anchor_height: BlockHeight,
is_shielding: bool,
}

impl<FeeRuleT, NoteRef> Proposal<FeeRuleT, NoteRef> {
/// Constructs a [`Proposal`] from its constituent parts.
#[allow(clippy::too_many_arguments)]
pub fn from_parts(
transaction_request: TransactionRequest,
transparent_inputs: Vec<WalletTransparentOutput>,
sapling_inputs: Vec<ReceivedSaplingNote<NoteRef>>,
balance: TransactionBalance,
fee_rule: FeeRuleT,
min_confirmations: NonZeroU32,
min_target_height: BlockHeight,
min_anchor_height: BlockHeight,
is_shielding: bool,
) -> Self {
Self {
transaction_request,
transparent_inputs,
sapling_inputs,
balance,
fee_rule,
min_confirmations,
min_target_height,
min_anchor_height,
is_shielding,
}
}

/// Returns the transaction request that describes the payments to be made.
pub fn transaction_request(&self) -> &TransactionRequest {
&self.transaction_request
@@ -107,6 +134,10 @@ impl<FeeRuleT, NoteRef> Proposal<FeeRuleT, NoteRef> {
pub fn fee_rule(&self) -> &FeeRuleT {
&self.fee_rule
}
/// Returns the value of `min_confirmations` used in input selection.
pub fn min_confirmations(&self) -> NonZeroU32 {
self.min_confirmations
}
/// Returns the target height for which the proposal was prepared.
///
/// The chain must contain at least this many blocks in order for the proposal to
@@ -399,6 +430,7 @@ where
sapling_inputs,
balance,
fee_rule: (*self.change_strategy.fee_rule()).clone(),
min_confirmations,
min_target_height: target_height,
min_anchor_height: anchor_height,
is_shielding: false,
@@ -505,6 +537,7 @@ where
sapling_inputs: vec![],
balance,
fee_rule: (*self.change_strategy.fee_rule()).clone(),
min_confirmations,
min_target_height: target_height,
min_anchor_height: latest_anchor,
is_shielding: true,
6 changes: 2 additions & 4 deletions zcash_client_sqlite/src/testing.rs
Original file line number Diff line number Diff line change
@@ -602,8 +602,7 @@ impl<Cache> TestState<Cache> {
&mut self,
usk: &UnifiedSpendingKey,
ovk_policy: OvkPolicy,
proposal: Proposal<FeeRuleT, ReceivedNoteId>,
min_confirmations: NonZeroU32,
proposal: &Proposal<FeeRuleT, ReceivedNoteId>,
) -> Result<
TxId,
data_api::error::Error<
@@ -617,14 +616,13 @@ impl<Cache> TestState<Cache> {
FeeRuleT: FeeRule,
{
let params = self.network();
create_proposed_transaction(
create_proposed_transaction::<_, _, InputsErrT, _>(
&mut self.db_data,
&params,
test_prover(),
usk,
ovk_policy,
&proposal,
min_confirmations,
)
}

69 changes: 23 additions & 46 deletions zcash_client_sqlite/src/wallet/sapling.rs
Original file line number Diff line number Diff line change
@@ -546,26 +546,28 @@ pub(crate) mod tests {
}])
.unwrap();

// TODO: This test was originally written to use the pre-zip-313 fee rule
// and has not yet been updated.
#[allow(deprecated)]
let fee_rule = StandardFeeRule::PreZip313;

let change_memo = "Test change memo".parse::<Memo>().unwrap();
let change_strategy =
standard::SingleOutputChangeStrategy::new(fee_rule, Some(change_memo.clone().into()));
let input_selector =
&GreedyInputSelector::new(change_strategy, DustOutputPolicy::default());
let proposal_result = st.propose_transfer(
account,
input_selector,
request,
NonZeroU32::new(1).unwrap(),
);
assert_matches!(proposal_result, Ok(_));

let create_proposed_result = st.create_proposed_transaction::<Infallible, _>(
&usk,
OvkPolicy::Sender,
proposal_result.unwrap(),
NonZeroU32::new(1).unwrap(),
);
let proposal = st
.propose_transfer(
account,
input_selector,
request,
NonZeroU32::new(1).unwrap(),
)
.unwrap();

let create_proposed_result =
st.create_proposed_transaction::<Infallible, _>(&usk, OvkPolicy::Sender, &proposal);
assert_matches!(create_proposed_result, Ok(_));

let sent_tx_id = create_proposed_result.unwrap();
@@ -682,7 +684,8 @@ pub(crate) mod tests {
}

#[test]
fn create_to_address_fails_with_no_blocks() {
#[allow(deprecated)]
fn proposal_fails_with_no_blocks() {
let mut st = TestBuilder::new()
.with_test_account(AccountBirthday::from_sapling_activation)
.build();
@@ -839,12 +842,7 @@ pub(crate) mod tests {

// Executing the proposal should succeed
let txid = st
.create_proposed_transaction::<Infallible, _>(
&usk,
OvkPolicy::Sender,
proposal,
min_confirmations,
)
.create_proposed_transaction::<Infallible, _>(&usk, OvkPolicy::Sender, &proposal)
.unwrap();

let (h, _) = st.generate_next_block_including(txid);
@@ -901,12 +899,7 @@ pub(crate) mod tests {

// Executing the proposal should succeed
assert_matches!(
st.create_proposed_transaction::<Infallible, _>(
&usk,
OvkPolicy::Sender,
proposal,
min_confirmations
),
st.create_proposed_transaction::<Infallible, _>(&usk, OvkPolicy::Sender, &proposal,),
Ok(_)
);

@@ -985,12 +978,7 @@ pub(crate) mod tests {
.unwrap();

let txid2 = st
.create_proposed_transaction::<Infallible, _>(
&usk,
OvkPolicy::Sender,
proposal,
min_confirmations,
)
.create_proposed_transaction::<Infallible, _>(&usk, OvkPolicy::Sender, &proposal)
.unwrap();

let (h, _) = st.generate_next_block_including(txid2);
@@ -1056,8 +1044,7 @@ pub(crate) mod tests {
)?;

// Executing the proposal should succeed
let txid =
st.create_proposed_transaction(&usk, ovk_policy, proposal, min_confirmations)?;
let txid = st.create_proposed_transaction(&usk, ovk_policy, &proposal)?;

// Fetch the transaction from the database
let raw_tx: Vec<_> = st
@@ -1156,12 +1143,7 @@ pub(crate) mod tests {

// Executing the proposal should succeed
assert_matches!(
st.create_proposed_transaction::<Infallible, _>(
&usk,
OvkPolicy::Sender,
proposal,
min_confirmations
),
st.create_proposed_transaction::<Infallible, _>(&usk, OvkPolicy::Sender, &proposal,),
Ok(_)
);
}
@@ -1214,12 +1196,7 @@ pub(crate) mod tests {

// Executing the proposal should succeed
assert_matches!(
st.create_proposed_transaction::<Infallible, _>(
&usk,
OvkPolicy::Sender,
proposal,
min_confirmations
),
st.create_proposed_transaction::<Infallible, _>(&usk, OvkPolicy::Sender, &proposal,),
Ok(_)
);
}

0 comments on commit c2bb2df

Please sign in to comment.