-
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: handle Isthmus operator fee params #14243
Conversation
/// The current operator fee scalar. | ||
pub operator_fee_scalar: Option<u128>, | ||
/// The current operator fee constant. | ||
pub operator_fee_constant: Option<u128>, |
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.
will those be added to the rpc receipt?
of not we can ignore those 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.
Yes they are added
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 okay, then we need those here:
unclear here, because this file uses the revm l1blockinfo type reth/crates/optimism/evm/src/l1.rs Lines 11 to 14 in 8f59efb
|
@mattsse the place where L1BlockInfo from op-alloy-rpc-types is used is here:
|
/// The current operator fee scalar. | ||
pub operator_fee_scalar: Option<u128>, | ||
/// The current operator fee constant. | ||
pub operator_fee_constant: Option<u128>, |
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 okay, then we need those here:
@mattsse I was thinking the same, but the PR doing that has been closed with this comment:
So I guess we need to move away from op-alloy-rpc-types to maili? Happy to work on it if so. cc @refcell |
no we need these additional fields on the rpc type if these should be included in the rpc receipt response: we don't need any maili here |
a3de220
to
702afcf
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,
we can do the rpc fields separately
} | ||
})?; | ||
|
||
let mut l1block = L1BlockInfo::default(); |
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.
btw would
L1BlockInfo {
l1_base_fee,...
..Default::default()
}
also work here,
this type is a bit strange because it has a single private field for some reason -.-
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.
My best bet would be a mistake during a conflict resolution
<!-- 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 We need to show operator fee new params in RPC receipts. Close #419 Ref paradigmxyz/reth#14243 <!-- Explain the context and why you're making that change. What is the problem you're trying to solve? In some cases there is not a problem and this can be thought of as being the motivation for your change. --> ## Solution <!-- Summarize the solution and provide any necessary context needed to understand the code change. --> ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes
For Isthmus we added 2 new fields in the L1Block:
operatorFeeScalar
andoperatorFeeConstant
.This PR handle them.
Currently build fails because
L1BlockInfo
fromop-alloy-rpc-types
doesn't have these 2 fields. So I think we need to switch to maili with this.