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

Blob data gas refunds #111

Merged
merged 10 commits into from
Jan 19, 2024
Merged

Blob data gas refunds #111

merged 10 commits into from
Jan 19, 2024

Conversation

yahgwai
Copy link
Contributor

@yahgwai yahgwai commented Jan 17, 2024

Now refunds if there were blobs on the transaction

@yahgwai yahgwai requested a review from gzeoneth January 17, 2024 15:47
@cla-bot cla-bot bot added the s label Jan 17, 2024
src/bridge/GasRefunder.sol Outdated Show resolved Hide resolved
src/bridge/GasRefunder.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM. I'd consider unifying the blob basefee reader contract with the data hash reader contract but that's mostly separate.

@gzeoneth gzeoneth changed the base branch from 4844-int-opt to 4844-only January 18, 2024 08:36
Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

LGTM

Formatting

Updates for code review

Remove whitespace

Formatting
@gzeoneth
Copy link
Member

Sorry for the force-push spam, should be good now.

src/libraries/GasRefundEnabled.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@shotaronowhere shotaronowhere left a comment

Choose a reason for hiding this comment

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

As gzeon mentioned, gas refunder in addSequencerL2BatchFromOrigin should be updated, but there is a stack too deep error with extra arguments for the modifier.

Plausibly refundsGas(x,y,z) could be refactored to refundsGas(x, bool refundBlob) or simply refundsGas(x), refundsGasWithBlob(x)

@gzeoneth gzeoneth merged commit a8e7709 into 4844-only Jan 19, 2024
5 checks passed
@gzeoneth gzeoneth deleted the 4844-gas-refunder branch January 19, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants