-
Notifications
You must be signed in to change notification settings - Fork 566
eth_call/eth_estimateGas don't need to pass base fee externally #671
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes LGTM, one question only. Can you also fix the linter issue and add a changelog entry?
x/evm/keeper/grpc_query.go
Outdated
|
||
// ignore base fee if not enabled by fee market params | ||
if !feemktParams.NoBaseFee { | ||
if types.IsLondon(ethCfg, ctx.BlockHeight()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we not checking the fee market param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I’m not sure what’s standard way to check for this, I copied the logic from existing code, should we check both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from what I have compiled so far
For not a london block -> block base fee should be nil, tx basefee should also be nil
For london block
- if its under market fee
activation height
ornobasefee
is true -> block base fee is equal tomarketfee.basefee
. Should we support EIP-1559 and usemarketfee.basefee
value or should we ignore it? - if its above
activation height
andnobasefee
is false -> block base fee is dynamically computed and tx base fee should usespreviousheader.basefee
If under activation height or no base fee means that we should use static value marketfee.basefee then just checking IsLondon is enough there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/tharsis/ethermint/blob/main/x/evm/keeper/state_transition.go#L164
Is this code correct? it's the same as here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I noticed that some part of the code does not follow the same logic:
https://github.com/tharsis/ethermint/blob/main/app/ante/eth.go#L362
I think we need some clarification @fedekunze about the spec for this baseFee
London not activated => baseFee value?
London activated, FeeMarket not enabled or height activation not reached = baseFee value?
London activated, FeeMarket enabled and height activation reached = baseFee value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea of having the parameter was to make the module available for other chains too. Maybe we could remove the feemarket parameters and just have an imported module interface that has a single IsBaseFeeEnabled(ctx)
func.
I can create an issue for it, so you can ignore it for this PR
@@ -319,7 +308,10 @@ func (k Keeper) EstimateGas(c context.Context, req *types.EthCallRequest) (*type | |||
return nil, status.Error(codes.Internal, err.Error()) | |||
} | |||
|
|||
baseFee := req.GetBaseFee() | |||
var baseFee *big.Int | |||
if types.IsLondon(ethCfg, ctx.BlockHeight()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto fee market param check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, pending lint fix and changelog entry. I can remove the fee market params in another PR
Solution: - load fee directly from state changelog
Codecov Report
@@ Coverage Diff @@
## main #671 +/- ##
==========================================
+ Coverage 57.23% 57.38% +0.15%
==========================================
Files 64 64
Lines 5596 5583 -13
==========================================
+ Hits 3203 3204 +1
+ Misses 2228 2215 -13
+ Partials 165 164 -1
|
done |
Solution:
Description
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)