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

feat: extend FeeHistory type with eip-4844 fields #188

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

allnil
Copy link
Contributor

@allnil allnil commented Feb 6, 2024

Related to reth task: paradigmxyz/reth#6330
according to: ethereum/execution-apis#486

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 50 to 55
/// of the returned range, because this value can be derived from the newest block. Zeroes
/// are returned for pre-EIP-4844 blocks.
pub base_fee_per_blob_gas: Vec<U256>,
/// An array of block blob gas used ratios. These are calculated as the ratio of gasUsed and
/// gasLimit.
pub blob_gas_used_ratio: Vec<f64>,
Copy link
Member

Choose a reason for hiding this comment

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

these need to be option for now, because only supported after cancun
and skip if none

Copy link
Member

Choose a reason for hiding this comment

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

Should be enough: #[serde(default, skip_serializing_if = "Vec::is_empty")]

Copy link
Member

Choose a reason for hiding this comment

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

these need to be option for now, because only supported after cancun
and skip if none

The spec says just to fill with 0s for pre-cancun blocks, would this not be ok even with cancun not activated?

Copy link
Member

@DaniPopes DaniPopes Feb 6, 2024

Choose a reason for hiding this comment

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

That's if the network and RPC provider implements the EIP/Cancun

Copy link
Member

Choose a reason for hiding this comment

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

right let's do

#[serde(default, skip_serializing_if = "Vec::is_empty")]

then @allnil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's both for base_fee_per_blob_gas and blob_gas_used_ratio or only for blob_gas_used_ratio?

Copy link
Member

Choose a reason for hiding this comment

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

both!

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Failures unrelated

@DaniPopes DaniPopes merged commit 48f1c0f into alloy-rs:main Feb 6, 2024
10 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants