Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsonrpc: when dry-running suggest a gas price if an input object is c… #21355

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bmwill
Copy link
Contributor

@bmwill bmwill commented Feb 26, 2025

…ongested

Description

Describe the changes or additions included in this PR.

Test plan

How did you test the new or updated feature?


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

Copy link

vercel bot commented Feb 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Feb 26, 2025 1:20am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Feb 26, 2025 1:20am

@bmwill bmwill temporarily deployed to sui-typescript-aws-kms-test-env February 26, 2025 01:19 — with GitHub Actions Inactive
Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to roll this into the gas price that we dry-run the transaction with? i.e. before we dry-run, get a suggested price (or use the reference gas price), then the price will be made available as part of the existing field in transaction data.

This also has the benefit that when you dry-run to do gas-cost estimation, it will take into account the suggested price because of contention.

@bmwill
Copy link
Contributor Author

bmwill commented Feb 26, 2025

One possible issue with that is that it would technically be a change in semantics of the api, that and it would only benefit users if they explicitly omit the gas price as a part of the dryrun. If the wallet/sdk team would prefer that approach though then we can look at doing it that way

@dmitri-perelman
Copy link
Contributor

Makes sense, thanks! No strong opinions about where to put the value (having it in a response is more intuitive for me). I assume most folks wouldn't use this RPC directly: it's going to be mostly leveraged by the SDKs, right?

Just want to make sure they understand the semantics of the result and we don't need to load the variable name to make it self-explanatory :) (it's None for most objects most of the time, and the non-None values might be volatile and lose freshness fast)

@amnn
Copy link
Contributor

amnn commented Feb 27, 2025

If the wallet/sdk team would prefer that approach though then we can look at doing it that way

This is the main question -- adding @Jordan-Mysten and @hayes-mysten to weigh in.

  • Adding it as a new field makes sense if we think that people may reasonably want to know what the estimate is without the local fee market information, or if people use the gas price from dry-run for anything other than setting the gas price on their transaction for now.
  • Otherwise, setting the gas price on the returned transaction data makes sense to me (especially if wallets in general are used to taking the price output from dry-run and using it to fill in the transaction before execution -- it means they will all pick up this change without updates on their side).

I'm also thinking about our discussions around a resolution/simulation API that takes a partial transaction and fills out all these details -- the latter approach is more aligned with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants