Skip to content
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

Merged
merged 3 commits into from May 10, 2024
Merged

fix estimateGas #3416

merged 3 commits into from May 10, 2024

Conversation

acolytec3
Copy link
Contributor

No description provided.

@@ -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'
Copy link
Member

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 🤔

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Member

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 😄 )

@jochem-brouwer
Copy link
Member

I have further cleaned up some default values (the estimateGas tests were failing) such that the right base fee is used on the block.

Copy link
Member

@jochem-brouwer jochem-brouwer left a 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? 😄

@acolytec3 acolytec3 merged commit 79a3316 into master May 10, 2024
33 of 34 checks passed
@acolytec3 acolytec3 deleted the fix-estimateGas branch May 10, 2024 22:06
Copy link
Contributor

@g11tech g11tech left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants