Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

BaseFee based on GasWanted #1105

Merged
merged 35 commits into from
Jun 5, 2022
Merged

Conversation

crypto-facs
Copy link
Contributor

Closes: #XXX

Description

This PR aims to fix a design issue on our current BaseFee calculation by the FeeMarket. Our BaseFee is currently reacting to network congestion based on how much gasUsed did the parent block had. We are storing this value in the GenesisState of the FeeMarket module for it to be used on the child block and calculate the new BaseFee based on that (increase or decrease). This does not reflect the network congestion on a Cosmos SDK chain as blocks are actually filled based on gasWanted (aka GasLimit) independently from how much we refund the user after how much gasUsed did the tx required.

Based on this, a solution had been discussed where we base this reaction on a total gasWanted value per block that is tracked on the FeeMarket TransientStore for it to be stored on EndBlock on the GenesisState of the FeeMarket module for the child block to calculate BaseFee on it.

The was no need to changed the GenesisState BlockGas property name, just the way it was calculated.

NOTE: I am missing some tests to make sure the BaseFee is being calculated correctly.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alexanderbez alexanderbez self-requested a review June 1, 2022 18:57
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

I can't speak for the changes in app/ante/eth.go, but otherwise this looks like the correct approach 👍

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.

Left some comments, there are also some tests failing 👍

Can you update the specification to reflect the new changes?

CHANGELOG.md Outdated Show resolved Hide resolved
app/ante/eth.go Outdated Show resolved Hide resolved
app/ante/eth.go Outdated Show resolved Hide resolved
app/ante/eth.go Outdated Show resolved Hide resolved
app/ante/fee_market.go Outdated Show resolved Hide resolved
app/ante/fee_market_test.go Show resolved Hide resolved
x/feemarket/keeper/abci.go Outdated Show resolved Hide resolved
crypto-facs and others added 2 commits June 3, 2022 11:16
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #1105 (240ac4c) into main (8155e1f) will increase coverage by 0.42%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1105      +/-   ##
==========================================
+ Coverage   61.36%   61.78%   +0.42%     
==========================================
  Files          88       88              
  Lines        7213     7257      +44     
==========================================
+ Hits         4426     4484      +58     
+ Misses       2563     2553      -10     
+ Partials      224      220       -4     
Impacted Files Coverage Δ
x/evm/keeper/migrations.go 100.00% <ø> (+33.33%) ⬆️
app/ante/fee_market.go 86.36% <86.36%> (ø)
app/ante/handler_options.go 80.00% <100.00%> (+0.83%) ⬆️
app/app.go 85.55% <100.00%> (ø)
x/evm/keeper/keeper.go 79.11% <100.00%> (+0.40%) ⬆️
x/evm/keeper/state_transition.go 75.45% <100.00%> (+0.07%) ⬆️
x/evm/keeper/utils.go 67.60% <100.00%> (ø)
x/evm/module.go 53.73% <100.00%> (+0.87%) ⬆️
x/evm/simulation/genesis.go 93.54% <100.00%> (ø)
x/evm/types/params.go 100.00% <100.00%> (+6.52%) ⬆️
... and 7 more

rpc/types/utils.go Outdated Show resolved Hide resolved
app/ante/fee_market_test.go Show resolved Hide resolved
x/feemarket/migrations/v011/migrate.go Outdated Show resolved Hide resolved
x/feemarket/migrations/v011/migrate.go Outdated Show resolved Hide resolved
x/feemarket/keeper/abci.go Outdated Show resolved Hide resolved
x/evm/keeper/keeper.go Show resolved Hide resolved
x/feemarket/types/params.go Outdated Show resolved Hide resolved
x/feemarket/types/params.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Show resolved Hide resolved
x/feemarket/types/params.go Show resolved Hide resolved
crypto-facs and others added 4 commits June 4, 2022 19:48
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@fedekunze fedekunze enabled auto-merge (squash) June 5, 2022 09:17
@fedekunze fedekunze merged commit 620f6a6 into evmos:main Jun 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants