feat: add eip1559 settings to chainconfig #99
Conversation
|
Claude finished @tac0turtle's task —— View job Code Review: EIP-1559 Chainconfig SettingsI've completed a comprehensive review of this PR that adds EIP-1559 configuration support to chainspec. Here's my analysis: Todo List:
Overall AssessmentThis is a well-implemented feature that adds valuable configurability for EIP-1559 parameters. The code is generally high quality with good validation and tests. ✅ StrengthsCode Quality
Security
Documentation
🐛 Critical Issues1. Solidity Script Bug (High Priority) - The Gemini review correctly identified this issue. The Lines 135-141 and 169-183: // INCORRECT: Creates "0x0x..." in JSON
console.log(' "0x0": "0x%s",', vm.toString(slot0));
// FIX: Remove the extra "0x"
console.log(' "0x0": "%s",', vm.toString(slot0));This creates invalid JSON that cannot be parsed.
|
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.
| console.log(' "0x0": "0x%s",', vm.toString(slot0)); | ||
| console.log(' "0x1": "0x%s",', vm.toString(slot1)); | ||
| console.log(' "0x2": "0x%s",', vm.toString(slot2)); | ||
| console.log(' "0x3": "0x%s",', vm.toString(slot3)); | ||
| console.log(' "0x4": "0x%s",', vm.toString(slot4)); | ||
| console.log(' "0x5": "0x%s",', vm.toString(slot5)); | ||
| console.log(' "0x6": "0x%s"', vm.toString(slot6)); |
There was a problem hiding this comment.
The vm.toString cheatcode for bytes32 values returns a hex string that already includes the 0x prefix. The current console.log format string "0x%s" will result in a double prefix (e.g., "0x0x..."), which creates invalid JSON for the storage values. Please remove the 0x from the format string to ensure correct JSON output.
console.log(' "0x0": "%s",', vm.toString(slot0));
console.log(' "0x1": "%s",', vm.toString(slot1));
console.log(' "0x2": "%s",', vm.toString(slot2));
console.log(' "0x3": "%s",', vm.toString(slot3));
console.log(' "0x4": "%s",', vm.toString(slot4));
console.log(' "0x5": "%s",', vm.toString(slot5));
console.log(' "0x6": "%s"', vm.toString(slot6));
There was a problem hiding this comment.
I think this should probably be verified
| abi.encodePacked( | ||
| '{"', | ||
| addressKey(cfg.feeVaultAddress), | ||
| '":{"balance":"0x0","code":"0x', | ||
| vm.toString(runtimeCode), | ||
| '","storage":{"0x0":"0x', | ||
| vm.toString(slot0), | ||
| '","0x1":"0x', | ||
| vm.toString(slot1), | ||
| '","0x2":"0x', | ||
| vm.toString(slot2), | ||
| '","0x3":"0x', | ||
| vm.toString(slot3), | ||
| '","0x4":"0x', | ||
| vm.toString(slot4), | ||
| '","0x5":"0x', | ||
| vm.toString(slot5), | ||
| '","0x6":"0x', | ||
| vm.toString(slot6), | ||
| '"}}}' | ||
| ) |
There was a problem hiding this comment.
Similar to the console.log calls, vm.toString for bytes32 values includes the 0x prefix. Hardcoding 0x in the abi.encodePacked call for storage values will result in an invalid JSON string with a double prefix (e.g., "0x0x..."). To fix this, please remove the hardcoded 0x prefix for the storage values.
abi.encodePacked(
'{"',
addressKey(cfg.feeVaultAddress),
'"_balance":"0x0","code":"0x',
vm.toString(runtimeCode),
'","storage":{"0x0":"',
vm.toString(slot0),
'","0x1":"',
vm.toString(slot1),
'","0x2":"',
vm.toString(slot2),
'","0x3":"',
vm.toString(slot3),
'","0x4":"',
vm.toString(slot4),
'","0x5":"',
vm.toString(slot5),
'","0x6":"',
vm.toString(slot6),
'"}}}'
)
There was a problem hiding this comment.
also should be verified
| 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)
| console.log(' "0x0": "0x%s",', vm.toString(slot0)); | ||
| console.log(' "0x1": "0x%s",', vm.toString(slot1)); | ||
| console.log(' "0x2": "0x%s",', vm.toString(slot2)); | ||
| console.log(' "0x3": "0x%s",', vm.toString(slot3)); | ||
| console.log(' "0x4": "0x%s",', vm.toString(slot4)); | ||
| console.log(' "0x5": "0x%s",', vm.toString(slot5)); | ||
| console.log(' "0x6": "0x%s"', vm.toString(slot6)); |
There was a problem hiding this comment.
I think this should probably be verified
| abi.encodePacked( | ||
| '{"', | ||
| addressKey(cfg.feeVaultAddress), | ||
| '":{"balance":"0x0","code":"0x', | ||
| vm.toString(runtimeCode), | ||
| '","storage":{"0x0":"0x', | ||
| vm.toString(slot0), | ||
| '","0x1":"0x', | ||
| vm.toString(slot1), | ||
| '","0x2":"0x', | ||
| vm.toString(slot2), | ||
| '","0x3":"0x', | ||
| vm.toString(slot3), | ||
| '","0x4":"0x', | ||
| vm.toString(slot4), | ||
| '","0x5":"0x', | ||
| vm.toString(slot5), | ||
| '","0x6":"0x', | ||
| vm.toString(slot6), | ||
| '"}}}' | ||
| ) |
There was a problem hiding this comment.
also should be verified
| 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