New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix estimateGas #3416
fix estimateGas #3416
Conversation
@@ -28,7 +29,7 @@ import type { EthereumClient } from '../../index.js' | |||
import type { EthProtocol } from '../../net/protocol/index.js' | |||
import type { FullEthereumService, Service } from '../../service/index.js' | |||
import type { RpcTx } from '../types.js' | |||
import type { Block, JsonRpcBlock } from '@ethereumjs/block' | |||
import type { JsonRpcBlock } from '@ethereumjs/block' |
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.
How come this is added but nothing is done with it in this PR? I don't see anything in the diff 🤔
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.
It should have moved block from a type import to a regular import
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.
Oh right yes this makes sense, thanks
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.
(its actually in the diff 😅 - time to call it a day I guess 😄 )
I have further cleaned up some default values (the estimateGas tests were failing) such that the right base fee is used on the block. |
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.
I will approve here, but before merging could you check my further changes, and also check that we can now still run kurtosis with tx-fuzz? 😄
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.
post merge review lgtm
No description provided.