From 068052d607e8524f5baa56b2d31666babda5a7f5 Mon Sep 17 00:00:00 2001 From: mootz12 Date: Mon, 12 Jan 2026 15:12:00 -0500 Subject: [PATCH 1/2] fix: apply inclusion fee and resource fee to simulated tx correctly --- cmd/soroban-cli/src/assembled.rs | 78 +++++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 17 deletions(-) diff --git a/cmd/soroban-cli/src/assembled.rs b/cmd/soroban-cli/src/assembled.rs index 06d4e48ff3..a001e2f4d8 100644 --- a/cmd/soroban-cli/src/assembled.rs +++ b/cmd/soroban-cli/src/assembled.rs @@ -11,8 +11,6 @@ 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, @@ -219,16 +217,26 @@ fn assemble( }); } + let mut transaction_data = simulation.transaction_data()?; let min_resource_fee = if let Some(rf) = resource_fee { - tracing::trace!( - "setting resource fee to {rf} from {}", + if let Ok(rf_i64) = i64::try_from(rf) { + tracing::trace!( + "setting resource fee to {rf} from {}", + simulation.min_resource_fee + ); + transaction_data.resource_fee = rf_i64; + rf + } else { + // Doesn't fit in i64, use simulation value + tracing::trace!( + "input resource fee {rf} does not fit in i64, using simulation value {}", + simulation.min_resource_fee + ); simulation.min_resource_fee - ); - rf + } } else { 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,7 +582,8 @@ mod tests { #[test] fn test_assemble_transaction_with_resource_fee() { let sim = simulation_response(); - let txn = single_contract_fn_transaction(); + let mut txn = single_contract_fn_transaction(); + txn.fee = 500; let resource_fee = 12345u64; let Ok(result) = assemble(&txn, &sim, Some(resource_fee)) else { panic!("assemble failed"); @@ -569,10 +591,32 @@ mod tests { // 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.try_into().unwrap(); + assert_eq!(TransactionExt::V1(expected_tx_data), result.ext); + } + + #[test] + fn test_assemble_transaction_input_resource_fee_too_large() { + let mut sim = simulation_response(); + sim.min_resource_fee = 12345; + let mut txn = single_contract_fn_transaction(); + txn.fee = 500; + let resource_fee = i64::MAX.unsigned_abs() + 1; + 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 + 500, 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] From b6b7e8f11b1b1fcc1259f2a7497c75862406ce55 Mon Sep 17 00:00:00 2001 From: mootz12 Date: Tue, 13 Jan 2026 11:40:37 -0500 Subject: [PATCH 2/2] fix: take in resource_fee as i64 --- cmd/soroban-cli/src/assembled.rs | 58 ++++++++++++++------------------ cmd/soroban-cli/src/resources.rs | 4 +-- 2 files changed, 28 insertions(+), 34 deletions(-) diff --git a/cmd/soroban-cli/src/assembled.rs b/cmd/soroban-cli/src/assembled.rs index a001e2f4d8..1a3fa7da71 100644 --- a/cmd/soroban-cli/src/assembled.rs +++ b/cmd/soroban-cli/src/assembled.rs @@ -15,7 +15,7 @@ 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(), @@ -62,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 }) @@ -203,7 +203,7 @@ impl Assembled { fn assemble( raw: &Transaction, simulation: &SimulateTransactionResponse, - resource_fee: Option, + resource_fee: Option, ) -> Result { let mut tx = raw.clone(); @@ -218,24 +218,24 @@ fn assemble( } let mut transaction_data = simulation.transaction_data()?; - let min_resource_fee = if let Some(rf) = resource_fee { - if let Ok(rf_i64) = i64::try_from(rf) { + 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_i64; - rf - } else { - // Doesn't fit in i64, use simulation value - tracing::trace!( - "input resource fee {rf} does not fit in i64, using simulation value {}", - simulation.min_resource_fee - ); - 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", + )) + })? } - } else { - simulation.min_resource_fee + // transaction_data is already set from simulation response + None => simulation.min_resource_fee, }; let mut op = tx.operations[0].clone(); @@ -584,7 +584,7 @@ mod tests { let sim = simulation_response(); let mut txn = single_contract_fn_transaction(); txn.fee = 500; - let resource_fee = 12345u64; + let resource_fee = 12345i64; let Ok(result) = assemble(&txn, &sim, Some(resource_fee)) else { panic!("assemble failed"); }; @@ -595,28 +595,22 @@ mod tests { // validate it updated sorobantransactiondata block in the tx ext let mut expected_tx_data = transaction_data(); - expected_tx_data.resource_fee = resource_fee.try_into().unwrap(); + 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_too_large() { + 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 = i64::MAX.unsigned_abs() + 1; - 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 + 500, result.fee); + let resource_fee = -1; + let result = assemble(&txn, &sim, Some(resource_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); + assert!(result.is_err()); } #[test] @@ -636,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) => { @@ -647,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 d000c2ddbf..8945f5b6fd 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,