diff --git a/libwallet/src/api_impl/foreign.rs b/libwallet/src/api_impl/foreign.rs index c682390..7d299ab 100644 --- a/libwallet/src/api_impl/foreign.rs +++ b/libwallet/src/api_impl/foreign.rs @@ -303,14 +303,9 @@ where { let mut client = w.w2n_client().clone(); - let wd = match proof.witness_data.clone() { - Some(w) => w, - None => { - return Err(Error::PaymentProof( - "Cannot verify early payment proof with no witness data".to_string(), - )) - } - }; + let wd = proof.witness_data.as_ref().ok_or_else(|| { + Error::PaymentProof("Cannot verify early payment proof with no witness data".to_string()) + })?; let (retrieved_kernel, _) = match client.get_kernel(&wd.kernel_commitment, None, None) { Err(e) => { diff --git a/libwallet/src/api_impl/owner.rs b/libwallet/src/api_impl/owner.rs index 00e0e7a..a011ddd 100644 --- a/libwallet/src/api_impl/owner.rs +++ b/libwallet/src/api_impl/owner.rs @@ -571,51 +571,39 @@ where .as_ref() .and_then(|proof| proof.sender_public_nonce); - let (mut proof, sender_part_sig) = match tx.payment_proof { - Some(p) => { - if p.receiver_public_nonce.is_none() { - return Err(Error::PaymentProofRetrieval( - "Early payment proof requires stored receiver public nonce".into(), - )); - }; - if p.receiver_public_excess.is_none() { - return Err(Error::PaymentProofRetrieval( - "Early payment proof requires stored receiver public excess".into(), - )); - }; - if p.timestamp.is_none() { - return Err(Error::PaymentProofRetrieval( - "Early payment proof requires stored timestamp".into(), - )); - }; - if p.sender_part_sig.is_none() { - return Err(Error::PaymentProofRetrieval( - "Early payment proof requires stored sender partial signature".into(), - )); - }; - - ( - EarlyPaymentProof { - proof_type: PaymentProofType::try_from( - p.proof_type.unwrap_or(PaymentProofType::Invoice.as_u8()), - )?, - amount, - receiver_public_nonce: p.receiver_public_nonce.unwrap(), - receiver_public_excess: p.receiver_public_excess.unwrap(), - sender_address: p.sender_address, - timestamp: p.timestamp.unwrap().timestamp(), - memo: p.memo, - promise_signature: p.receiver_signature, - witness_data: None, - }, - p.sender_part_sig.unwrap(), - ) - } - None => { - return Err(Error::PaymentProofRetrieval( - "Transaction does not contain a payment proof".to_owned(), - )); - } + let p = tx.payment_proof.ok_or_else(|| { + Error::PaymentProofRetrieval("Transaction does not contain a payment proof".to_owned()) + })?; + let receiver_public_nonce = p.receiver_public_nonce.ok_or_else(|| { + Error::PaymentProofRetrieval( + "Early payment proof requires stored receiver public nonce".into(), + ) + })?; + let receiver_public_excess = p.receiver_public_excess.ok_or_else(|| { + Error::PaymentProofRetrieval( + "Early payment proof requires stored receiver public excess".into(), + ) + })?; + let timestamp = p.timestamp.ok_or_else(|| { + Error::PaymentProofRetrieval("Early payment proof requires stored timestamp".into()) + })?; + let sender_part_sig = p.sender_part_sig.ok_or_else(|| { + Error::PaymentProofRetrieval( + "Early payment proof requires stored sender partial signature".into(), + ) + })?; + let mut proof = EarlyPaymentProof { + proof_type: PaymentProofType::try_from( + p.proof_type.unwrap_or(PaymentProofType::Invoice.as_u8()), + )?, + amount, + receiver_public_nonce, + receiver_public_excess, + sender_address: p.sender_address, + timestamp: timestamp.timestamp(), + memo: p.memo, + promise_signature: p.receiver_signature, + witness_data: None, }; // Now to kernel lookup, to fill in the witness data diff --git a/libwallet/src/backend.rs b/libwallet/src/backend.rs index 9a1f958..3e8cbda 100644 --- a/libwallet/src/backend.rs +++ b/libwallet/src/backend.rs @@ -328,7 +328,7 @@ where /// Get an (Optional) tx log entry by parent key id and log id. pub fn get_tx_log_entry_by_id( &self, - parent_id: Identifier, + parent_id: &Identifier, log_id: u32, ) -> Result, Error> { let tx_log_key = to_key_u64(parent_id.to_bytes(), log_id as u64); diff --git a/libwallet/src/contract/actions/revoke.rs b/libwallet/src/contract/actions/revoke.rs index 15542ef..f8ba512 100644 --- a/libwallet/src/contract/actions/revoke.rs +++ b/libwallet/src/contract/actions/revoke.rs @@ -55,7 +55,7 @@ where // cannot be built until the input is unlocked. // FUTURE: we may want to boost fees if we notice the original tx in the mempool. let tx_id = args.tx_id; - let parent_key_id = utils::parent_key_for(w, args.src_acct_name.as_ref())?; + let parent_key_id = utils::parent_key_for(w, args.src_acct_name.as_deref())?; // Inputs we contributed to tx_id that are still recoverable. Locked => the original tx // is still active; Unspent => a previous revoke cancelled it but the self-spend did not @@ -75,7 +75,7 @@ where // it is already a *Cancelled type (and the inputs are Unspent), so we skip straight to // re-spending them. let revoked = w - .get_tx_log_entry_by_id(parent_key_id.clone(), tx_id)? + .get_tx_log_entry_by_id(&parent_key_id, tx_id)? .ok_or_else(|| Error::NotFoundErr(format!("Transaction {}", tx_id)))?; let revoked_slate_id = revoked.tx_slate_id; let needs_cancel = matches!( diff --git a/libwallet/src/contract/context.rs b/libwallet/src/contract/context.rs index b93ed8a..aedd337 100644 --- a/libwallet/src/contract/context.rs +++ b/libwallet/src/contract/context.rs @@ -46,7 +46,7 @@ where // Get data required for creating a context let height = w.w2n_client().get_chain_tip()?.0; let parent_key_id = - contract_utils::parent_key_for(w, setup_args.src_acct_name.as_ref())?; + contract_utils::parent_key_for(w, setup_args.src_acct_name.as_deref())?; self::create( w, keychain_mask, @@ -179,14 +179,14 @@ where ))); } // Add selected/created inputs/outputs to the context - add_inputs_to_ctx(context, &inputs)?; + add_inputs_to_ctx(context, &inputs); add_outputs_to_ctx(w, keychain_mask, context, my_output_amounts)?; Ok(()) } /// Add inputs to Context -fn add_inputs_to_ctx(context: &mut Context, inputs: &Vec) -> Result<(), Error> { +fn add_inputs_to_ctx(context: &mut Context, inputs: &[OutputData]) { debug!("contract::utils::add_inputs_to_ctx => adding inputs to context"); for input in inputs { context.add_input(&input.key_id, &input.mmr_index, input.value); @@ -195,8 +195,6 @@ fn add_inputs_to_ctx(context: &mut Context, inputs: &Vec) -> Result< &input.key_id, input.value ); } - - Ok(()) } /// Add outputs to Context diff --git a/libwallet/src/contract/selection.rs b/libwallet/src/contract/selection.rs index 02bc37d..61b84f7 100644 --- a/libwallet/src/contract/selection.rs +++ b/libwallet/src/contract/selection.rs @@ -71,10 +71,8 @@ where pub fn compute( setup_args: &ContractSetupArgsAPI, committed_fee: Option, - inputs: &mut Vec, -) -> Result<(Vec, Vec, FeeFields), Error> -where -{ + inputs: &mut [OutputData], +) -> Result<(Vec, Vec, FeeFields), Error> { let (inputs, fee) = select_inputs(setup_args, committed_fee, inputs)?; let input_sum = inputs .iter() @@ -88,10 +86,8 @@ where fn select_inputs( setup_args: &ContractSetupArgsAPI, committed_fee: Option, - inputs: &mut Vec, -) -> Result<(Vec, FeeFields), Error> -where -{ + inputs: &mut [OutputData], +) -> Result<(Vec, FeeFields), Error> { // We use 'lhs' and 'rhs' to denote the amounts on the left/right-hand side of the equation. // To simulate receive/payment value we: // - add positive net_change to the 'lhs' for the receiver (to simulate sender's input) @@ -142,15 +138,16 @@ where } // NOTE: that these are inputs that MUST be selected. We should lock the inputs if they're // required to minimize any potential race conditions. - let must_use_list = required_inputs.unwrap_or(vec![]); - if must_use_list.len() > 0 { + let must_use_list = required_inputs.unwrap_or_default(); + if !must_use_list.is_empty() { // Sort the inputs first by the ones listed in the use_inputs and then by value inputs.sort_by_key(|out| { ( // We have to negate the boolean to prioritize truthy values because // false is 0 and hence would be sorted before truthy entries - !(out.commit.is_some() - && must_use_list.contains(&&out.commit.as_ref().unwrap()[..])), + !out.commit + .as_deref() + .is_some_and(|commit| must_use_list.contains(&commit)), out.value, ) }); @@ -171,7 +168,7 @@ where // We want to count how many inputs we've picked _so far_. This is used to prevent picking // all 0*H +r*G outputs when we call with min_input_amount=0 and want just a payjoin. let mut n_inputs = 0; - let mut must_use_list_cnt: u32 = 0; + let mut must_use_list_cnt: usize = 0; // If we have already committed to a fee (context.fee) then set this as our "minimum" fee. The reason we have to // do this is to avoid solving the equation for less than the committed fee. We have to guarantee the inputs we take // are enough to cover the committed fee. At the end of selection, we check that the fees for the selection were not @@ -193,8 +190,10 @@ where let mut selected_inputs = Vec::new(); for out in inputs.iter() { // Take the commitment if it is listed as one of those we MUST take - let must_take = - out.commit.is_some() && must_use_list.contains(&&out.commit.as_ref().unwrap()[..]); + let must_take = out + .commit + .as_deref() + .is_some_and(|commit| must_use_list.contains(&commit)); // Compute the fee without this input let fee_without = fee_contribution( n_inputs, @@ -251,13 +250,15 @@ where } // Return an error if the fee computed is larger than the committed fee - if committed_fee.is_some() && my_fee.fee() > committed_fee.unwrap().fee() { - let msg = format!( - "Fee computed ({}) is larger than the committed fee ({}); cancel the transaction and retry", - my_fee.fee(), - committed_fee.unwrap().fee() - ); - return Err(Error::Fee(msg).into()); + if let Some(committed_fee) = committed_fee { + if my_fee.fee() > committed_fee.fee() { + let msg = format!( + "Fee computed ({}) is larger than the committed fee ({}); cancel the transaction and retry", + my_fee.fee(), + committed_fee.fee() + ); + return Err(Error::Fee(msg)); + } } // Check that the inputs we picked are enough to cover all our output amounts and fees @@ -276,19 +277,18 @@ where available_disp: amount_to_hr_string(total, false), needed, needed_disp: amount_to_hr_string(needed, false), - } - .into()); - // return Err(ErrorKind::GenericError(msg.into()).into()); + }); } // Assert that all the use_inputs have been selected - if must_use_list.len() != must_use_list_cnt as usize { - let msg = format!( - "We have not found all the inputs that have been requested. {}, found only: {}", - setup_args.selection_args.use_inputs.as_ref().unwrap(), - must_use_list_cnt - ); - return Err(Error::GenericError(msg.into()).into()); + if let Some(use_inputs) = &setup_args.selection_args.use_inputs { + if must_use_list.len() != must_use_list_cnt { + let msg = format!( + "We have not found all the inputs that have been requested. {}, found only: {}", + use_inputs, must_use_list_cnt + ); + return Err(Error::GenericError(msg)); + } } debug!( @@ -307,7 +307,7 @@ fn build_output_amount_list( let expected_net_change = setup_args.net_change.ok_or_else(|| { Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) })?; - let mut my_output_amounts = setup_args.selection_args.output_amounts()?; + let mut my_output_amounts = setup_args.selection_args.output_amounts(); let custom_outputs_sum = my_output_amounts .iter() .try_fold(0u64, |acc, v| acc.checked_add(*v)) diff --git a/libwallet/src/contract/types.rs b/libwallet/src/contract/types.rs index f2a39cb..9a15601 100644 --- a/libwallet/src/contract/types.rs +++ b/libwallet/src/contract/types.rs @@ -49,21 +49,14 @@ impl OutputSelectionArgs { } /// Return a list of commitments we must use pub fn required_inputs(&self) -> Option> { - if self.use_inputs.is_some() { - Some( - self.use_inputs.as_ref().unwrap()[..] - .split(",") - .filter(|x| *x != "any") - .collect(), - ) - } else { - None - } + self.use_inputs + .as_deref() + .map(|inputs| inputs.split(',').filter(|x| *x != "any").collect()) } /// Returns the output amounts (nanogrin) we have to create. Amounts arrive already /// parsed by the caller (e.g. the CLI), so this is just an accessor. - pub fn output_amounts(&self) -> Result, Error> { - Ok(self.make_outputs.clone().unwrap_or_default()) + pub fn output_amounts(&self) -> Vec { + self.make_outputs.clone().unwrap_or_default() } /// Returns the sum of our output amounts pub fn sum_output_amounts(&self) -> Result { diff --git a/libwallet/src/contract/utils.rs b/libwallet/src/contract/utils.rs index abb7ee4..c705586 100644 --- a/libwallet/src/contract/utils.rs +++ b/libwallet/src/contract/utils.rs @@ -33,7 +33,7 @@ pub fn create_tx_log_entry( net_change: i64, parent_key_id: Identifier, log_id: u32, -) -> Result { +) -> TxLogEntry { let log_type = if slate.num_participants == 1 { TxLogEntryType::TxSelfSpend } else { @@ -43,7 +43,7 @@ pub fn create_tx_log_entry( TxLogEntryType::TxSent } }; - let mut t = TxLogEntry::new(parent_key_id.clone(), log_type, log_id); + let mut t = TxLogEntry::new(parent_key_id, log_type, log_id); // stored_tx is set in save_step, once we have signed and the transaction is written. t.tx_slate_id = Some(slate.id); @@ -57,7 +57,7 @@ pub fn create_tx_log_entry( n => Some(n), }; - Ok(t) + t } /// Update TxLogEntry with data from the sign step @@ -158,16 +158,13 @@ pub fn get_net_change( // We have a context so we must have agreed on a certain net_change value in Context.net_change. // If we have both Context.net_change and setup_args.net_change, then they must be equal. let ctx_net_change = context.get_net_change()?; - match expected_net_change { - Some(args_net_change) => { - if ctx_net_change != args_net_change { - return Err(Error::GenericError(format!( - "Expected net change mismatch! Context.net_change: {}, setup_args.net_change: {}", - ctx_net_change, args_net_change - ))); - } + if let Some(args_net_change) = expected_net_change { + if ctx_net_change != args_net_change { + return Err(Error::GenericError(format!( + "Expected net change mismatch! Context.net_change: {}, setup_args.net_change: {}", + ctx_net_change, args_net_change + ))); } - None => (), } expected_net_change = Some(ctx_net_change); } else { @@ -176,18 +173,15 @@ pub fn get_net_change( // Fail if net_change was not passed to setup_args and was also not present in the context. // This means it has not been explicitly agreed on and we require the user to pass it. - if expected_net_change.is_none() { - return Err(Error::GenericError( - "You did not agree on the expected net difference.".into(), - ) - .into()); - } + let expected_net_change = expected_net_change.ok_or_else(|| { + Error::GenericError("You did not agree on the expected net difference.".into()) + })?; debug!( "contract::utils::get_net_change => expected_net_change: {}", - expected_net_change.unwrap() + expected_net_change ); - Ok(expected_net_change.unwrap()) + Ok(expected_net_change) } /// Lock inputs and store the Context, TxLogEntry and OutputData atomically @@ -221,15 +215,14 @@ where // We are at step2 if we don't have context.log_id and we have signed the slate let is_step2 = context.log_id.is_none() && is_signed; - let mut tx_log_entry = { - if context.log_id.is_none() { + let mut tx_log_entry = match context.log_id { + None => { // We create a new entry with log_id=0 and but replace it with the real id before committing - create_tx_log_entry(slate, context.get_net_change()?, parent_key_id.clone(), 0)? - } else { - let log_id = context.log_id.unwrap(); - w.get_tx_log_entry_by_id(parent_key_id.clone(), log_id)? - .ok_or_else(|| Error::NotFoundErr(format!("Transaction log entry {}", log_id)))? + create_tx_log_entry(slate, context.get_net_change()?, parent_key_id.clone(), 0) } + Some(log_id) => w + .get_tx_log_entry_by_id(parent_key_id, log_id)? + .ok_or_else(|| Error::NotFoundErr(format!("Transaction log entry {}", log_id)))?, }; // Update TxLogEntry if we have signed the contract (we have data about the kernel) @@ -280,11 +273,11 @@ where // Update TxLogEntry if context.log_id.is_none() { // If we just created the TxLogEntry, we have to assign it an id - let log_id = batch.next_tx_log_id(&parent_key_id)?; + let log_id = batch.next_tx_log_id(parent_key_id)?; tx_log_entry.id = log_id; context.log_id = Some(log_id); } - batch.save_tx_log_entry(tx_log_entry.clone(), &parent_key_id)?; + batch.save_tx_log_entry(tx_log_entry, parent_key_id)?; // Create OutputData entries and lock inputs if we added outputs at this step if step_added_outputs { // Create an OutputData entry for every created output @@ -493,16 +486,16 @@ pub fn verify_setup_args_consistency( /// active account. pub fn parent_key_for( w: &mut WalletBackend, - src_acct_name: Option<&String>, + src_acct_name: Option<&str>, ) -> Result where C: NodeClient, K: Keychain, { let parent_key_id = match src_acct_name { - Some(d) => match w.get_acct_path(d.clone())? { + Some(d) => match w.get_acct_path(d.to_owned())? { Some(p) => p.path, - None => return Err(Error::UnknownAccountLabel(d.clone())), + None => return Err(Error::UnknownAccountLabel(d.to_owned())), }, None => w.parent_key_id(), }; @@ -519,7 +512,7 @@ mod tests { for (net_change, credited, debited) in [(1, 1, 0), (0, 0, 0), (-1, 0, 1), (i64::MIN, 0, 1u64 << 63)] { - let entry = create_tx_log_entry(&slate, net_change, Identifier::zero(), 0).unwrap(); + let entry = create_tx_log_entry(&slate, net_change, Identifier::zero(), 0); assert_eq!(entry.amount_credited, credited); assert_eq!(entry.amount_debited, debited); } diff --git a/libwallet/src/payment_proof.rs b/libwallet/src/payment_proof.rs index ef6e26d..23c2ae8 100644 --- a/libwallet/src/payment_proof.rs +++ b/libwallet/src/payment_proof.rs @@ -328,11 +328,10 @@ impl EarlyPaymentProof { recipient_address: &DalekPublicKey, ) -> Result<(), Error> { check_proof_type(&self.proof_type)?; - if self.promise_signature.is_none() { - return Err(Error::PaymentProofValidation( - "Missing promise signature".into(), - )); - } + let promise_signature = self + .promise_signature + .as_ref() + .ok_or_else(|| Error::PaymentProofValidation("Missing promise signature".into()))?; // Rebuild message let mut sig_data_bin = Vec::new(); @@ -341,7 +340,7 @@ impl EarlyPaymentProof { })?; if recipient_address - .verify(&sig_data_bin, self.promise_signature.as_ref().unwrap()) + .verify(&sig_data_bin, promise_signature) .is_err() { return Err(Error::PaymentProof( @@ -359,13 +358,13 @@ impl EarlyPaymentProof { excess_sig: &Signature, msg: &Message, ) -> Result<(), Error> { - if self.witness_data.is_none() { - return Err(Error::PaymentProofValidation("Missing witness data".into())); - } + let wd = self + .witness_data + .as_ref() + .ok_or_else(|| Error::PaymentProofValidation("Missing witness data".into()))?; self.verify_promise_signature(recipient_address)?; - let wd = self.witness_data.as_ref().unwrap().clone(); { let static_secp = static_secp_instance(); let static_secp = static_secp.lock();