soroban-rpc: Calculate soroban transaction data in libpreflight #636
Conversation
2opremio
force-pushed
the
calculate-soroban-transaction-data
branch
from
a91f46b to
2530c56
Compare
| )?, | ||
| }; | ||
| let (min_fee, ref_fee) = | ||
| compute_transaction_resource_fee(&transaction_resources, &fee_configuration); |
There was a problem hiding this comment.
@dmkozh I am interpreting min_fee as the fee required to access resources, on top of which the transaction submitter needs to add the Stellar classic base fee. Is that correct?
There was a problem hiding this comment.
Yes, that's the current state, hence they need to add at least 100 stroops on top of this ('inclusion fee'). Going forward we may set a different minimum value for the inclusion fee.
There was a problem hiding this comment.
@tsachiherman , @2opremio , based on this statement and response on the js-soroban-client pr for handling inclusion fee , I've tentatively added 'suggestedInclusionFee' to expected rpc simulation response in js-soroban-client , if you agree, would need to be added here on SimulateTransactionResponse, let me know if this is right direction, thanks!
There was a problem hiding this comment.
I had already added MinResourceFee to the SimulateTransactionResponse, isn't that enough?
There was a problem hiding this comment.
yes, if MinResourceFee sums in the 'inclusion fee' suggested by @dmkozh also, I wasn't sure if that was the case as he was mentioning this 'inclusion fee' as something that should be added on top by 'they' which I understood to be remote clients after getting simulation response?
There was a problem hiding this comment.
MinResourceFee needs to be added on top of the classic Stellar fee (typically the minimum base fee of 100).
There was a problem hiding this comment.
ok, I'm trying to confirm this rule on handling fees to complete js-soroban-client when it prepares a users tx with preflight results, @tsachiherman you mentioned a different approach of taking max of classic fee(already set on the tx by user) or MinResourceFee. Which approach is correct for applying MinResourceFee on a user's tx, Max comparison or Summation? And this means there will be no other suggested inclusion fees provided by simulation response?
There was a problem hiding this comment.
received clarification, the rule is to use summation with existing tx classic fee:
given that it's called MinResourceFee, it would imply that the user would need to pay a minimum of MinResourceFee + classic fees. so, if the user provided an amount that is anywhere less then (MinResourceFee + classic fees), then we should bump that up to this number. ( since we know it won't work otherwise )
|
Not exactly. The preflightlib wrap the call to the preflight with the fee
calculation and return that in the same call.
That allows us to avoid additional round trip to calculate the fees.
…On Mon, May 8, 2023, 11:16 AM shawn ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In cmd/soroban-rpc/internal/methods/simulate_transaction.go
<#636 (comment)>
:
> }
type SimulateTransactionResponse struct {
- Error string `json:"error,omitempty"`
- Results []SimulateTransactionResult `json:"results,omitempty"`
- Cost SimulateTransactionCost `json:"cost"`
- LatestLedger int64 `json:"latestLedger,string"`
+ Error string `json:"error,omitempty"`
+ // TODO: update documentation and review field names
+ TransactionData string `json:"transaction_data"` // SorobanTransactionData XDR in base64
+ Events []string `json:"events"` // DiagnosticEvent XDR in base64
+ MinResourceFee int64 `json:"minResourceFee,string"`
is minResourceFee the extent of suggested inclusion fees from preflight
for now?
—
Reply to this email directly, view it on GitHub
<#636 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF2OOHZ5TC4RODVIROIW3ELXFES6DANCNFSM6AAAAAAXXCF2QI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
| TransactionData string `json:"transaction_data"` // SorobanTransactionData XDR in base64 | ||
| Events []string `json:"events"` // DiagnosticEvent XDR in base64 | ||
| MinResourceFee int64 `json:"minResourceFee,string"` | ||
| Results []SimulateHostFunctionResult `json:"results,omitempty"` |
There was a problem hiding this comment.
for consideration, remove the omitempty, to include results array in response, consistent with other collections in the struct like the events. makes a little easier to use from client side, doesn't have to check for absence first, etc.
There was a problem hiding this comment.
How would it behave with classic transaction ?
There was a problem hiding this comment.
the results array would be empty, clients figure out what do when no results, but at least they can depend on presence and don't have to null check first then reference the length, in cases where you expect this to be absent instead, should those be conveyed more as an error?
| )?, | ||
| }; | ||
| let (min_fee, ref_fee) = | ||
| compute_transaction_resource_fee(&transaction_resources, &fee_configuration); |
There was a problem hiding this comment.
Yes, that's the current state, hence they need to add at least 100 stroops on top of this ('inclusion fee'). Going forward we may set a different minimum value for the inclusion fee.
|
Thanks for the review @dmkozh !! |
* Bump Rust dependencies to support fees (#625) * soroban-rpc: update xdr (#627) update * soroban-rpc: Bump core dependencies supporting fees (#634) * fees: Bump stellar/go dependency (#637) * Bump Rust dependencies to their latest version (#635) * port: merge soroban cli inspect from main (#639) * Try new docker compose plugin (#631) * Try new docker compose plugin * Ok we need to add the docker apt repo :( * Need an apt-get update * Force-install docker-compose * Remove old docker-compose first? * try removing moby-compose * Clean up workflow * Add meta output to soroban-cli contract inspect output (#620) * Add env_meta output to soroban-cli contract inspect output * Output contract meta and improve contract inspect formatting * Indent the docs to match * rustfmt * Improve function inspect output * improving the inspect output formatting * handle lib in inspect output * Satiate clippy --------- Co-authored-by: Tsachi Herman <[email protected]> --------- Co-authored-by: Paul Bellamy <[email protected]> * soroban-rpc: Calculate soroban transaction data in libpreflight (#636) * Create Skeletton to calculate soroban transaction data in libpreflight * Calculate transaction data a minimum fee in libpreflight * Calculate transaction data a minimum fee in libpreflight * Make integration tests compilable again * Assume missing entries from ledger storage are created * Update simulate transaction integration tests * Add TODOs for the integration tests to pass * Make events global * Address review feedback * Add TODO about taking into account the Auth size * Add a few more TODOs and debug info * First attempt and preflighting all transactions * Always take into account keys when computing unmodified ledgers * Come up with the minimum fee to add in order to make it work * Fix transaction_resources computation * Remove debug printout * Increase instruction leeway * Fix the rest of the simulate transaction test * Fix remaining intgeration tests * Address review feedback * Tweak the instruction estimation further * Address review feedback * Fix tests again * Obtain auth data per function * Take auth data into account for estimating envelope size * Split up libpreflight in multiple modules and improve the storage code * Fix TODO * Remove another TODO * Remove yet another TODO * Appease clippy * update * update core version * make sure to test multiple host functions invocations. --------- Co-authored-by: Tsachi Herman <[email protected]> * try to replace the dns servers to avoid stale addresses. * rollback dns change * Add handling for invalid toml file support in ConfigOption.setValue * #519: fixed issue with file path configs being lost after re loading cli/env flags, ordering of config value application was modified to default->env/cli->file path. --------- Co-authored-by: Tsachi Herman <[email protected]> Co-authored-by: Paul Bellamy <[email protected]> Co-authored-by: Shawn Reuland <[email protected]>
* Bump Rust dependencies to support fees (#625) * soroban-rpc: update xdr (#627) update * soroban-rpc: Bump core dependencies supporting fees (#634) * fees: Bump stellar/go dependency (#637) * Bump Rust dependencies to their latest version (#635) * port: merge soroban cli inspect from main (#639) * Try new docker compose plugin (#631) * Try new docker compose plugin * Ok we need to add the docker apt repo :( * Need an apt-get update * Force-install docker-compose * Remove old docker-compose first? * try removing moby-compose * Clean up workflow * Add meta output to soroban-cli contract inspect output (#620) * Add env_meta output to soroban-cli contract inspect output * Output contract meta and improve contract inspect formatting * Indent the docs to match * rustfmt * Improve function inspect output * improving the inspect output formatting * handle lib in inspect output * Satiate clippy --------- Co-authored-by: Tsachi Herman <[email protected]> --------- Co-authored-by: Paul Bellamy <[email protected]> * soroban-rpc: Calculate soroban transaction data in libpreflight (#636) * Create Skeletton to calculate soroban transaction data in libpreflight * Calculate transaction data a minimum fee in libpreflight * Calculate transaction data a minimum fee in libpreflight * Make integration tests compilable again * Assume missing entries from ledger storage are created * Update simulate transaction integration tests * Add TODOs for the integration tests to pass * Make events global * Address review feedback * Add TODO about taking into account the Auth size * Add a few more TODOs and debug info * First attempt and preflighting all transactions * Always take into account keys when computing unmodified ledgers * Come up with the minimum fee to add in order to make it work * Fix transaction_resources computation * Remove debug printout * Increase instruction leeway * Fix the rest of the simulate transaction test * Fix remaining intgeration tests * Address review feedback * Tweak the instruction estimation further * Address review feedback * Fix tests again * Obtain auth data per function * Take auth data into account for estimating envelope size * Split up libpreflight in multiple modules and improve the storage code * Fix TODO * Remove another TODO * Remove yet another TODO * Appease clippy * update * update core version * make sure to test multiple host functions invocations. --------- Co-authored-by: Tsachi Herman <[email protected]> * try to replace the dns servers to avoid stale addresses. * rollback dns change * Add handling for invalid toml file support in ConfigOption.setValue * update * appease cargo fmt * update * update * Add rpc::server.get_network * Add rpc::server.prepare_transaction * get prepare_transaction stuff compiling * use prepare_transaction and simplify * Add and use client.prepare_and_send_transaction * appease cargo fmt * update fix compilation. * Improve output for failed txn submission * Add missing field from getLedgerEntries response * Add transaction::assemble unit tests based on js-soroban-client * Clean up some leftover printlns * We don't need to manually set transaction_data, it can come from the simulation --------- Co-authored-by: Alfonso Acosta <[email protected]> Co-authored-by: Paul Bellamy <[email protected]>
What
Calculate soroban transaction data in libpreflight
This changes the API of
simulateTransaction.SimulateTransactionResponsechanges as follows:minResourceFee, is added to tell the user what fee should be used with respect to the resources consumed by the host function invocations.Why
Closes #622
Known limitations
N/A