-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: add eip 4844 blob tx type #3807
feat: add eip 4844 blob tx type #3807
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice,
we need to get rid of the clippy warning by making exporting it from the crate, see other tx type for ref
It'd be great to get some test cases from geth as well, there should exist some
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smol nit
mem::size_of::<u128>() + // value | ||
self.access_list.size() + // access_list | ||
self.input.len() + // input | ||
self.blob_hashes.len() * mem::size_of::<H256>() + // blob hashes size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be capacity since it's a Vec
self.blob_hashes.len() * mem::size_of::<H256>() + // blob hashes size | |
self.blob_hashes.capacity() * mem::size_of::<H256>() + // blob hashes size |
Codecov Report
... and 10 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
#[inline] | ||
pub fn size(&self) -> usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rjected should we make this a trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we should make this a trait, will open an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Fixes #3746