From f13ea7f01fa3d2fa642742e7b2bd8b338ac3e67c Mon Sep 17 00:00:00 2001 From: jwinterm Date: Wed, 10 Jun 2026 14:18:50 -0400 Subject: [PATCH] Contracts: make_outputs takes nanogrin amounts (Vec), not a string OutputSelectionArgs.make_outputs was a comma-separated grin string parsed inside libwallet via amount_from_hr_string. Move that parsing to the CLI boundary and carry explicit nanogrin u64 amounts through the API and Context instead: - OutputSelectionArgs.make_outputs: Option -> Option>; output_amounts() becomes a plain accessor, num_custom_outputs() uses len(). - The CLI (wallet_args) parses grin -> nanogrin per amount; ContractNewArgs and ContractSetupArgs carry Vec and copy it through. - Update the two tests that supplied make_outputs as a string. Addresses PR #754 review (types.rs, wallet_args.rs). --- controller/src/command.rs | 18 ++++------- .../tests/contract_self_spend_custom.rs | 9 +++++- libwallet/src/contract/selection.rs | 2 +- libwallet/src/contract/types.rs | 31 +++++-------------- src/cmd/wallet_args.rs | 18 ++++++----- 5 files changed, 33 insertions(+), 45 deletions(-) diff --git a/controller/src/command.rs b/controller/src/command.rs index 341f46b..8fb4541 100644 --- a/controller/src/command.rs +++ b/controller/src/command.rs @@ -1636,8 +1636,8 @@ pub struct ContractNewArgs { pub as_json: bool, /// Use the specified inputs (comma separated input commitments) pub use_inputs: Option, - /// How to separate outputs (command separated amounts) - pub make_outputs: Option, + /// Output amounts in nanogrin (one entry per output) + pub make_outputs: Option>, // Future features /// Custom fee contribution @@ -1679,10 +1679,7 @@ impl ContractNewArgs { Some(v) => Some(v.to_string()), None => None, }, - make_outputs: match self.make_outputs.as_ref() { - Some(v) => Some(v.to_string()), - None => None, - }, + make_outputs: self.make_outputs.clone(), ..Default::default() }, proof_args: Default::default(), @@ -1734,8 +1731,8 @@ pub struct ContractSetupArgs { pub as_json: bool, /// Use the specified inputs (comma separated input commitments) pub use_inputs: Option, - /// How to separate outputs (command separated amounts) - pub make_outputs: Option, + /// Output amounts in nanogrin (one entry per output) + pub make_outputs: Option>, // Future features /// Whether we should automatically sign a receive of any value @@ -1779,10 +1776,7 @@ impl ContractSetupArgs { Some(v) => Some(v.to_string()), None => None, }, - make_outputs: match self.make_outputs.as_ref() { - Some(v) => Some(v.to_string()), - None => None, - }, + make_outputs: self.make_outputs.clone(), ..Default::default() }, ..Default::default() diff --git a/controller/tests/contract_self_spend_custom.rs b/controller/tests/contract_self_spend_custom.rs index 6ab7637..a080089 100644 --- a/controller/tests/contract_self_spend_custom.rs +++ b/controller/tests/contract_self_spend_custom.rs @@ -58,7 +58,14 @@ fn contract_self_spend_custom_tx_impl(test_dir: &'static str) -> Result<(), libw let selection_args = OutputSelectionArgs { min_input_confirmation: 0, use_inputs: Some(use_inputs.clone()), // we will use two coinbase inputs - make_outputs: Some(String::from("88,35,3,0.2,15")), // the sum is such that it will need to pick another input making total of 3 inputs + // the sum is such that it will need to pick another input making total of 3 inputs + make_outputs: Some(vec![ + 88 * consensus::GRIN_BASE, + 35 * consensus::GRIN_BASE, + 3 * consensus::GRIN_BASE, + (0.2 * consensus::GRIN_BASE as f64) as u64, + 15 * consensus::GRIN_BASE, + ]), }; wallet::controller::owner_single_use(Some(send_wallet.clone()), send_mask, None, |api, m| { // Send wallet inititates a standard transaction with --send=0 diff --git a/libwallet/src/contract/selection.rs b/libwallet/src/contract/selection.rs index 02cf576..def393f 100644 --- a/libwallet/src/contract/selection.rs +++ b/libwallet/src/contract/selection.rs @@ -545,7 +545,7 @@ mod tests { net_change: Some(-2_000_000_000), selection_args: OutputSelectionArgs { // there is no abc5 input - make_outputs: Some(String::from("1,3")), + make_outputs: Some(vec![1_000_000_000, 3_000_000_000]), ..Default::default() }, ..Default::default() diff --git a/libwallet/src/contract/types.rs b/libwallet/src/contract/types.rs index 48c491f..b847a66 100644 --- a/libwallet/src/contract/types.rs +++ b/libwallet/src/contract/types.rs @@ -25,9 +25,9 @@ pub struct OutputSelectionArgs { pub min_input_confirmation: u64, /// Which inputs we want to use - default to payjoin if available with Some("any") pub use_inputs: Option, - /// Change output specification (comma separated amounts which don't include fee subtraction) - /// e.g. "3,1,4,0,0" describes 5 outputs two of which hold 0 value - pub make_outputs: Option, + /// Change output specification: explicit output amounts in nanogrin, not including + /// fee subtraction. e.g. [3, 1, 4, 0, 0] describes 5 outputs, two of which hold 0 value. + pub make_outputs: Option>, } impl OutputSelectionArgs { @@ -48,34 +48,19 @@ impl OutputSelectionArgs { None } } - /// Returns the outputs we have to create - // TODO: move consensus code outside of here. Consider turning make_outputs to Vec + /// 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> { - match self.make_outputs.as_ref() { - Some(s) => s - .split(",") - .map(|amt| { - // Exact decimal parsing (integer nanogrin), not f64. - crate::grin_core::core::amount_from_hr_string(amt).map_err(|e| { - Error::GenericError(format!("Invalid output amount '{}': {}", amt, e)) - }) - }) - .collect(), - None => Ok(vec![]), - } + Ok(self.make_outputs.clone().unwrap_or_default()) } /// Returns the sum of our output amounts pub fn sum_output_amounts(&self) -> Result { Ok(self.output_amounts()?.iter().sum()) } - /// Returns the number of custom outputs (count only; does not parse the amounts) + /// Returns the number of custom outputs pub fn num_custom_outputs(&self) -> usize { - self.make_outputs - .as_ref() - .map(|s| s.split(",").count()) - .unwrap_or(0) + self.make_outputs.as_ref().map(|v| v.len()).unwrap_or(0) } - } impl Default for OutputSelectionArgs { diff --git a/src/cmd/wallet_args.rs b/src/cmd/wallet_args.rs index 4f0e573..3634c40 100644 --- a/src/cmd/wallet_args.rs +++ b/src/cmd/wallet_args.rs @@ -1020,12 +1020,13 @@ pub fn parse_contract_new_args( }; let make_outputs = match args.value_of("make-outputs") { Some(v) => { - // Validate each comma-separated amount here so a bad value returns a - // ParseError instead of panicking later in output_amounts(). + // Parse each comma-separated grin amount into nanogrin at the CLI boundary, + // so the API and Context carry explicit u64 amounts. + let mut amounts = Vec::new(); for amt in v.split(',') { - parse_grin_amount(amt, "make-outputs")?; + amounts.push(parse_grin_amount(amt, "make-outputs")?); } - Some(String::from(v)) + Some(amounts) } None => None, }; @@ -1096,12 +1097,13 @@ pub fn parse_contract_setup_args( }; let make_outputs = match args.value_of("make-outputs") { Some(v) => { - // Validate each comma-separated amount here so a bad value returns a - // ParseError instead of panicking later in output_amounts(). + // Parse each comma-separated grin amount into nanogrin at the CLI boundary, + // so the API and Context carry explicit u64 amounts. + let mut amounts = Vec::new(); for amt in v.split(',') { - parse_grin_amount(amt, "make-outputs")?; + amounts.push(parse_grin_amount(amt, "make-outputs")?); } - Some(String::from(v)) + Some(amounts) } None => None, };