-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: add sanity tests for Args Default impls #5660
Conversation
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,
can do constants issue as good first issue
blocks: Some(20), | ||
ignore_price: Some(2), | ||
max_price: Some(500000000000), | ||
percentile: Some(60), |
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.
let's open an issue to replace those with constants
extradata: default_extradata(), | ||
max_gas_limit: 30_000_000, | ||
interval: Duration::from_secs(1), | ||
deadline: Duration::from_secs(12), | ||
max_payload_tasks: 3, | ||
#[cfg(feature = "optimism")] | ||
compute_pending_block: false, |
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.
we should use the config's default here
reth/crates/payload/basic/src/lib.rs
Lines 306 to 321 in 5ac4a3d
impl Default for BasicPayloadJobGeneratorConfig { | |
fn default() -> Self { | |
let mut extradata = BytesMut::new(); | |
RETH_CLIENT_VERSION.as_bytes().encode(&mut extradata); | |
Self { | |
extradata: extradata.freeze().into(), | |
max_gas_limit: ETHEREUM_BLOCK_GAS_LIMIT, | |
interval: Duration::from_secs(1), | |
// 12s slot time | |
deadline: SLOT_DURATION, | |
max_payload_tasks: 3, | |
#[cfg(feature = "optimism")] | |
compute_pending_block: false, | |
} | |
} | |
} |
ref #5658 (review)
added tests accordingly, this was a very good idea, since the
GasPriceOracleArgs::default
impl just set everyting toNone