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

eip4844: use alloy TxEip4844 #10624

Merged
merged 22 commits into from
Sep 1, 2024
Merged

eip4844: use alloy TxEip4844 #10624

merged 22 commits into from
Sep 1, 2024

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented Aug 30, 2024

Related #9484

@klkvr
Copy link
Collaborator

klkvr commented Aug 30, 2024

@tcoratger mind pointing out where exactly do we need encode_fields in reth? it is pretty much internal alloy utility function, so making it pub would likely mean we just need additional specialized fn

@klkvr
Copy link
Collaborator

klkvr commented Aug 30, 2024

If it's here

let blob_tx_header = Header { list: true, payload_length };
// Encode the blob tx header first
blob_tx_header.encode(out);
// Encode the inner tx list header, then its fields
tx_header.encode(out);
self.transaction.encode_fields(out);
// Encode the signature
self.signature.encode(out);
then I think this is just self.transaction.encode_with_signature_fields(self.signature, out)?

@tcoratger
Copy link
Contributor Author

@klkvr nice just fixed

@tcoratger tcoratger marked this pull request as ready for review August 30, 2024 13:00
@klkvr klkvr mentioned this pull request Aug 30, 2024
1 task
@emhane emhane requested a review from klkvr August 30, 2024 21:35
crates/storage/codecs/src/alloy/eip4844.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

lgtm, pending conflicts caused by #10262

Co-authored-by: joshieDo <93316087+joshieDo@users.noreply.github.com>
Copy link
Collaborator

@joshieDo joshieDo left a comment

Choose a reason for hiding this comment

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

tested on a local db and decoded existing txes fine 👍

@klkvr klkvr added this pull request to the merge queue Sep 1, 2024
Merged via the queue into paradigmxyz:main with commit d4cfb95 Sep 1, 2024
34 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