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

chore(imp) add fees auto flag logic #1386

Merged
merged 25 commits into from
Feb 22, 2023
Merged

chore(imp) add fees auto flag logic #1386

merged 25 commits into from
Feb 22, 2023

Conversation

GAtom22
Copy link
Contributor

@GAtom22 GAtom22 commented Feb 13, 2023

Description

Add logic to fulfill the requirements specified on the task.

With the changes included in this PR, when no fees are specified, the requiredFees are used as a fallback instead of throwing an error. This is needed to achieve the desired behavior that --gas='auto' has the same functionality as --fees='auto'

Note: To work as described in the mentioned ticket, this PR on evmos' cosmos-sdk fork must be merged and included on a release. Then, that new cosmos-sdk version should be used on this repo.


Closes https://linear.app/evmos/issue/ENG-1474/cli-errors-when-paying-for-gas-fees

@linear
Copy link

linear bot commented Feb 13, 2023

ENG-1474 CLI errors when paying for gas fees

Context

There are three conditions that need to be satisfied in order to pay for gas:

  • gas >= gasWanted
  • gasPrice >= minGasPrices (this is a setting in the node configuration of the node that you are sending the Tx to)
  • gasPrice >= global min-gas-price

Fees calculation: fees = gas price * gas limit

Current behavior

As a user, you specify two fields in a transaction. Using the --gas=auto flag sometimes breaks one of the rules above. Users are often confused by the error message on the CLI or using a frontend that forwards the error from the protocol.

Expected behavior

There are two types of users interacting with fees: normal users and experts

  • Normal users have one standard way of defining fees for any transaction type (i.e. just --fees=auto).
    • gas limit is the gas calculated through simulation
    • gas price is defined by the global min gas price
    • fees are calculated based on gas limit and gas price
  • Experts can provide specific values of the fees = gas price * gas limit formula. The different cases are:
    • User provides only --fees=[specific amount] OR --fees=[specific amount] and --gas=auto
      • gas limit is the gas calculated through simulation
      • gas price is calculated by gas limit
    • User provides --fees=[specific amount] and --gas=[specific amount]
      • gas price is calculated by gas limit and fees
    • User provides --fees=[specific amount] and --gas-prices=[specific amount]
      • gas limit is calculated by gas price and fees
    • User provides only --gas=auto
    • same as --fees=auto
    • User provides --gas=[specific amount] and --gas-prices=[specific amount]
      • fees are calculated by gas price and gas limit
  • In any case, where the Tx fails because it violates one of the rules mentioned in #Context the error message should tell me exactly how much gas, gasPrice and fees were defined/calculated and which field wasn’t high enough. It also should tell me the minimum amount of fees that I need to provide for that specific Tx.

Tasks:

  • add "auto" option for the --fees flag
  • when passing only --fees make sure gas is estimated thru simulation (ATM default gas is 20000)
  • make --gas=auto behave the same as --fees=auto

@github-actions github-actions bot added the tests label Feb 14, 2023
@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #1386 (f8390b9) into main (e7a8fae) will decrease coverage by 0.01%.
The diff coverage is 65.00%.

❗ Current head f8390b9 differs from pull request most recent head 550a373. Consider uploading reports for the commit 550a373 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1386      +/-   ##
==========================================
- Coverage   72.32%   72.31%   -0.01%     
==========================================
  Files         260      260              
  Lines       17723    17732       +9     
==========================================
+ Hits        12818    12823       +5     
- Misses       4336     4337       +1     
- Partials      569      572       +3     
Impacted Files Coverage Δ
tests/e2e/upgrade/govexec.go 0.00% <0.00%> (ø)
app/ante/evm/fees.go 73.19% <62.50%> (-1.53%) ⬇️
app/ante/cosmos/min_price.go 100.00% <100.00%> (ø)
app/ante/evm/fee_checker.go 93.93% <100.00%> (-2.80%) ⬇️

@GAtom22 GAtom22 marked this pull request as ready for review February 15, 2023 12:05
@GAtom22 GAtom22 requested a review from a team as a code owner February 15, 2023 12:05
@GAtom22 GAtom22 requested review from facs95, MalteHerrmann, fedekunze, 0a1c and Vvaradinov and removed request for a team February 15, 2023 12:05
Copy link
Contributor

@Vvaradinov Vvaradinov left a comment

Choose a reason for hiding this comment

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

tACK. Good work @GAtom22

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Thanks @GAtom22, left some comments regarding the UX of using the different combinations. I feel we should create a table with the different options for the 3 flags

server/flags/flags.go Outdated Show resolved Hide resolved
tests/e2e/e2e_test.go Show resolved Hide resolved
tests/e2e/e2e_test.go Outdated Show resolved Hide resolved
tests/e2e/e2e_test.go Show resolved Hide resolved
tests/e2e/e2e_test.go Outdated Show resolved Hide resolved
tests/e2e/e2e_test.go Outdated Show resolved Hide resolved
@satochi2017

This comment was marked as spam.

@GAtom22
Copy link
Contributor Author

GAtom22 commented Feb 21, 2023

Thanks @GAtom22, left some comments regarding the UX of using the different combinations. I feel we should create a table with the different options for the 3 flags

@fedekunze Added this comment on the ticket for clarification

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM

server/flags/flags.go Outdated Show resolved Hide resolved
GAtom22 and others added 2 commits February 21, 2023 16:48
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@fedekunze
Copy link
Contributor

@GAtom22 can you fix the Docker linter?

warning: Pin versions in apt get install. Instead of `apt-get install <package>` use `apt-get install <package>=<version>`

@fedekunze fedekunze enabled auto-merge (squash) February 22, 2023 15:08
@fedekunze fedekunze merged commit dda2f65 into main Feb 22, 2023
@fedekunze fedekunze deleted the GAtom22/imp-cli branch February 22, 2023 15:22
@linear linear bot mentioned this pull request Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants