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

Disable basefee for non london block #662

Merged
merged 6 commits into from
Oct 13, 2021

Conversation

thomas-nguy
Copy link
Contributor

Closes: #XXX

Description

Unifying the logic between the rpc endpoint "GasPrice", "EstimateGas", "SendTransaction" and "GetBlock"


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)

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #662 (8f96fec) into main (c7a2fb9) will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #662      +/-   ##
==========================================
+ Coverage   57.00%   57.02%   +0.01%     
==========================================
  Files          63       63              
  Lines        5540     5547       +7     
==========================================
+ Hits         3158     3163       +5     
- Misses       2219     2221       +2     
  Partials      163      163              
Impacted Files Coverage Δ
app/ante/eth.go 87.08% <0.00%> (ø)
x/evm/keeper/state_transition.go 66.76% <50.00%> (-0.21%) ⬇️
x/evm/types/params.go 100.00% <100.00%> (ø)

@thomas-nguy thomas-nguy force-pushed the thomas/fix-rpc-basefee-for-fork branch 2 times, most recently from 1831217 to 2b21f00 Compare October 12, 2021 09:01
app/ante/eth.go Outdated
@@ -359,7 +359,7 @@ func (ctd CanTransferDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate
}

var baseFee *big.Int
if !feeMktParams.NoBaseFee {
if evmtypes.IsLondon(ctx, ctd.evmKeeper.GetParams(ctx), ctd.evmKeeper.ChainID()) && !feeMktParams.NoBaseFee {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can pass the params directly instead

Suggested change
if evmtypes.IsLondon(ctx, ctd.evmKeeper.GetParams(ctx), ctd.evmKeeper.ChainID()) && !feeMktParams.NoBaseFee {
if evmtypes.IsLondon(ctx, params, ethCfg.ChainID) && !feeMktParams.NoBaseFee {

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto for the others

Comment on lines 872 to 884
func (e *EVMBackend) getBaseFee(height int64) (*big.Int, error) {
cfg := e.ChainConfig()
if !cfg.IsLondon(new(big.Int).SetInt64(height)) {
return nil, nil
}

baseFee, err := e.BaseFee()
if err != nil {
return nil, err
}

return baseFee, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please update the BaseFee function instead

"math/big"
)

func IsLondon(ctx sdk.Context, params Params, chainID *big.Int) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add use the ethereum ChainConfig instead?

Copy link
Contributor Author

@thomas-nguy thomas-nguy Oct 13, 2021

Choose a reason for hiding this comment

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

it uses github.com/ethereum/go-ethereum/params now. Is it the one you are talking about?

@thomas-nguy thomas-nguy force-pushed the thomas/fix-rpc-basefee-for-fork branch 4 times, most recently from e1977c3 to 3edc9a8 Compare October 13, 2021 07:15
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.

Minor changes requested

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
rpc/ethereum/backend/backend.go Outdated Show resolved Hide resolved
rpc/ethereum/namespaces/eth/api.go Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/types/eip1559.go Outdated Show resolved Hide resolved
x/evm/types/eip1559_test.go Outdated Show resolved Hide resolved
@thomas-nguy thomas-nguy force-pushed the thomas/fix-rpc-basefee-for-fork branch from 661eb5f to 7f1b7ee Compare October 13, 2021 12:52
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.

ACK, pending Changelog entry and lint fix

@fedekunze fedekunze merged commit 75d5536 into evmos:main Oct 13, 2021
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.

2 participants