[ENG-190] fees param: MinGasPrice #610

loredanacirstea · 2022-05-16T15:39:51Z

Description

Create a minimum gas price param in the x/fees module that rejects transactions with gas prices lower than the minimum.

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

PR review checkboxes:

I have...

added a relevant changelog entry to the Unreleased section in CHANGELOG.md
included the correct type prefix in the PR title
targeted the correct branch (see PR Targeting)
provided a link in the PR description to the relevant issue or specification
reviewed "Files changed" and left comments if necessary
confirmed all required CI checks have passed

Code maintenance:

I have...

written unit and integration tests
added relevant godoc and code comments.
updated relevant documentation (docs/) or specification (x/<module>/spec/)

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed all author checklist items have been addressed
confirmed that this PR does not change production code

* main: Correct inflation.proto typo (evmos#608) change algolia search key (evmos#607) Changing erroneous epoch skips to daily instead of weekly (evmos#554) fix: remove dup set claims record + CLI update (evmos#605) build(deps): bump github.com/spf13/cast from 1.4.1 to 1.5.0 (evmos#604) imp: fees test coverage (evmos#586) fees: limit address derivation to 20 iterations at registration (evmos#603)

codecov · 2022-05-16T15:47:15Z

Codecov Report

Merging #610 (7fd0cf1) into main (d29dd33) will decrease coverage by 3.03%.
The diff coverage is 87.75%.

❗ Current head 7fd0cf1 differs from pull request most recent head 93f2d93. Consider uploading reports for the commit 93f2d93 to get more accurate results

@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
- Coverage   85.71%   82.67%   -3.04%     
==========================================
  Files         117      122       +5     
  Lines        6390     6731     +341     
==========================================
+ Hits         5477     5565      +88     
- Misses        784     1027     +243     
- Partials      129      139      +10

Impacted Files	Coverage Δ
app/ante/handler_options.go	`2.50% <0.00%> (ø)`
x/fees/types/params.go	`89.28% <82.35%> (-1.90%)`	⬇️
app/ante/fees.go	`95.58% <95.58%> (ø)`
app/app.go	`84.81% <100.00%> (+0.11%)`	⬆️
app/ante/comission.go	`0.00% <0.00%> (ø)`
app/ante/vesting.go	`0.00% <0.00%> (ø)`
app/ante/ante.go	`9.67% <0.00%> (ø)`

x/fees/ante/eth.go

loredanacirstea · 2022-05-17T11:05:39Z

Notes:
This PR:

sets a global minimum gas price for Cosmos & EVM transactions.
checks MinGasPrice in DeliverTx, CheckTx and simulation mode in the AnteHandle
for Cosmos transactions, the MinGasPrice has priority over the local min-gas-prices flag that validators can individually set
for EVM, legacy transactions, the MinGasPrice must be higher than the tx gasPrice
for EVM, dynamic transactions MinGasPrice has priority over the feemarket formula. This means that if the feemarket formula returns a lower gas price than MinGasPrice, the transaction will be reverted in the AnteHandle. In this case, users can increase the GasTipCap until the effective price is higher than MinGasPrice. Ideally, the feemarket formula should not allow the price to go lower than MinGasPrice.
- this is a test for the above case: https://github.com/tharsis/evmos/blob/6c03861b6195828ca2034f70b8361bfde4b7c93e/x/fees/keeper/integration_test.go#L1018-L1022 fails, even though 1001 is bigger than MinGasPrice = 1000, because the base fee decreases. But https://github.com/tharsis/evmos/blob/6c03861b6195828ca2034f70b8361bfde4b7c93e/x/fees/keeper/integration_test.go#L1036-L1040 is successful.

fedekunze

Thanks @loredanacirstea. Additional comments and spec updates are needed in order to document the cases that involve the base fee / fee market.

x/fees/ante/eth.go

x/fees/types/params.go

x/fees/keeper/integration_test.go

x/fees/ante/eth.go

Co-authored-by: Federico Kunze Küllmer <[email protected]>

fedekunze

Thanks! Changes LGTM. We need to update the specification in order to reflect the new changes 👍

x/fees/ante/eth.go

Otherwise we get a `ERR ante handler panicked error="UnmarshalJSON cannot decode empty bytes"` error when trying to get fees & evm parameters from the stores in AnteHandle, when the gentx transactions are ran

Co-authored-by: Federico Kunze Küllmer <[email protected]>

fedekunze

LGTM, can you add a state machine breaking change?

app/ante/fees.go

app/ante/fees_test.go

x/fees/spec/01_concepts.md

x/fees/spec/03_state_transitions.md

Co-authored-by: Federico Kunze Küllmer <[email protected]>

evmos#610 (comment) evmos#610 (comment)

loredanacirstea · 2022-05-20T11:38:56Z

I tested on a local chain started with a min gas price of 2000000000aevmos - equivalent to 2 GWEI. And it worked as intended.

cat $HOME/.evmosd/config/genesis.json | jq '.app_state["fees"]["params"]["min_gas_price"]="2000000000"' > $HOME/.evmosd/config/tmp_genesis.json && mv $HOME/.evmosd/config/tmp_genesis.json $HOME/.evmosd/config/genesis.json

* fees: add minGasPrice module param * enforce fees param for min gas prices * fees - min gas prices ante handler * fix fees ante handler, add tests * Change DefaultMinGasPrice to 0 * fees - refactor integration test descriptions for clarity * fees: introduce EthMinPriceFeeDecorator and tests * fees - fix param test * fees - compare MinGasPrice with EffectivePrice for dynamic tx * test remove test * Reintroduce removed test with proper price values * Add comment * Apply suggestions from code review Co-authored-by: Federico Kunze Küllmer <[email protected]> * Fix inaccurate test comments, update evm AnteHandle explanation * Reorder genesis initialization Otherwise we get a `ERR ante handler panicked error="UnmarshalJSON cannot decode empty bytes"` error when trying to get fees & evm parameters from the stores in AnteHandle, when the gentx transactions are ran * Update x/fees/ante/eth.go error Co-authored-by: Federico Kunze Küllmer <[email protected]> * fees: fix error message in cosmos antehandle & tests * fees: be more specific with the init genesis ordering * Move MinPriceFeeDecorator to app/ante/fees.go (PR review) evmos/evmos#610 (comment) * Add unit tests for MinPriceFeeDecorators, remove redundant evm denom evmos/evmos#610 (comment) * Small fixes (PR review) * fees - update spec for MinGasPrice * s/MinPriceFeeDecorator/MinGasPriceDecorator * Apply suggestions from code review Co-authored-by: Federico Kunze Küllmer <[email protected]> * Apply suggestions from code review Co-authored-by: Federico Kunze Küllmer <[email protected]> * add change log * fees - handle error in EthMinGasPriceDecorator (PR review) evmos/evmos#610 (comment) evmos/evmos#610 (comment) * Test only DeliverTx app context Co-authored-by: Federico Kunze Küllmer <[email protected]>

loredanacirstea added 6 commits May 12, 2022 01:52

fees: add minGasPrice module param

41907a8

enforce fees param for min gas prices

a98dcca

fees - min gas prices ante handler

c2bc7f6

fix fees ante handler, add tests

bedb2b9

Change DefaultMinGasPrice to 0

5eea3bc

loredanacirstea added 4 commits May 16, 2022 18:53

fees - refactor integration test descriptions for clarity

cd27a79

Merge branch 'main' into loredana/ENG-190-fees-min-gas-price

64910ab

fees: introduce EthMinPriceFeeDecorator and tests

6e0a348

fees - fix param test

6a9be09

loredanacirstea marked this pull request as ready for review May 16, 2022 20:33

loredanacirstea requested review from danburck, fedekunze, khoslaventures and jolube as code owners May 16, 2022 20:33

fees - compare MinGasPrice with EffectivePrice for dynamic tx

ab12098

loredanacirstea commented May 17, 2022

View reviewed changes

x/fees/ante/eth.go Outdated Show resolved Hide resolved

loredanacirstea added 3 commits May 17, 2022 11:50

test remove test

c5d0b5d

Reintroduce removed test with proper price values

cc20135

Add comment

6c03861

fedekunze requested changes May 17, 2022

View reviewed changes

loredanacirstea and others added 2 commits May 17, 2022 21:36

Apply suggestions from code review

06527c1

Co-authored-by: Federico Kunze Küllmer <[email protected]>

Fix inaccurate test comments, update evm AnteHandle explanation

acf14a1

loredanacirstea marked this pull request as draft May 18, 2022 08:55

fedekunze reviewed May 18, 2022

View reviewed changes

x/fees/ante/eth.go Outdated Show resolved Hide resolved

fedekunze requested a review from ramacarlucho May 18, 2022 13:14

loredanacirstea and others added 2 commits May 18, 2022 16:20

Reorder genesis initialization

3637d9f

Otherwise we get a `ERR ante handler panicked error="UnmarshalJSON cannot decode empty bytes"` error when trying to get fees & evm parameters from the stores in AnteHandle, when the gentx transactions are ran

Update x/fees/ante/eth.go error

71d1e19

Co-authored-by: Federico Kunze Küllmer <[email protected]>

fedekunze mentioned this pull request May 20, 2022

Initial app.toml minimum-gas-price #593

Closed

fedekunze approved these changes May 20, 2022

View reviewed changes

loredanacirstea and others added 6 commits May 20, 2022 13:37

Apply suggestions from code review

9e01711

Co-authored-by: Federico Kunze Küllmer <[email protected]>

Apply suggestions from code review

1dd15ac

Co-authored-by: Federico Kunze Küllmer <[email protected]>

add change log

b84e1ff

fees - handle error in EthMinGasPriceDecorator (PR review)

e3db3c9

evmos#610 (comment) evmos#610 (comment)

Test only DeliverTx app context

1dbb90f

Merge branch 'main' into loredana/ENG-190-fees-min-gas-price

7fd0cf1

fedekunze requested a review from danburck May 20, 2022 14:21

Merge branch 'main' into loredana/ENG-190-fees-min-gas-price

93f2d93

fedekunze enabled auto-merge (squash) May 25, 2022 06:18

fedekunze merged commit 19df3bb into evmos:main May 25, 2022

loredanacirstea mentioned this pull request May 30, 2022

[ENG-190] global MinGasPrice evmos/ethermint#1104

Merged

11 tasks

fedekunze mentioned this pull request Jun 3, 2022

imp(fees): revert MinGasPrices param #647

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-190] fees param: MinGasPrice #610

[ENG-190] fees param: MinGasPrice #610

loredanacirstea commented May 16, 2022

codecov bot commented May 16, 2022 •

edited

Loading

loredanacirstea commented May 17, 2022

fedekunze left a comment

fedekunze left a comment

fedekunze left a comment

loredanacirstea commented May 20, 2022

[ENG-190] fees param: MinGasPrice #610

[ENG-190] fees param: MinGasPrice #610

Conversation

loredanacirstea commented May 16, 2022

Description

Reviewers Checklist

codecov bot commented May 16, 2022 • edited Loading

Codecov Report

loredanacirstea commented May 17, 2022

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

loredanacirstea commented May 20, 2022

codecov bot commented May 16, 2022 •

edited

Loading