Apply inclusion fee and resource fee to simulated tx correctly

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<ResourceConfig>,
-    resource_fee: Option<u64>,
+    resource_fee: Option<i64>,
 ) -> Result<Assembled, Error> {
     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<u64>,
+        resource_fee: Option<i64>,
     ) -> Result<Self, Error> {
         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<u64>,
+    resource_fee: Option<i64>,
 ) -> Result<Transaction, Error> {
     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)) => {

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<u64>,
+    #[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<i64>,
     /// ⚠️ Deprecated, use `--instruction-leeway` to increase instructions. Number of instructions to allocate for the transaction
     #[arg(long, help_heading = HEADING_RPC)]
     pub instructions: Option<u32>,