Use the new Soroban RPC simulation method when preparing transactions #77
Conversation
…apply new minium suggested fee
package.json
Outdated
| @@ -138,7 +138,7 @@ | |||
| "axios": "0.25.0", | |||
| "es6-promise": "^4.2.4", | |||
| "lodash": "4.17.21", | |||
| "stellar-base": "8.2.2-soroban.12", | |||
| "stellar-base": "https://github.com/stellar/js-stellar-base\\#host_functions_ts", | |||
There was a problem hiding this comment.
I'll change this to v9.0.0-soroban.1 when it's out, for now using this slightly newer stellar-base that has tweaks on txbuilder - js-stellar-base#604
There was a problem hiding this comment.
just going to unresolve this so you don't forget after you get approval
There was a problem hiding this comment.
sreuland
changed the title
| rawInvokeHostFunctionOp.functions.length !== simulation.results.length | ||
| ) { | ||
| throw new Error( | ||
| "preflight simulation results do not contain same count of HostFunctions that InvokeHostFunctionOp in the transaction has.", |
There was a problem hiding this comment.
I don't understand the intricacies of the multi-op host functions yet: why is the length enforced to be 1 above, but that isn't enforced here? I guess it's implicit, but I just want to make sure.
Related: is there no way to simulate transactions with multiple invokeHostOperations?
There was a problem hiding this comment.
@dmkozh , can you confirm with the latest changes for multi-function InvokeHostFunctionOp, the limitation for a single InvokeHostFunctionOp in Tx is still being enforced? I thought have seen that mentioned in chat and was assuming that limitation on validation of tx here.
There was a problem hiding this comment.
ohhhh yeah sorry, you're right 🤦 one op per tx, but the op can have many fns. got it!
😆 too many layers 🍰
src/transaction.ts
Outdated
| // surpass that value. | ||
| // 'classic' fees are measured as the product of tx.fee * 'number of operations', In soroban contract tx, | ||
| // there can only be single operation in the tx, so can safely make simplification of classic fee = tx.fee. | ||
| fee: Math.max(classicFeeNum + minResourceFeeNum, classicFeeNum).toString(), |
There was a problem hiding this comment.
wait, when will this ever return the second case of the max? it seems like the first arg is strictly larger. I guess minResourceFeeNum could be negative, but that's... adversarial on the client's end?
There was a problem hiding this comment.
I think that the second argument to the Math.max function was supposed to be raw.fee
There was a problem hiding this comment.
ah, good point, max is un-needed at this point, the incoming tx.fee has really become the 'suggested inclusion fee' when using server.prepareTransaction(tx, networkPassphrase).
I think we can only append minResourceFee to what user provides on incoming tx.fee and return the sum as the updated tx.fee on the returned transaction from server.prepareTransaction(tx, networkPassphrase). I've updated the js docs on that method to illustrate further.
src/transaction.ts
Outdated
| const txnBuilder = new TransactionBuilder(source, { | ||
| // automatically update the 'classic' tx fee if min resource fees from simulation response | ||
| // surpass that value. | ||
| // 'classic' fees are measured as the product of tx.fee * 'number of operations', In soroban contract tx, |
There was a problem hiding this comment.
classic fees are classicFeeNum * operations num right ?
There was a problem hiding this comment.
yes, on classic tx's that would be the way classic fees would result, with the limitation of only one operation in the soroban tx, have reduced to just classic fees = classicFeeNum(the parsed numeric value of user specified tx.fee).
package.json
Outdated
| @@ -138,7 +138,7 @@ | |||
| "axios": "0.25.0", | |||
| "es6-promise": "^4.2.4", | |||
| "lodash": "4.17.21", | |||
| "stellar-base": "8.2.2-soroban.12", | |||
| "stellar-base": "https://github.com/stellar/js-stellar-base\\#host_functions_ts", | |||
There was a problem hiding this comment.
just going to unresolve this so you don't forget after you get approval
| rawInvokeHostFunctionOp.functions.length !== simulation.results.length | ||
| ) { | ||
| throw new Error( | ||
| "preflight simulation results do not contain same count of HostFunctions that InvokeHostFunctionOp in the transaction has.", |
There was a problem hiding this comment.
ohhhh yeah sorry, you're right 🤦 one op per tx, but the op can have many fns. got it!
😆 too many layers 🍰
Shaptic
changed the title
Adding support to 'prepare transaction' to apply new rpc simulation data, including multi-function auths, and suggested fees to the tx. Wanted to put the PR up asap get visibility and any course correction here if needed. working on unit tests now.
This depends on the rpc simulation changes - https://github.com/stellar/soroban-tools/pull/636/files
Closes #76