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

bug(feemarket): set lower bound of base fee to min gas price param #1135

Merged
merged 19 commits into from
Jun 22, 2022
Merged

bug(feemarket): set lower bound of base fee to min gas price param #1135

merged 19 commits into from
Jun 22, 2022

Conversation

danburck
Copy link
Contributor

@danburck danburck commented Jun 17, 2022 •
edited
Loading

Description

This PR adds a lower bound to the baseFee with the min gas prices param on the feemarket. I also refactored the integration tests. Furthermore, I added concepts in the specs as I noticed that there has been confusion about fees and gas prices.

@linear
Copy link

linear bot commented Jun 17, 2022

ENG-473 Users can't sign Metamask txs with high Min Gas Price

Message to validators:

Hey Validators, here is a quick summary of what we’ve seen on the testnet,

Testnet Upgrade

The upgrade procedure was successful and now we have ~2s block times in avg! Mad props to all our testnet validators that made this possible.

UX issues

The new MinGasPrices parameter on testnet was set to the same value that we decided for mainnet, i.e 25,000,000,000. Because the difference is so high with the base fee on testnet (currently has a value of 7), the difference needs to be added in the priority tip field (in the case of EIP-1559 tx). The main issue is that the Metamask UI restricts this behavior by disabling the Send button when PriorityTip >>> BaseFee .

We’re going to create a parameter change proposal in the next hour to lower the MinGasPrice value on testnet so that EVM txs can go through.

What this means for Mainnet

In the meantime the core Evmos team is advising all Validators to vote NO on the proposal to upgrade mainnet so that we can upgrade without impacting UX on dApps.

These UX issues are resolved in the next 12-16hrs, we’ll recreate an upgrade proposal for mainnet.

@fedekunze fedekunze changed the title bug(feemarket): set lower bound of base fee to min gas price param) bug(feemarket): set lower bound of base fee to min gas price param Jun 17, 2022
@danburck danburck marked this pull request as ready for review June 21, 2022 17:51
fedekunze
fedekunze reviewed Jun 22, 2022
View reviewed changes
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, minor comments

CHANGELOG.md Outdated Show resolved Hide resolved
app/ante/fees.go Outdated Show resolved Hide resolved
x/feemarket/keeper/eip1559_test.go Outdated Show resolved Hide resolved
x/feemarket/spec/01_concepts.md Outdated Show resolved Hide resolved
x/feemarket/spec/01_concepts.md Outdated Show resolved Hide resolved
x/feemarket/spec/01_concepts.md Outdated Show resolved Hide resolved
x/feemarket/spec/01_concepts.md Show resolved Hide resolved
x/feemarket/spec/01_concepts.md Outdated Show resolved Hide resolved
x/feemarket/spec/01_concepts.md Outdated Show resolved Hide resolved
x/feemarket/spec/01_concepts.md Outdated Show resolved Hide resolved
crypto-facs
crypto-facs approved these changes Jun 22, 2022
View reviewed changes
Copy link
Contributor

@crypto-facs crypto-facs left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

x/feemarket/keeper/eip1559.go Show resolved Hide resolved
@danburck danburck enabled auto-merge (squash) June 22, 2022 10:54
@fedekunze fedekunze disabled auto-merge June 22, 2022 10:57
@fedekunze fedekunze merged commit d333341 into main Jun 22, 2022
@fedekunze fedekunze deleted the ENG-473-set-base-fee-lower-bound-to-min-gas-prices branch June 22, 2022 10:57
devon-chain pushed a commit to PundiAI/ethermint that referenced this pull request Jun 28, 2022
@danburck @fedekunze @devon-chain
bug(feemarket): set lower bound of base fee to min gas price param (e…
d2bee85 
…vmos#1135)

* bug(feemarket): set lower bound of base fee to min gas price param)

* fix

* bug(feemarket): flag necessary improvement to integration tests, as the baseFee changes for every test

* bug(feemarket): add unit tests for CalculateBaseFee

* bug(feemarket): move integration test setup out of Describe block

* wip fix tests

* bug(feemarket): fix integration tests

* bug(feemarket): wip improve specs

* bug(feemarket): add spec concepts

* bug(feemarket): remove todo

* bug(feemarket): remove changes used for debugging in params

* bug(feemarket): remove todo in integration test

* add changelog

* address PR comments

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crypto-facs crypto-facs crypto-facs approved these changes

@fedekunze fedekunze fedekunze approved these changes

@khoslaventures khoslaventures Awaiting requested review from khoslaventures

@jolube jolube Awaiting requested review from jolube

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danburck @fedekunze @crypto-facs