From 2ca3637a27f177ecf265ed735f635a06d0b9e64e Mon Sep 17 00:00:00 2001 From: wiesche Date: Sun, 13 Sep 2026 08:20:22 +0200 Subject: [PATCH] clean up contracts --- controller/src/display.rs | 52 ++++++++++++------------- libwallet/src/api_impl/foreign.rs | 10 ++--- libwallet/src/contract/actions/new.rs | 4 +- libwallet/src/contract/actions/setup.rs | 6 +-- libwallet/src/contract/context.rs | 6 +-- libwallet/src/contract/selection.rs | 8 +--- libwallet/src/contract/slate.rs | 14 +++---- libwallet/src/contract/types.rs | 39 ++++++++++++------- libwallet/src/contract/utils.rs | 7 ++-- libwallet/src/payment_proof.rs | 8 ++-- libwallet/src/slate_versions/mod.rs | 2 +- libwallet/src/types.rs | 16 ++++---- 12 files changed, 85 insertions(+), 87 deletions(-) diff --git a/controller/src/display.rs b/controller/src/display.rs index d800bf7..deca5a3 100644 --- a/controller/src/display.rs +++ b/controller/src/display.rs @@ -20,6 +20,7 @@ use crate::libwallet::{ AcctPathMapping, Error, OutputCommitMapping, OutputStatus, Slate, TxLogEntry, ViewWallet, WalletInfo, }; +use crate::util::secp::pedersen::Commitment; use crate::util::ToHex; use grin_wallet_util::OnionV3Address; use prettytable; @@ -673,12 +674,12 @@ fn contract_view_table( table.add_row(row![bFC->"Transfer Amount", bGC->amount_to_hr_string(slate.amount, false)]); let suggested = view .suggested_net_change - .map(|change| format_net_change(Some(change))) + .map(format_net_change) .unwrap_or_else(|| "Not applicable".to_string()); table.add_row(row![bFC->"Expected Amount Change (Before Fee)", bGC->suggested]); let agreed = view .agreed_net_change - .map(|change| format_net_change(Some(change))) + .map(format_net_change) .unwrap_or_else(|| "Not agreed yet".to_string()); table.add_row(row![bFC->"Agreed Amount Change (Before Fee)", bGC->agreed]); table.add_row( @@ -689,10 +690,10 @@ fn contract_view_table( .map(|fee| amount_to_hr_string(fee, false)) .unwrap_or_else(|| "Not known before signing".to_string()); table.add_row(row![bFC->"Your Fee", bGC->own_fee]); - let balance_change = match view.balance_change { - Some(change) => format_net_change(Some(change)), - None => "Not known before signing".to_string(), - }; + let balance_change = view + .balance_change + .map(format_net_change) + .unwrap_or_else(|| "Not known before signing".to_string()); table.add_row(row![bFC->"Your Balance Change", bGC->balance_change]); table.add_row(row![bFC->"Confirmed", bGC->yes_no(view.is_executed)]); let (unexpected, warning) = match view.own_commitment_status { @@ -720,22 +721,14 @@ fn contract_view_table( } Inputs::FeaturesAndCommit(inputs) => { for (index, input) in inputs.iter().enumerate() { - let commitment = input.commitment().as_ref().to_hex(); - let value = match input.features { - OutputFeatures::Plain => commitment, - OutputFeatures::Coinbase => format!("Coinbase {}", commitment), - }; + let value = format_commitment(input.commitment(), input.features); table.add_row(row![bFC->format!("Input {}", index + 1), bGC->value]); } } } table.add_row(row![bFC->"Outputs", bGC->tx.outputs().len()]); for (index, output) in tx.outputs().iter().enumerate() { - let commitment = output.commitment().as_ref().to_hex(); - let value = match output.features() { - OutputFeatures::Plain => commitment, - OutputFeatures::Coinbase => format!("Coinbase {}", commitment), - }; + let value = format_commitment(output.commitment(), output.features()); table.add_row(row![bFC->format!("Output {}", index + 1), bGC->value]); } } else { @@ -755,17 +748,22 @@ fn yes_no(value: bool) -> &'static str { } } -fn format_net_change(change: Option) -> String { - match change { - Some(value) => format!( - "{}{}", - if value < 0 { "-" } else { "+" }, - amount_to_hr_string(value.unsigned_abs(), false) - ), - None => String::from("None"), +fn format_commitment(commitment: Commitment, features: OutputFeatures) -> String { + let commitment = commitment.as_ref().to_hex(); + match features { + OutputFeatures::Plain => commitment, + OutputFeatures::Coinbase => format!("Coinbase {}", commitment), } } +fn format_net_change(change: i64) -> String { + format!( + "{}{}", + if change < 0 { "-" } else { "+" }, + amount_to_hr_string(change.unsigned_abs(), false) + ) +} + #[cfg(test)] mod tests { use super::{contract_view_table, format_net_change}; @@ -791,9 +789,9 @@ mod tests { #[test] fn net_change_sign() { - assert_eq!(format_net_change(Some(1_000_000_000)), "+1.000000000"); - assert_eq!(format_net_change(Some(-1_000_000_000)), "-1.000000000"); - assert_eq!(format_net_change(None), "None"); + assert_eq!(format_net_change(1_000_000_000), "+1.000000000"); + assert_eq!(format_net_change(-1_000_000_000), "-1.000000000"); + assert_eq!(format_net_change(0), "+0.000000000"); } #[test] diff --git a/libwallet/src/api_impl/foreign.rs b/libwallet/src/api_impl/foreign.rs index 53ff439..e629b1a 100644 --- a/libwallet/src/api_impl/foreign.rs +++ b/libwallet/src/api_impl/foreign.rs @@ -247,10 +247,8 @@ where } // The foreign API only handles the receiving side -fn check_receiving_contract(net_change: Option, action: &str) -> Result<(), Error> { - let net_change = net_change.ok_or_else(|| { - Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) - })?; +fn check_receiving_contract(args: &ContractSetupArgsAPI, action: &str) -> Result<(), Error> { + let net_change = args.required_net_change()?; if net_change <= 0 { return Err(Error::GenericError(format!( "Can't {} a non-receiving contract from a foreign API.", @@ -270,7 +268,7 @@ where C: NodeClient, K: Keychain, { - check_receiving_contract(args.setup_args.net_change, "create")?; + check_receiving_contract(&args.setup_args, "create")?; owner_contract_new(w, keychain_mask, args) } @@ -285,7 +283,7 @@ where C: NodeClient, K: Keychain, { - check_receiving_contract(args.net_change, "sign")?; + check_receiving_contract(args, "sign")?; owner_contract_sign(w, keychain_mask, args, slate) } diff --git a/libwallet/src/contract/actions/new.rs b/libwallet/src/contract/actions/new.rs index b0e5a4a..60f80fa 100644 --- a/libwallet/src/contract/actions/new.rs +++ b/libwallet/src/contract/actions/new.rs @@ -62,9 +62,7 @@ where C: NodeClient, K: Keychain, { - let net_change = setup_args.net_change.ok_or_else(|| { - Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) - })?; + let net_change = setup_args.required_net_change()?; debug!("contract::new => net_change passed: {}", net_change); // Initialize a new contract (if net_change is positive, I'm the receiver meaning this is invoice flow) diff --git a/libwallet/src/contract/actions/setup.rs b/libwallet/src/contract/actions/setup.rs index e18f225..cc86e8f 100644 --- a/libwallet/src/contract/actions/setup.rs +++ b/libwallet/src/contract/actions/setup.rs @@ -115,12 +115,12 @@ where } contract::proofs::commit_sender_nonce(&sl, &mut context, keychain.secp())?; contract::slate::add_keys(&mut sl, &keychain, &mut context)?; - contract::slate::add_payment_proof( + contract::slate::maybe_add_payment_proof( w, &mut sl, keychain_mask, - &mut context, - &setup_args.net_change, + &context, + setup_args.net_change, &setup_args.proof_args, )?; // noop for the sender diff --git a/libwallet/src/contract/context.rs b/libwallet/src/contract/context.rs index aedd337..bf83930 100644 --- a/libwallet/src/contract/context.rs +++ b/libwallet/src/contract/context.rs @@ -85,9 +85,7 @@ where updater::refresh_outputs(w, keychain_mask, parent_key_id, false)?; // Fee contribution estimation - let net_change = setup_args.net_change.ok_or_else(|| { - Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) - })?; + let net_change = setup_args.required_net_change()?; // select inputs to estimate fee cost let (inputs, _, my_fee) = prepare_outputs(w, &parent_key_id, current_height, &setup_args, None)?; @@ -98,7 +96,7 @@ where ); // Make sure `my_fee < net_change` holds for the receiver. This can't be true for a self-spend, because nobody // has a net_change > 0 which makes a self-spend ok to be a net negative when fees are included. - if net_change > 0 && my_fee.fee() > net_change.abs() as u64 { + if net_change > 0 && my_fee.fee() > net_change as u64 { return Err(Error::GenericError(format!( "My contribution as a receiver would be net negative. my_fee: {}, net_change: {}", my_fee.fee(), diff --git a/libwallet/src/contract/selection.rs b/libwallet/src/contract/selection.rs index 61b84f7..a44ff9d 100644 --- a/libwallet/src/contract/selection.rs +++ b/libwallet/src/contract/selection.rs @@ -95,9 +95,7 @@ fn select_inputs( // For either party, the following MUST hold for the inputs the function returns: // Σmy_inputs >= Σmy_outputs + my_fee_cost // Each party later balances the equation by adding an additional output (change output or receiver output) - let net_change = setup_args.net_change.ok_or_else(|| { - Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) - })?; + let net_change = setup_args.required_net_change()?; let custom_outputs_amount_sum = setup_args.selection_args.sum_output_amounts()?; // unsigned_abs, as abs() panics on i64::MIN let pay_amount = if net_change < 0 { @@ -304,9 +302,7 @@ fn build_output_amount_list( my_fee_cost: u64, setup_args: &ContractSetupArgsAPI, ) -> Result, Error> { - let expected_net_change = setup_args.net_change.ok_or_else(|| { - Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) - })?; + let expected_net_change = setup_args.required_net_change()?; let mut my_output_amounts = setup_args.selection_args.output_amounts(); let custom_outputs_sum = my_output_amounts .iter() diff --git a/libwallet/src/contract/slate.rs b/libwallet/src/contract/slate.rs index ca1108c..f9d047b 100644 --- a/libwallet/src/contract/slate.rs +++ b/libwallet/src/contract/slate.rs @@ -32,26 +32,26 @@ use std::collections::BTreeSet; use super::types::{OwnCommitmentStatus, ProofArgs}; /// Add payment proof data to slate, noop for sender -pub fn add_payment_proof( +pub fn maybe_add_payment_proof( w: &mut WalletBackend, slate: &mut Slate, keychain_mask: Option<&SecretKey>, context: &Context, - net_change: &Option, + net_change: Option, proof_args: &ProofArgs, ) -> Result<(), Error> where C: NodeClient, K: Keychain, { - debug!("contract::slate::add_payment_proof => called"); + debug!("contract::slate::maybe_add_payment_proof => called"); if !proof_args.suppress_proof { crate::payment_proof::check_proof_type(&proof_args.proof_type)?; } - // If we're a recipient, generate proof unless explicity told not to - if let Some(ref c) = net_change { - if *c > 0 && !proof_args.suppress_proof && slate.payment_proof.is_none() { - super::proofs::add_payment_proof(w, keychain_mask, slate, &context, proof_args)?; + // If we're a recipient, generate proof unless explicitly told not to + if let Some(change) = net_change { + if change > 0 && !proof_args.suppress_proof && slate.payment_proof.is_none() { + super::proofs::add_payment_proof(w, keychain_mask, slate, context, proof_args)?; } } diff --git a/libwallet/src/contract/types.rs b/libwallet/src/contract/types.rs index 9a15601..f48f574 100644 --- a/libwallet/src/contract/types.rs +++ b/libwallet/src/contract/types.rs @@ -136,6 +136,15 @@ pub struct ContractSetupArgsAPI { pub proof_args: ProofArgs, } +impl ContractSetupArgsAPI { + /// Return the net change or fail if it is missing + pub fn required_net_change(&self) -> Result { + self.net_change.ok_or_else(|| { + Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) + }) + } +} + impl Default for ContractSetupArgsAPI { fn default() -> ContractSetupArgsAPI { ContractSetupArgsAPI { @@ -144,16 +153,14 @@ impl Default for ContractSetupArgsAPI { num_participants: 2, fee_rate: None, add_outputs: false, - selection_args: OutputSelectionArgs { - ..Default::default() - }, + selection_args: OutputSelectionArgs::default(), proof_args: ProofArgs::default(), } } } /// Contract New -#[derive(Clone, Serialize, Deserialize)] +#[derive(Clone, Default, Serialize, Deserialize)] pub struct ContractNewArgsAPI { /// Setup args - contract new also initiates the setup by default pub setup_args: ContractSetupArgsAPI, @@ -163,15 +170,6 @@ pub struct ContractNewArgsAPI { pub ttl_blocks: Option, } -impl Default for ContractNewArgsAPI { - fn default() -> ContractNewArgsAPI { - ContractNewArgsAPI { - ttl_blocks: None, - setup_args: ContractSetupArgsAPI::default(), - } - } -} - /// ContractView #[derive(Clone, Serialize, Deserialize)] pub struct ContractView { @@ -185,7 +183,7 @@ pub struct ContractView { pub own_fee: Option, /// Agreed balance change after this wallet's fee, or None while the fee is unknown pub balance_change: Option, - /// Number of singatures on the contract + /// Number of signatures on the contract pub num_sigs: u8, /// Has the contract been executed on chain pub is_executed: bool, @@ -242,6 +240,19 @@ pub struct ContractRevokeArgsAPI { mod tests { use super::*; + #[test] + fn required_net_change() { + let mut args = ContractSetupArgsAPI::default(); + assert_eq!( + args.required_net_change().unwrap_err(), + Error::GenericError("Contract requires a net change (--send or --receive)".into()) + ); + for change in [i64::MIN, -1, 0, 1, i64::MAX] { + args.net_change = Some(change); + assert_eq!(args.required_net_change().unwrap(), change); + } + } + #[test] fn proof_args_json() { for (proof_type, value) in [ diff --git a/libwallet/src/contract/utils.rs b/libwallet/src/contract/utils.rs index c705586..4c7d1ba 100644 --- a/libwallet/src/contract/utils.rs +++ b/libwallet/src/contract/utils.rs @@ -154,7 +154,7 @@ pub fn get_net_change( ) -> Result { let mut expected_net_change: Option = setup_args_net_change; if let Some(context) = context { - debug!("contract::sign => context found"); + debug!("contract::utils::get_net_change => context found"); // 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()?; @@ -296,11 +296,10 @@ where // Update context if is_signed && !is_step2 { - // NOTE: We MUST forget the context when we sign. Ideally, these two would be atomic or perhaps - // when we call slate::sigadd_partial_signaturen we could swap the secret key with a temporary one just to be safe. - // Keep the step2 context for contract view + // Forget the signing context in the same batch as the transaction log update batch.delete_private_context(slate.id.as_bytes())?; } else { + // Keep unsigned contexts for retries and the step2 context for contract view batch.save_private_context(slate.id.as_bytes(), &context)?; } diff --git a/libwallet/src/payment_proof.rs b/libwallet/src/payment_proof.rs index 23c2ae8..1fb75dc 100644 --- a/libwallet/src/payment_proof.rs +++ b/libwallet/src/payment_proof.rs @@ -25,7 +25,7 @@ use crate::grin_util::secp::Secp256k1; use crate::grin_util::secp::Signature; use crate::grin_util::static_secp_instance; use crate::slate::{PaymentMemo, PaymentProofType, Slate}; -use crate::slate_versions::ser as dalek_ser; +use crate::slate_versions::ser::{dalek_pubkey_serde, option_dalek_sig_serde, option_pubkey_serde}; use crate::Error; use ed25519_dalek::Signature as DalekSignature; use ed25519_dalek::SigningKey as DalekSecretKey; @@ -117,7 +117,7 @@ pub struct ProofWitness { /// Untweaked sender nonce used by sender-nonce proofs #[serde( default, - with = "dalek_ser::option_pubkey_serde", + with = "option_pubkey_serde", skip_serializing_if = "Option::is_none" )] pub sender_public_nonce: Option, @@ -138,7 +138,7 @@ pub struct EarlyPaymentProof { #[serde(with = "secp_ser::pubkey_serde")] pub receiver_public_excess: PublicKey, /// Sender's address - #[serde(with = "dalek_ser::dalek_pubkey_serde")] + #[serde(with = "dalek_pubkey_serde")] pub sender_address: DalekPublicKey, /// Timestamp provided by recipient when signing pub timestamp: i64, @@ -146,7 +146,7 @@ pub struct EarlyPaymentProof { #[serde(skip_serializing_if = "Option::is_none")] pub memo: Option, /// Not serialized in binary format - #[serde(with = "dalek_ser::option_dalek_sig_serde")] + #[serde(with = "option_dalek_sig_serde")] pub promise_signature: Option, /// Not serialized in binary format, just a convenient place to insert /// the witness kernel commitment index diff --git a/libwallet/src/slate_versions/mod.rs b/libwallet/src/slate_versions/mod.rs index 657c865..d7df717 100644 --- a/libwallet/src/slate_versions/mod.rs +++ b/libwallet/src/slate_versions/mod.rs @@ -74,7 +74,7 @@ impl TryFrom for SlateVersion { impl SlateVersion { /// Use V5 only when the slate has fields that V4 cannot store /// V4 does not support the proof type, timestamp or memo - pub fn lowest_for(slate: &Slate) -> SlateVersion { + fn lowest_for(slate: &Slate) -> SlateVersion { match &slate.payment_proof { Some(p) if p.requires_v5() => SlateVersion::V5, _ => SlateVersion::V4, diff --git a/libwallet/src/types.rs b/libwallet/src/types.rs index f9dd24a..1f4059e 100644 --- a/libwallet/src/types.rs +++ b/libwallet/src/types.rs @@ -29,7 +29,7 @@ use crate::grin_util::secp::key::{PublicKey, SecretKey}; use crate::grin_util::secp::{pedersen, Secp256k1, Signature}; use crate::grin_util::{ToHex, ZeroingString}; use crate::slate::PaymentMemo; -use crate::slate_versions::ser as dalek_ser; +use crate::slate_versions::ser::{dalek_pubkey_serde, option_dalek_sig_serde, option_pubkey_serde}; use crate::{InitTxArgs, SlateState, WalletBackend}; use chrono::prelude::*; use ed25519_dalek::Signature as DalekSignature; @@ -776,18 +776,18 @@ impl TxLogEntry { #[derive(Serialize, Deserialize, Debug, Clone)] pub struct StoredProofInfo { /// receiver address - #[serde(with = "dalek_ser::dalek_pubkey_serde")] + #[serde(with = "dalek_pubkey_serde")] pub receiver_address: DalekPublicKey, - #[serde(with = "dalek_ser::option_dalek_sig_serde")] + #[serde(with = "option_dalek_sig_serde")] /// Receiver signature for legacy proofs, or promise signature for early proofs pub receiver_signature: Option, /// sender address derivation path index pub sender_address_path: u32, /// sender address - #[serde(with = "dalek_ser::dalek_pubkey_serde")] + #[serde(with = "dalek_pubkey_serde")] pub sender_address: DalekPublicKey, /// Legacy sender signature - #[serde(with = "dalek_ser::option_dalek_sig_serde")] + #[serde(with = "option_dalek_sig_serde")] pub sender_signature: Option, // Fields beyond here are specific to early payment proofs /// Assumed to be 0x00 (Legacy) if missing @@ -796,14 +796,14 @@ pub struct StoredProofInfo { /// receiver's public nonce from signing #[serde( default, - with = "dalek_ser::option_pubkey_serde", + with = "option_pubkey_serde", skip_serializing_if = "Option::is_none" )] pub receiver_public_nonce: Option, /// receiver's public excess from signing #[serde( default, - with = "dalek_ser::option_pubkey_serde", + with = "option_pubkey_serde", skip_serializing_if = "Option::is_none" )] pub receiver_public_excess: Option, @@ -823,7 +823,7 @@ pub struct StoredProofInfo { /// Untweaked sender public nonce for a sender-nonce proof #[serde( default, - with = "dalek_ser::option_pubkey_serde", + with = "option_pubkey_serde", skip_serializing_if = "Option::is_none" )] pub sender_public_nonce: Option,