-
Notifications
You must be signed in to change notification settings - Fork 631
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
EIP-7516: BLOBBASEFEE opcode #721
Conversation
Waiting for on possible naming change: https://github.com/ethereum/go-ethereum/pull/28098/files#r1327222175 |
Co-authored-by: DaniPopes <[email protected]>
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, just one question about the name
crates/primitives/src/env.rs
Outdated
/// Takes `blob_excess_gas` saves it inside env | ||
/// and calculates `blob_fee` with [`BlobGasAndFee`]. | ||
pub fn set_blob_gas_and_fee(&mut self, excess_blob_gas: u64) { | ||
self.blob_gas_and_fee = Some(BlobGasAndFee::new(excess_blob_gas)); | ||
} |
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.
I like this, makes it very easy to set both when initializing the env
@@ -264,7 +265,7 @@ pub const OPCODE_JUMPMAP: [Option<&'static str>; 256] = [ | |||
/* 0x47 */ Some("SELFBALANCE"), | |||
/* 0x48 */ Some("BASEFEE"), | |||
/* 0x49 */ Some("BLOBHASH"), | |||
/* 0x4a */ None, | |||
/* 0x4a */ Some("BLOBFEE"), |
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.
does it matter that the opcode is called BLOBBASEFEE
in the spec here?
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.
This: #721 (comment)
I like BLOBFEE 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.
ah I missed that, yeah I like BLOBFEE
more too
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.
Renamed back to BLOBBASEFEE
per Eth R&D discussion
@Rjected can you review, had some small (name) changes, it is ready to be merged |
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!
Cancun: https://eips.ethereum.org/EIPS/eip-7516
add Blobfee opcode