Calculate fees correctly based on zero/non-zero bytes#540
Conversation
🦋 Changeset detectedLatest commit: bed9dc6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tynes
left a comment
There was a problem hiding this comment.
Left some comments and questions
|
Noting this for other reviewers: the correct spelling for the plural of "zero" is both "zeros" and "zeroes" |
As long as we charge slightly more than the cost of submitting the batch + running the hardware, things should be fine. The profits will go towards funding public goods |
044498d to
8769e26
Compare
l2geth/core/rollup_fee.go
Outdated
There was a problem hiding this comment.
Confirming the implementation:
4 * zeroDataBytes is zeroesCost
16 * (nonZeroDataBytes + RollupBaseTxSize) is onesCost
Their sum (4 * zeroDataBytes + 16 * (nonZeroDataBytes + RollupBaseTxSize)) is dataCost
(4 * zeroDataBytes + 16 * (nonZeroDataBytes + RollupBaseTxSize)) * dataPrice is dataFee
executionPrice * gasUsed is executionFee
And finally,
(4 * zeroDataBytes + 16 * (nonZeroDataBytes + RollupBaseTxSize)) * dataPrice + executionPrice * gasUsed is fee
The math looks good to me
l2geth/core/rollup_fee_test.go
Outdated
There was a problem hiding this comment.
Should this use the constants as well? Or intentionally covering the fact that the constants may change
There was a problem hiding this comment.
Yeah this was intentional to avoid regressions if anything else changes (e.g. the 96 once RLP is in)
8769e26 to
bed9dc6
Compare
tynes
left a comment
There was a problem hiding this comment.
Looks good to me pending passing integration tests. Using the exact values in the integration tests is a double edged sword, especially if they were not calculated out of band
|
I agree but IMO we should try pinning down certain values as regression tests. If we see this becoming a pain in the future (I don't think it will) we can consider reverting, but IMO net good now. They were calculated by |
* test: pin down gas estimation for transfers * fix(l2geth): charge fees differently for zero/non-zero bytes of data * test: update estimated gas values * chore: gofmt & changeset * fix(l2geth): calculate zeros correctly
* checkpoint * checkpoint * compiling + passing tests * rename error * lint * fix test-utils feature * add resets * codecov * clean * update codecov rules * upload test results * nextest cfg
<!-- Thank you for your Pull Request. Please provide a description above and review the requirements below. Bug fixes and new features should include tests. Contributors guide: https://github.com/alloy-rs/core/blob/main/CONTRIBUTING.md The contributors guide includes instructions for running rustfmt and building the documentation. --> <!-- ** Please select "Allow edits from maintainers" in the PR Options ** --> ## Motivation on this page is bug - https://alloy-rs.github.io/op-alloy/starting.html  ## Solution due to this line https://github.com/alloy-rs/op-alloy/blob/main/book/src/links.md?plain=1#L10 we need to do this change. ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes
Fixes #515.
Does not introduce
nonCallDataL1GasOverheadsince we do not have a standard way to estimate / calculate it yet and seems to be a micro optimization.