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(consensus): add extra EIP-4844 types needed #229

Merged
merged 24 commits into from
Feb 27, 2024
Merged

Conversation

Evalir
Copy link
Contributor

@Evalir Evalir commented Feb 22, 2024

Motivation

We still need to add a few EIP-4844 types to be able to properly send and decode full EIP-4844 txs with a sidecar. Right now we can't do that, and the TxEip4844 contained in the envelope is incomplete, as it misses the sidecar.

Solution

Add the needed types. Closes #231

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@Evalir Evalir changed the title [WIP] feat(consensus): add extra EIP-4844 types needed feat(consensus): add extra EIP-4844 types needed Feb 23, 2024
@Evalir Evalir marked this pull request as ready for review February 23, 2024 17:37
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.

the enum approach is the most straight forward one imo.

though we still want an actual type for the 4844+blob variant

crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved
crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved
crates/consensus/src/transaction/envelope.rs Outdated Show resolved Hide resolved
crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved
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.

the cfg on the type itself is not pretty.

I think we should try to limit kzg feature to validation.
we should be able to use primitive types for the blob fields instead and not kzg types here

crates/consensus/Cargo.toml Outdated Show resolved Hide resolved
@Evalir Evalir requested a review from mattsse February 24, 2024 15:49
crates/eips/src/eip4844.rs Outdated Show resolved Hide resolved
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.

last nits

now that we're limiting c-kzg to validation, we can make this no default :)

crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved
crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved
@Evalir Evalir requested a review from mattsse February 24, 2024 16:21
@Evalir Evalir requested a review from mattsse February 24, 2024 16:26
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,

though maybe we can find a better name than Wrapper, maybe Variant?

pending @DaniPopes

@mattsse
Copy link
Member

mattsse commented Feb 27, 2024

@Evalir let's rename to Variant then send it

@Evalir
Copy link
Contributor Author

Evalir commented Feb 27, 2024

clippy failure unrelated.

@Evalir Evalir merged commit 7b35f83 into main Feb 27, 2024
13 of 14 checks passed
@Evalir Evalir deleted the evalir/4844-sidecar branch February 27, 2024 12:59
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.

How to properly encode and submit en eip-4844 blob transaction with TxEip4844 ?
3 participants