Vendor gas-estimation crate into shared module #4043
Conversation
Removed external gas-estimation dependency by vendoring its code into shared/gas_price_estimation module: - Inlined GasPriceEstimating and PriorityGasPriceEstimating traits - Vendored GasPrice1559 type into price module - Created NodeGasPriceEstimator to replace web3 legacy gas estimation - Removed NativeGasEstimator and Native gas estimator config option - Updated driver and refunder crates to use vendored implementations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
![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 is a significant step in migrating away from ethcontract by vendoring the gas-estimation crate's functionality directly into the shared crate. The changes are well-structured, removing the old dependency and updating all usage sites to the new vendored implementation. The removal of NativeGasEstimator and its replacement with NodeGasPriceEstimator and making Alloy the default is a notable change that simplifies the configuration. Overall, the refactoring is clean and aligns with the goal of moving towards Alloy APIs. I've found two issues that need to be addressed: a critical syntax error that prevents compilation and an outdated error message.
| "alloy" => Ok(GasEstimatorType::Alloy), | ||
| "native" => Ok(GasEstimatorType::Native), | ||
| _ => Url::parse(s).map(GasEstimatorType::Driver).map_err(|e| { | ||
| format!("expected 'web3', 'native', or a valid driver URL; got {s:?}: {e}") |
There was a problem hiding this comment.
The error message for parsing GasEstimatorType is outdated. It incorrectly lists "native" as a valid option while it has been removed, and it omits the valid "alloy" option. This could be confusing for users. The message should be updated to reflect the currently available string-based estimators.
format!("expected 'web3', 'alloy', or a valid driver URL; got {s:?}: {e}")
jmg-duarte
changed the title
Description
Vendors the
gas-estimationcrate code into theshared/gas_price_estimationmodule as part of the migration away from ethcontract/web3 APIs to Alloy. The externalgas-estimationcrate depends on web3, so vendoring its core types and traits allows us to reimplement gas price estimation using Alloy APIs without the web3 dependency.Changes
gas-estimationdependency from workspace Cargo.tomlgas-estimationdependency from driver, refunder, and shared cratesGasPriceEstimatingtrait intoshared/gas_price_estimationPriorityGasPriceEstimatingintoshared/gas_price_estimation/priorityGasPrice1559type toshared/gas_price_estimation/pricemoduleNodeGasPriceEstimatorto replace Web3 legacy gas price estimationNativeGasEstimatorand its configuration options (Native gas estimator type)NodeGasPriceEstimatorinstead ofNativeGasEstimatorNodeGasPriceEstimatorinstead of web3 legacy estimatorHow to test
Existing tests