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

RFC: feat(keys/client): support automatic gas calculation with -simulate test #3330

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Dec 12, 2024

Addresses #1826 (and potentially resolves, if this can be seen as a good implementation?).

This PR implements gas calculations in a manner similar to what's described in this comment; but I'm making this PR as a proof of concept before moving ahead and adding tests and cleaning up the code.

What this PR enables, is to work locally with gnodev with gnokey commands as simple as the following:

$ gnokey maketx call -pkgpath "gno.land/r/demo/counter" -func "Increment" -broadcast test1
Enter password.
(5 int)


OK!
GAS WANTED: 101000
GAS USED:   91809
HEIGHT:     11
EVENTS:     []
TX HASH:    ahbx2M9cIxOuZqSNWLhjAVPb0YxeCBKOrHpm+wi4jjw=

For how this works, here's a brief description from gnokey maketx --help:

  -gas-fee ...                    gas payment fee; automatically set with -gas-wanted auto
  -gas-wanted auto                gas requested for tx; with auto, exclusively with -broadcast and -simulate test,
                                  detect required gas from the transaction simulation. transaction is simulated with
                                  max_gas = 10000000, then run with min(gas_used * 1.10, max_gas)

The objective is to take advantage of the fact we're now doing transaction simulation in gnokey, to try to use that for calculating the amount of gas used in the transaction.

  1. Do we want this?
  2. Right now -gas-fee is always 1gnot (as this field is ignored on-chain for now, anyway). How should it eventually be calculated? Should we ask the user for confirmation?
  3. The values for max_gas and gas_fee are set with globals in crypto/keys/client, so that other clients aside from keyscli can modify them. Should we prefer something else?

@thehowl thehowl self-assigned this Dec 12, 2024
@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Dec 12, 2024
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 12, 2024

🛠 PR Checks Summary

🔴 The pull request head branch must be up-to-date with its base (more info)

Manual Checks (for Reviewers):
  • SKIP: Do not block the CI for this PR
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🔴 The pull request head branch must be up-to-date with its base (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: thehowl/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base (more info)

If

🟢 Condition met
└── 🟢 On every pull request

Then

🔴 Requirement not satisfied
└── 🔴 Head branch (thehowl:dev/morgan/auto-gas-estimate) is up to date with base (master): behind by 2 / ahead by 2

Manual Checks
**SKIP**: Do not block the CI for this PR

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 12.24490% with 129 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
tm2/pkg/crypto/keys/client/maketx.go 12.58% 125 Missing ⚠️
tm2/pkg/crypto/keys/client/broadcast.go 0.00% 2 Missing ⚠️
tm2/pkg/crypto/keys/client/send.go 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@leohhhn
Copy link
Contributor

leohhhn commented Dec 12, 2024

This would be a huge UX bump; just one concern: what if the price of gas is too high, and I as a user am not willing to pay at that moment, but rather wait for it to drop? For this, I would need an estimate for how much ugnot I will pay to execute the tx. This is a feature all standard crypto wallets support.

If gas-wanted is auto, I suggest this flow:

  • Run the simulation
  • Get gas used + fetch gas price
  • Display estimated total ugnot cost (possibly also gas used, etc) to user
  • Ask for confirmation from user
  • Ultimately send tx to chain.

WDYT?

On another note, I would love if we can set broadcast=true by default.

@Kouteki Kouteki added the in focus Core team is prioritizing this work label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Agenda
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants