[cli] Add automatic gas estimation, and move all CLI client commands to use it #16503
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
stefan-mysten
force-pushed
the
cli_gas_management
branch
from
208c7c2
to
1cfb13f
Compare
stefan-mysten
changed the title
stefan-mysten
changed the title
stefan-mysten
changed the title
stefan-mysten
changed the title
stefan-mysten
force-pushed
the
cli_gas_management
branch
from
f89b4ce
to
b27958c
Compare
stefan-mysten
force-pushed
the
cli_gas_management
branch
from
b27958c
to
144dd86
Compare
There was a problem hiding this comment.
I like the change overall! Main thing I'd love to change before we land this is how the transactions are created.
Today, you are duplicating the logic involved in creating the TransactionData, once for the gas estimation and another time to actually run it. This is not great from a code repetition perspective, but it's also not good from an efficiency or consistency perspective. Some of these transaction building commands do a fair bit of work, talking to RPC to resolve ObjectIDs into ObjectRefs etc, you don't want to redo that work because it's not cheap, and in general you may get a different answer from run to run, and while that's usually going to mean the transaction execution will fail, we should still return a consistent response in that case (e.g. the transaction you dry ran is the same as the one that you executed).
The fix is to split the logic for generating transactions into three phases:
- Build the
TransactionKind-- this is usually where most of the effort and consistency concerns go, and we can always do this once because the gas budget is not relevant for this part. - Build the
TransactionDatafor dry-run, from a clone of theTransactionKind. - Build the
TransactionDatafor execution, using the gas budget you got from dry-run, and execute it.
crates/sui/src/client_commands.rs
Outdated
| ) -> Result<u64, anyhow::Error> { | ||
| let client = context.get_client().await?; | ||
| let dry_run = client.read_api().dry_run_transaction_block(tx).await?; | ||
| Ok(dry_run.effects.gas_cost_summary().gas_used()) |
There was a problem hiding this comment.
I believe we like to add a "fudge factor" to the estimated gas budget to account for differences due to shared object usage (or Clock, or Randomness, etc). I believe it's about 10%, but the source of truth is the equivalent logic in TS SDK.
There was a problem hiding this comment.
Added, let me know if you have any feedback. Here's the TypeScript SDK code for this.
… Rename the function to estimate_gas_budget
stefan-mysten
force-pushed
the
cli_gas_management
branch
from
89aaac9
to
b76830b
Compare
|
Superseded by #17322 |
Description
This PR adds automatic gas estimation support for all CLI client commands by default. Providing a specific gas budget is still possible through the
--gas-budgetoption.Note this also fixes a problem with the way CLI PTB tests are executed, which led to issues (cannot find test files).
Test Plan
Added new tests + existing tests.
cargo nextest run --test cli_testsIf your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.
Type of Change (Check all that apply)
Release notes
The CLI client commands do not require to provide a
--gas-budgetvalue anymore, as it is automatically inferred through a dry run. It is still possible to provide one, if the user wants.