Skip to content

Conversation

@stevencartavia
Copy link

Motivation

closes #12222

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

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.

cool

some nits, suggestions

Comment on lines 337 to 339
// Convert BlobTransactionSidecarVariant to BlobTransactionSidecar for TxEnvelope
let converted_tx = match tx {
TxEip4844Variant::TxEip4844(tx) => TxEip4844Variant::TxEip4844(tx),
Copy link
Member

Choose a reason for hiding this comment

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

instead of converting, we should also update the return type here

https://github.com/foundry-rs/foundry/blob/master/crates/anvil/core/src/eth/transaction/mod.rs#L294-L294

which is

pub struct Transaction<T = TxEnvelope> {

Comment on lines 3307 to 3312
let blobs = match &sidecar.sidecar {
BlobTransactionSidecarVariant::Eip4844(sidecar) => &sidecar.blobs,
BlobTransactionSidecarVariant::Eip7594(sidecar) => &sidecar.blobs,
};
return Ok(Some(blobs.clone()));
}
Copy link
Member

Choose a reason for hiding this comment

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

could you upstream this as

fn blobs

and

fn into_blobs

Comment on lines 1064 to 1066
Self::Legacy(tx) => TxEnvelope::from(tx.clone()).encode_2718_len(),
Self::EIP2930(tx) => TxEnvelope::from(tx.clone()).encode_2718_len(),
Self::EIP1559(tx) => TxEnvelope::from(tx.clone()).encode_2718_len(),
Copy link
Member

Choose a reason for hiding this comment

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

I think even those from calls here are redundant,

if you want, please submit this separately

Comment on lines +35 to +40
pub trait BlobTransactionSidecarVariantExt {
/// Get a reference to the blobs
fn blobs(&self) -> &[alloy_consensus::Blob];

/// Consume self and return the blobs
fn into_blobs(self) -> Vec<alloy_consensus::Blob>;
Copy link
Member

@mattsse mattsse Oct 24, 2025

Choose a reason for hiding this comment

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

I meant adding this natively to alloy by sending a pr there :)

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Add support for 7594 blobs to anvil

3 participants