diff --git a/cmd/soroban-cli/src/assembled.rs b/cmd/soroban-cli/src/assembled.rs index 06d4e48ff..1a3fa7da7 100644 --- a/cmd/soroban-cli/src/assembled.rs +++ b/cmd/soroban-cli/src/assembled.rs @@ -11,13 +11,11 @@ use soroban_rpc::{ Error, LogEvents, LogResources, ResourceConfig, RestorePreamble, SimulateTransactionResponse, }; -pub(crate) const DEFAULT_TRANSACTION_FEES: u32 = 100; - pub async fn simulate_and_assemble_transaction( client: &soroban_rpc::Client, tx: &Transaction, resource_config: Option, - resource_fee: Option, + resource_fee: Option, ) -> Result { let envelope = TransactionEnvelope::Tx(TransactionV1Envelope { tx: tx.clone(), @@ -64,7 +62,7 @@ impl Assembled { pub fn new( txn: &Transaction, sim_res: SimulateTransactionResponse, - resource_fee: Option, + resource_fee: Option, ) -> Result { let txn = assemble(txn, &sim_res, resource_fee)?; Ok(Self { txn, sim_res }) @@ -205,7 +203,7 @@ impl Assembled { fn assemble( raw: &Transaction, simulation: &SimulateTransactionResponse, - resource_fee: Option, + resource_fee: Option, ) -> Result { let mut tx = raw.clone(); @@ -219,16 +217,26 @@ fn assemble( }); } - let min_resource_fee = if let Some(rf) = resource_fee { - tracing::trace!( - "setting resource fee to {rf} from {}", - simulation.min_resource_fee - ); - rf - } else { - simulation.min_resource_fee + let mut transaction_data = simulation.transaction_data()?; + let min_resource_fee = match resource_fee { + Some(rf) => { + tracing::trace!( + "setting resource fee to {rf} from {}", + simulation.min_resource_fee + ); + transaction_data.resource_fee = rf; + // short circuit the submission error if the resource fee is negative + // technically, a negative resource fee is valid XDR so it won't panic earlier + // this should not occur as we validate resource fee before calling assemble + u64::try_from(rf).map_err(|_| { + Error::TransactionSubmissionFailed(String::from( + "TxMalformed - negative resource fee", + )) + })? + } + // transaction_data is already set from simulation response + None => simulation.min_resource_fee, }; - let transaction_data = simulation.transaction_data()?; let mut op = tx.operations[0].clone(); if let OperationBody::InvokeHostFunction(ref mut body) = &mut op.body { @@ -257,13 +265,10 @@ fn assemble( } } - // Update transaction fees to meet the minimum resource fees. - // Choose larger of existing fee or inclusion + resource fee. - let min_tx_fee: u64 = DEFAULT_TRANSACTION_FEES.into(); - tx.fee = tx.fee.max( - u32::try_from(min_tx_fee + min_resource_fee) - .map_err(|_| Error::LargeFee(min_tx_fee + min_resource_fee))?, - ); + // Update the transaction fee to be the sum of the inclusion fee and the + // minimum resource fee from simulation. + let total_fee: u64 = u64::from(raw.fee) + min_resource_fee; + tx.fee = u32::try_from(total_fee).map_err(|_| Error::LargeFee(total_fee))?; tx.operations = vec![op].try_into()?; tx.ext = TransactionExt::V1(transaction_data); @@ -519,6 +524,22 @@ mod tests { } } + #[test] + fn test_assemble_transaction_calcs_fee() { + let mut sim = simulation_response(); + sim.min_resource_fee = 12345; + let mut txn = single_contract_fn_transaction(); + txn.fee = 10000; + let Ok(result) = assemble(&txn, &sim, None) else { + panic!("assemble failed"); + }; + + assert_eq!(12345 + 10000, result.fee); + // validate it updated sorobantransactiondata block in the tx ext + let expected_tx_data = transaction_data(); + assert_eq!(TransactionExt::V1(expected_tx_data), result.ext); + } + #[test] fn test_assemble_transaction_overflow_behavior() { // @@ -561,18 +582,35 @@ mod tests { #[test] fn test_assemble_transaction_with_resource_fee() { let sim = simulation_response(); - let txn = single_contract_fn_transaction(); - let resource_fee = 12345u64; + let mut txn = single_contract_fn_transaction(); + txn.fee = 500; + let resource_fee = 12345i64; let Ok(result) = assemble(&txn, &sim, Some(resource_fee)) else { panic!("assemble failed"); }; // validate it auto updated the tx fees from sim response fees // since it was greater than tx.fee - assert_eq!(12345 + 100, result.fee); + assert_eq!(12345 + 500, result.fee); // validate it updated sorobantransactiondata block in the tx ext - assert_eq!(TransactionExt::V1(transaction_data()), result.ext); + let mut expected_tx_data = transaction_data(); + expected_tx_data.resource_fee = resource_fee; + assert_eq!(TransactionExt::V1(expected_tx_data), result.ext); + } + + // This should never occur, as resource fee is validated before being passed into + // assemble. But test the behavior just in case. + #[test] + fn test_assemble_transaction_input_resource_fee_negative_errors() { + let mut sim = simulation_response(); + sim.min_resource_fee = 12345; + let mut txn = single_contract_fn_transaction(); + txn.fee = 500; + let resource_fee = -1; + let result = assemble(&txn, &sim, Some(resource_fee)); + + assert!(result.is_err()); } #[test] @@ -592,7 +630,7 @@ mod tests { assert_eq!(txn.fee, 100, "modified txn.fee: update the math below"); // 1: wiggle room math overflows but result fits - let resource_fee: u64 = (u32::MAX - 100).into(); + let resource_fee: i64 = (u32::MAX - 100).into(); match assemble(&txn, &response, Some(resource_fee)) { Ok(asstxn) => { @@ -603,7 +641,7 @@ mod tests { } // 2: combo overflows, should throw - let resource_fee: u64 = (u32::MAX - 99).into(); + let resource_fee: i64 = (u32::MAX - 99).into(); match assemble(&txn, &response, Some(resource_fee)) { Err(Error::LargeFee(fee)) => { diff --git a/cmd/soroban-cli/src/resources.rs b/cmd/soroban-cli/src/resources.rs index d000c2ddb..8945f5b6f 100644 --- a/cmd/soroban-cli/src/resources.rs +++ b/cmd/soroban-cli/src/resources.rs @@ -11,8 +11,8 @@ use crate::commands::HEADING_RPC; #[group(skip)] pub struct Args { /// Set the fee for smart contract resource consumption, in stroops. 1 stroop = 0.0000001 xlm. Overrides the simulated resource fee - #[arg(long, env = "STELLAR_RESOURCE_FEE", help_heading = HEADING_RPC)] - pub resource_fee: Option, + #[arg(long, env = "STELLAR_RESOURCE_FEE", value_parser = clap::value_parser!(i64).range(0..u32::MAX.into()), help_heading = HEADING_RPC)] + pub resource_fee: Option, /// ⚠️ Deprecated, use `--instruction-leeway` to increase instructions. Number of instructions to allocate for the transaction #[arg(long, help_heading = HEADING_RPC)] pub instructions: Option,