Apply inclusion fee and resource fee to simulated tx correctly #2355
Conversation
leighmcculloch
left a comment
leighmcculloch
left a comment
There was a problem hiding this comment.
👋🏻 Little concerned we're working around and covering a bug, take a look inline.
cmd/soroban-cli/src/assembled.rs
Outdated
| resource_fee: Option<u64>, | ||
| ) -> Result<Transaction, Error> { | ||
| let mut tx = raw.clone(); | ||
|
|
||
| // Right now simulate.results is one-result-per-function, and assumes there is only one | ||
| // operation in the txn, so we need to enforce that here. I (Paul) think that is a bug | ||
| // in soroban-rpc.simulateTransaction design, and we should fix it there. | ||
| // TODO: We should to better handling so non-soroban txns can be a passthrough here. | ||
| if tx.operations.len() != 1 { | ||
| return Err(Error::UnexpectedOperationCount { | ||
| count: tx.operations.len(), | ||
| }); | ||
| } | ||
|
|
||
| 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) { |
There was a problem hiding this comment.
To me the bug seems like resource_fee is a u64, not that we're missing a conversion from u64 to i64 and need a fallback value if a user enters a value too large.
Does the issue stem from the Args specifying fees on the command line as u64 instead of i64?
stellar-cli/cmd/soroban-cli/src/resources.rs
Lines 12 to 15 in 068052d
We should collect it as the correct type so that if a user enters an invalid value, the command line hard errors right there, forcing the developer to discover the issue rather than hide the mistake.
There was a problem hiding this comment.
No, this was just overlooked during the first PR adding this.
The real issue was twofold:
- we didn't have the
transaction_data.resource_feegetting set when a user supplied this arg - the tx.fee was being set based on the
min_tx_fee, not theraw.fee
This being said, I can make this change with the additional check that it is positive to improve the error handling, as it is likely the right way to do it.
What
We were overriding inclusion fee and not applying resource fee inputs to the simulated transaction