Migrate gas estimation to match alloy's type#4054
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>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.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 and valuable refactoring that migrates gas price handling to use integer arithmetic and native alloy types, moving away from floating-point calculations. This is a great improvement for precision and aligns the codebase better with alloy's conventions. The changes are extensive and well-executed across the board. My review focuses on a few areas to enhance correctness and maintainability: I've identified a potential panic due to an unchecked conversion, a compilation error, a possible regression in an API response, and opportunities for better code reuse and consistency.
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring that migrates gas price handling from floating-point arithmetic to integer-based calculations using alloy's native types. This is a great improvement for precision and consistency throughout the codebase. The changes are extensive and well-implemented.
I've identified one potential panic due to an unchecked type conversion that should be addressed. I've also included a suggestion to improve code organization by moving a new utility trait to a shared location for better reusability.
Overall, this is a solid piece of work that improves the robustness of gas price handling.
Could you explain in the PR description why the base fee is no longer included and what exactly will be affected if the base fee is not provided? |
squadgazzz
left a comment
squadgazzz
left a comment
There was a problem hiding this comment.
I can't really think of any issues. I trust your tests 🙂
jmg-duarte
requested review from
MartinquaXD
and removed request for
MartinquaXD
| let calculated_tip = eth::U256::from(estimate.max_priority_fee_per_gas) | ||
| .saturating_sub(eth::U256::from(tip_percentage_as_bps)) | ||
| / eth::U256::from(10000u128); |
There was a problem hiding this comment.
This math looks wrong to me. This should use a checked_mul instead of saturating_sub(), no?
| /// The current base gas price that will be charged to all accounts on the | ||
| /// next block. | ||
| base: FeePerGas, | ||
| base: Option<u64>, |
There was a problem hiding this comment.
Looks like this can now be non-optional. AFAICS only the conversionimpl From<EffectiveGasPrice> for GasPrice would complain but this does not seem to be used anywhere.
There was a problem hiding this comment.
I removed the conversion too, nice find!
4 participants
Caution
Review with care! The changes are non trivial and there is one breaking change, the gas price returned by the driver no longer includes the base fee!
Description
Refactors gas price handling to use integer arithmetic and alloy's native types instead of floating-point
calculations. This eliminates precision loss in gas price calculations and better aligns with alloy's conventions.
The removal of the base fee is not a true removal, before this change, the base fee was either 0 or the max value available, both leading to inaccurate results. The new code removes the base fee from the type that was being used to describe the estimations (because the base_fee isn't an estimate, it comes in the previous block); but starts querying the chain for the latest block so it's able to get the proper base_fee (if available). The gas estimates themselves should suffer a big change (since the estimate code is the same) but the effective gas price should become more accurate due to the inclusion of the base fee in the calculations.
Changes
How to test
Note
Tested on staging, starting at Mon, 19 Jan 2026 12:10:18 +0000.
Performed a successful trade: https://staging.explorer.cow.fi/lens/orders/0x06677572a2715cc28241a34f5d669247fba167c8d9adc3fcd338e40a3c52ea4109fbad1ea29c36dfe4f8f7baa87c5edf85e0d9f3696e28f5