-
Notifications
You must be signed in to change notification settings - Fork 759
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
block: Add data gas used and refactor cal excess data gas cal #2750
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
const blobs = getBlobs('hello world') | ||
const commitments = blobsToCommitments(blobs) | ||
const versionedHashes = commitmentsToVersionedHashes(commitments) | ||
const proofs = blobsToProofs(blobs, commitments) |
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.
Do you think it would be worthwhile creating a generic test utility method for creating blob transactions (like here but more generic)? Or maybe we update the static constructor to let you pass in the data for one or more blobs and it generates all of it? It's just a lot of boilerplate to create these blob transactions (the more I think about it).
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 thats a very good idea to have a static method in the tx OR
we can modify fromTxData to just accept blobs string[] and just generate the rest?
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 thats a very good idea to have a static method in the tx OR we can modify fromTxData to just accept blobs string[] and just generate the rest?
Yup, that's more or less what I was thinking. Let's tackle as a separate PR though. Not crucial for this one.
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.
Left one additional comment (just cleaning up n/a docs) but this looks good!
bcf6441
to
92b6277
Compare
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. Lets merge pending CI passing.
Great PR overall! 😃🎉❤️ |
Implements
data_gas_used
to header ethereum/EIPs#7062excess_data_gas
to 64 bit ethereum/EIPs#7095as part of devnet 6 target spec