feat: add eip1559 settings to chainconfig #99
Conversation
|
Claude finished @tac0turtle's task —— View job Code Review CompletedSummaryThis PR adds valuable EIP-1559 configuration capabilities to ev-reth's chainspec, enabling custom networks to optimize gas pricing for high-frequency block production (100ms). The implementation is generally well-structured with good validation and testing, though I've identified some critical bugs and areas for improvement. Critical Issues to Fix🔴 Bug in GenerateFeeVaultAlloc.s.sol (lines 135-141, 169-182)The Solidity script has a critical JSON generation bug. Fix required: // Remove hardcoded "0x" prefixes:
console.log(' "0x0": "%s",', vm.toString(slot0)); // not "0x%s"
// And in the JSON concatenation:
'","0x0":"',
vm.toString(slot0), // not "0x" + vm.toString(slot0)Code Quality & Architecture✅ Strengths
|
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the configurability of the 'ev-reth' node by allowing custom EIP-1559 parameters to be specified in the chainspec. This feature is crucial for tailoring fee market behavior to specific network requirements, especially for high-throughput chains with rapid block times. Additionally, it provides a new utility for simplified genesis deployment of the 'FeeVault' contract, streamlining network setup. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for custom networks by allowing EIP-1559 parameters to be configured via the chainspec. The implementation is well-structured, with clear validation and comprehensive tests. I've provided a minor refactoring suggestion for the new Rust code to improve its conciseness. Additionally, I've identified and suggested fixes for a bug in the new GenerateFeeVaultAlloc.s.sol script that causes it to generate invalid JSON. The accompanying documentation updates are thorough and well-written.
| fn apply_chain_spec_overrides( | ||
| chain_spec: &mut ChainSpec, | ||
| overrides: &EvolveEip1559Config, | ||
| ) -> Result<()> { | ||
| if let Some(denominator) = overrides.base_fee_max_change_denominator { | ||
| if denominator == 0 { | ||
| bail!("baseFeeMaxChangeDenominator must be greater than 0"); | ||
| } | ||
| } | ||
|
|
||
| if let Some(elasticity) = overrides.base_fee_elasticity_multiplier { | ||
| if elasticity == 0 { | ||
| bail!("baseFeeElasticityMultiplier must be greater than 0"); | ||
| } | ||
| } | ||
|
|
||
| if !overrides.has_base_fee_overrides() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let mut params = chain_spec.base_fee_params_at_timestamp(chain_spec.genesis.timestamp); | ||
|
|
||
| if let Some(denominator) = overrides.base_fee_max_change_denominator { | ||
| params.max_change_denominator = denominator as u128; | ||
| } | ||
|
|
||
| if let Some(elasticity) = overrides.base_fee_elasticity_multiplier { | ||
| params.elasticity_multiplier = elasticity as u128; | ||
| } | ||
|
|
||
| chain_spec.base_fee_params = BaseFeeParamsKind::Constant(params); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The validation for base_fee_max_change_denominator and base_fee_elasticity_multiplier is performed at the beginning of the function, and then the values are unwrapped again for assignment. This can be made more concise by moving the validation checks inside the if let Some(...) blocks where the overrides are applied. This refactoring would reduce redundancy and improve code clarity.
fn apply_chain_spec_overrides(
chain_spec: &mut ChainSpec,
overrides: &EvolveEip1559Config,
) -> Result<()> {
if !overrides.has_base_fee_overrides() {
return Ok(());
}
let mut params = chain_spec.base_fee_params_at_timestamp(chain_spec.genesis.timestamp);
if let Some(denominator) = overrides.base_fee_max_change_denominator {
if denominator == 0 {
bail!("baseFeeMaxChangeDenominator must be greater than 0");
}
params.max_change_denominator = denominator as u128;
}
if let Some(elasticity) = overrides.base_fee_elasticity_multiplier {
if elasticity == 0 {
bail!("baseFeeElasticityMultiplier must be greater than 0");
}
params.elasticity_multiplier = elasticity as u128;
}
chain_spec.base_fee_params = BaseFeeParamsKind::Constant(params);
Ok(())
}
damiannolan
left a comment
damiannolan
left a comment
There was a problem hiding this comment.
LGTM, think the gemini comments should be checked before merge and maybe clarify what ntia means exactly in the docs (decimals etc)
| use std::sync::Arc; | ||
|
|
||
| /// Chains supported by ev-reth. First value should be used as the default. | ||
| pub const SUPPORTED_CHAINS: &[&str] = &["mainnet", "sepolia", "holesky", "hoodi", "dev"]; |
There was a problem hiding this comment.
could use an enum + FromStr impl here but not necessary I guess...
#[derive(Debug, Clone, Copy)]
pub enum SupportedChain {
Mainnet,
Sepolia,
Holesky,
Hoodi,
Dev,
}
Description
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes