feat(polygon-zkevm): Support for custom estimation methods #461
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds Polygon zkEVM tagging/detection, a zkEVM price handler and fetcher, refactors EVM price pipeline to use l1_fee (replacing extra_fee) with cap/finalize steps, expands ProviderError for -32601, removes the L2 fee module, and updates polygon network config and related tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant PriceCalc as PriceCalculator
participant Selector as PriceParamsHandler
participant OptimismH as OptimismHandler
participant ZkEvmH as PolygonZkEVMHandler
Client->>PriceCalc: get_transaction_price_params(tx)
PriceCalc->>PriceCalc: fetch_price_params_based_on_tx_type
PriceCalc->>PriceCalc: apply_gas_price_cap_and_constraints
PriceCalc->>Selector: handle_price_params(tx, params)
alt network.is_polygon_zkevm()
Selector->>ZkEvmH: handle_price_params(tx, params)
ZkEvmH-->>Selector: params (total_cost updated via zkevm_estimateFee)
else network.is_optimism()
Selector->>OptimismH: handle_price_params(tx, params)
OptimismH-->>Selector: params'
else
Selector-->>PriceCalc: params
end
PriceCalc->>PriceCalc: finalize_price_params
PriceCalc-->>Client: final PriceParams (includes l1_fee / total_cost)
sequenceDiagram
autonumber
participant FetchFactory as GasPriceFetcherFactory
participant DefaultF as DefaultGasPriceFetcher
participant ZkFetch as PolygonZkEvmGasPriceFetcher
participant Provider
FetchFactory->>FetchFactory: create_for_network(network)
alt network.is_polygon_zkevm()
FetchFactory->>ZkFetch: fetch_gas_price(provider, network)
ZkFetch->>Provider: zkevm_estimateGasPrice (probe)
alt returns price
ZkFetch-->>FetchFactory: price
else method not available / invalid
ZkFetch->>Provider: get_gas_price() (fallback)
ZkFetch-->>FetchFactory: price or error
end
else
FetchFactory->>DefaultF: fetch_gas_price(provider, network)
DefaultF->>Provider: get_gas_price()
DefaultF-->>FetchFactory: price or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/services/gas/handlers/optimism.rs (3)
72-83: Ceil the compressed size to avoid undercharging L1 data gas.Integer division floors; rounding down underestimates cost. Use ceil-div by 16.
- ((zero_bytes * U256::from(4)) + (non_zero_bytes * U256::from(16))) / U256::from(16) + let calldata_gas = (zero_bytes * U256::from(4)) + (non_zero_bytes * U256::from(16)); + // ceil((zero*4 + non_zero*16)/16) + (calldata_gas + U256::from(15)) / U256::from(16)
107-121: Use GasPriceOracle decimals in the L1/blob weighting; current code likely overestimates.
decimalsis fetched but never applied. Per OP/Bedrock oracles, scalars are fixed-point and must be divided bydecimals. Add safe division for both components.- let weighted_gas_price = U256::from(16) - .saturating_mul(U256::from(fee_data.base_fee_scalar)) - .saturating_mul(U256::from(fee_data.l1_base_fee)) - + U256::from(fee_data.blob_base_fee_scalar) - .saturating_mul(U256::from(fee_data.blob_base_fee)); + let denom = std::cmp::max(fee_data.decimals, U256::from(1)); + let weighted_l1 = U256::from(16) + .saturating_mul(fee_data.base_fee_scalar) + .saturating_mul(fee_data.l1_base_fee) + .saturating_div(denom); + let weighted_blob = fee_data + .blob_base_fee_scalar + .saturating_mul(fee_data.blob_base_fee) + .saturating_div(denom); + let weighted_gas_price = weighted_l1.saturating_add(weighted_blob);
123-144: Let the calculator finalize total_cost after caps; don’t set it here.The price calculator re-applies caps after the handler. Precomputing
total_costhere can make it stale vs. capped values.- // Recalculate total cost with the extra fee - let gas_limit = tx.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT); - let value = tx.value; - let is_eip1559 = original_params.max_fee_per_gas.is_some(); - - original_params.total_cost = - original_params.calculate_total_cost(is_eip1559, gas_limit, value); + // Defer total_cost recomputation to the finalizer (after caps are re-applied)
🧹 Nitpick comments (8)
src/services/provider/mod.rs (1)
40-42: Map JSON-RPC -32601 to a distinct error typeGood addition. Consider adding a focused unit test asserting that -32601 maps to MethodNotAvailable and is not treated as retriable.
Would you like me to add a small test in this module’s test suite to cover this case?
Also applies to: 150-159
src/services/provider/evm/mod.rs (1)
176-188: Optional: avoid mixing legacy and EIP-1559 fieldsWhen maxFeePerGas/maxPriorityFeePerGas are set, omit gasPrice in tx_params to reduce ambiguity.
src/services/gas/handlers/polygon_zkevm.rs (2)
14-19: Docs contradict behavior on fallbacksComment says “no fallbacks needed” but code returns original params when methods are unavailable. Update wording.
Apply this diff:
-/// - Directly leverages the accurate pricing data from the gas price service (no fallbacks needed) +/// - Prefers zkEVM endpoints; gracefully falls back to original params if methods are unavailable
29-59: Optional: parallelize RPC calls to reduce latencyRun zkevm_estimate_gas_price and zkevm_estimate_fee concurrently (try_join) and handle MethodNotAvailable in either result.
src/services/gas/handlers/optimism.rs (1)
164-179: LGTM on EvmTransactionData migration; add a stronger assertion.Since oracle mocks return zeros, assert
extra_fee == Some(0)to harden the test.- // Extra fee should be added - assert!(handled_params.extra_fee.is_some()); + // Extra fee should be present and zero with mocked oracle + assert_eq!(handled_params.extra_fee, Some(U256::ZERO));Also applies to: 208-223
src/domain/transaction/evm/price_calculator.rs (1)
544-587: Be explicit when neither legacy nor EIP-1559 fields are set.Today the else-branch treats “all None” as legacy and caps 0. Consider early-returning an error in that case to avoid silently producing zero pricing.
- } else { + } else if let Some(gp) = price_params.gas_price { // Handle legacy transaction - price_params.gas_price = Some(Self::cap_gas_price( - price_params.gas_price.unwrap_or_default(), + price_params.gas_price = Some(Self::cap_gas_price( + gp, gas_price_cap, )); // For legacy transactions, EIP1559 fields should be None price_params.max_fee_per_gas = None; price_params.max_priority_fee_per_gas = None; + } else { + return Err(TransactionError::InvalidType( + "Missing gas price parameters".to_string(), + )); }src/services/gas/price_params_handler.rs (2)
29-40: Dispatch order/ownership: OK; consider documenting precedence.If a network ever carries multiple tags, document which handler wins (Polygon ZK EVM currently takes precedence).
97-110: Solid unit test for Polygon ZK EVM selection.Optional: also assert handler actually calls into zkEVM estimation (mock provider) in an integration-style test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
config/networks/polygon.json(1 hunks)src/constants/network_tags.rs(1 hunks)src/domain/transaction/evm/price_calculator.rs(3 hunks)src/models/network/evm/network.rs(3 hunks)src/services/gas/handlers/mod.rs(1 hunks)src/services/gas/handlers/optimism.rs(8 hunks)src/services/gas/handlers/polygon_zkevm.rs(1 hunks)src/services/gas/handlers/test_mock.rs(2 hunks)src/services/gas/price_params_handler.rs(7 hunks)src/services/provider/evm/mod.rs(1 hunks)src/services/provider/mod.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-28T11:19:51.397Z
Learnt from: LuisUrrutia
PR: OpenZeppelin/openzeppelin-relayer#427
File: src/constants/network_tags.rs:5-6
Timestamp: 2025-08-28T11:19:51.397Z
Learning: In the OpenZeppelin relayer codebase, when deprecating constants that are still used internally (like OPTIMISM_TAG in LACKS_MEMPOOL_TAGS), use doc comments instead of #[deprecated] attribute to avoid clippy failures during the transition period.
Applied to files:
src/constants/network_tags.rs
📚 Learning: 2025-08-19T14:31:09.686Z
Learnt from: LuisUrrutia
PR: OpenZeppelin/openzeppelin-relayer#427
File: src/models/network/evm/network.rs:1-1
Timestamp: 2025-08-19T14:31:09.686Z
Learning: In the OpenZeppelin relayer codebase, test helper functions may use network names (like "optimism") as string literals for creating mock networks, which are different from network tag constants (like OPTIMISM_BASED_TAG). These should not be flagged as inconsistencies when reviewing tag-related changes.
Applied to files:
src/constants/network_tags.rs
🧬 Code graph analysis (4)
src/services/gas/handlers/polygon_zkevm.rs (3)
src/services/gas/handlers/optimism.rs (1)
handle_price_params(123-144)src/services/gas/price_params_handler.rs (1)
handle_price_params(46-63)src/services/provider/mod.rs (9)
from(47-49)from(53-55)from(59-61)from(102-104)from(108-110)from(114-122)from(127-129)from(137-162)from(167-169)
src/services/provider/evm/mod.rs (1)
src/services/provider/mod.rs (9)
from(47-49)from(53-55)from(59-61)from(102-104)from(108-110)from(114-122)from(127-129)from(137-162)from(167-169)
src/domain/transaction/evm/price_calculator.rs (3)
src/domain/transaction/evm/evm_transaction.rs (1)
relayer(123-125)src/models/transaction/repository.rs (3)
is_eip1559(421-421)is_eip1559(430-432)from(1046-1053)src/models/relayer/repository.rs (2)
from(85-97)from(101-128)
src/services/gas/price_params_handler.rs (4)
src/domain/transaction/evm/price_calculator.rs (1)
new(156-161)src/services/gas/handlers/optimism.rs (1)
new(31-36)src/services/gas/handlers/polygon_zkevm.rs (1)
new(25-27)src/services/provider/evm/mod.rs (3)
new(214-236)new(614-624)setup_test_env(635-638)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: test
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: Analyze (rust)
- GitHub Check: semgrep/ci
🔇 Additional comments (12)
src/services/gas/handlers/mod.rs (1)
4-4: Expose Polygon zkEVM handler — looks goodPublic module and re-export are correctly wired for dispatch.
Also applies to: 9-9
src/constants/network_tags.rs (1)
5-5: Add POLYGON_ZKEVM_TAG — looks goodTag name aligns with config and classification; no change to LACKS_MEMPOOL_TAGS is appropriate.
src/models/network/evm/network.rs (1)
3-5: Polygon zkEVM classification and tests — looks goodImport, is_polygon_zkevm(), and the test validate the tag-driven detection correctly.
Also applies to: 131-134, 257-266
src/services/gas/handlers/test_mock.rs (1)
3-3: Mock updated to EvmTransactionData — looks goodSignature and behavior remain consistent for tests.
Also applies to: 16-16
config/networks/polygon.json (2)
1-26: Note on summary discrepancy (no action required)The first “polygon” mainnet entry doesn’t include "tags": ["polygon-zkevm"] in this file, contrary to the AI summary. Treat the code as source of truth.
61-65: Cardona chain ID and RPC verified — no changes required.
Chain ID 2442 is correct for Polygon zkEVM Cardona (testnet), and https://rpc.cardona.zkevm-rpc.com is the recommended/public testnet RPC endpoint.src/services/provider/evm/mod.rs (1)
158-166: Reject object-parsing refactor — zkevm_estimateGasPrice and zkevm_estimateFee return hex stringsVerified: both RPCs return hex‑encoded wei as plain strings in the JSON‑RPC "result" field; the suggested diff to accept object shapes is unnecessary and incorrect.
Location: src/services/provider/evm/mod.rs (lines 158–166 and 194–201) — keep current string parsing.Likely an incorrect or invalid review comment.
src/domain/transaction/evm/price_calculator.rs (2)
183-198: Pipeline refactor is clean and readable.Good separation (fetch → cap → handler → finalize). Resetting transient fields before finalization is sensible.
268-271: Finalization hook usage looks correct.Delegating to
finalize_price_paramskeeps bump logic focused and consistent.src/services/gas/price_params_handler.rs (3)
18-19: New PolygonZKEvm handler variant looks good.Enum expansion and type wiring are coherent with existing Optimism path.
47-55: Signature switched to EvmTransactionData: LGTM.Async delegation per variant is consistent.
140-155: EvmTransactionData construction in tests is correct.Fields align with the new model.
| /// Applies price params handler and finalizes price parameters. | ||
| async fn finalize_price_params( | ||
| &self, | ||
| relayer: &RelayerRepoModel, | ||
| tx_data: &EvmTransactionData, | ||
| mut price_params: PriceParams, | ||
| ) -> Result<PriceParams, TransactionError> { | ||
| let is_eip1559 = tx_data.is_eip1559(); | ||
|
|
||
| // Apply price params handler if available | ||
| let mut recompute_total_cost = true; | ||
| if let Some(handler) = &self.price_params_handler { | ||
| price_params = handler.handle_price_params(tx_data, price_params).await?; | ||
|
|
||
| // Re-apply cap after handler in case it changed fee fields | ||
| self.apply_gas_price_cap_and_constraints(&mut price_params, relayer)?; | ||
|
|
||
| recompute_total_cost = price_params.total_cost == U256::ZERO; | ||
| } | ||
|
|
||
| // Only recompute total cost if it was not set by the handler | ||
| if recompute_total_cost { | ||
| price_params.total_cost = price_params.calculate_total_cost( | ||
| is_eip1559, | ||
| tx_data.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT), | ||
| U256::from(tx_data.value), | ||
| ); | ||
| } | ||
|
|
||
| Ok(price_params) | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix EIP-1559 detection during finalization; use PriceParams, not tx_data.
Speed-based txs can yield EIP-1559 params even when tx_data.is_eip1559() is false. Using tx_data here can miscompute total_cost.
- let is_eip1559 = tx_data.is_eip1559();
+ let is_eip1559 = price_params.max_fee_per_gas.is_some()
+ && price_params.max_priority_fee_per_gas.is_some();
@@
- price_params.total_cost = price_params.calculate_total_cost(
- is_eip1559,
- tx_data.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT),
- U256::from(tx_data.value),
- );
+ price_params.total_cost = price_params.calculate_total_cost(
+ is_eip1559,
+ tx_data.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT),
+ tx_data.value,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Applies price params handler and finalizes price parameters. | |
| async fn finalize_price_params( | |
| &self, | |
| relayer: &RelayerRepoModel, | |
| tx_data: &EvmTransactionData, | |
| mut price_params: PriceParams, | |
| ) -> Result<PriceParams, TransactionError> { | |
| let is_eip1559 = tx_data.is_eip1559(); | |
| // Apply price params handler if available | |
| let mut recompute_total_cost = true; | |
| if let Some(handler) = &self.price_params_handler { | |
| price_params = handler.handle_price_params(tx_data, price_params).await?; | |
| // Re-apply cap after handler in case it changed fee fields | |
| self.apply_gas_price_cap_and_constraints(&mut price_params, relayer)?; | |
| recompute_total_cost = price_params.total_cost == U256::ZERO; | |
| } | |
| // Only recompute total cost if it was not set by the handler | |
| if recompute_total_cost { | |
| price_params.total_cost = price_params.calculate_total_cost( | |
| is_eip1559, | |
| tx_data.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT), | |
| U256::from(tx_data.value), | |
| ); | |
| } | |
| Ok(price_params) | |
| } | |
| /// Applies price params handler and finalizes price parameters. | |
| async fn finalize_price_params( | |
| &self, | |
| relayer: &RelayerRepoModel, | |
| tx_data: &EvmTransactionData, | |
| mut price_params: PriceParams, | |
| ) -> Result<PriceParams, TransactionError> { | |
| let is_eip1559 = price_params.max_fee_per_gas.is_some() | |
| && price_params.max_priority_fee_per_gas.is_some(); | |
| // Apply price params handler if available | |
| let mut recompute_total_cost = true; | |
| if let Some(handler) = &self.price_params_handler { | |
| price_params = handler.handle_price_params(tx_data, price_params).await?; | |
| // Re-apply cap after handler in case it changed fee fields | |
| self.apply_gas_price_cap_and_constraints(&mut price_params, relayer)?; | |
| recompute_total_cost = price_params.total_cost == U256::ZERO; | |
| } | |
| // Only recompute total cost if it was not set by the handler | |
| if recompute_total_cost { | |
| price_params.total_cost = price_params.calculate_total_cost( | |
| is_eip1559, | |
| tx_data.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT), | |
| tx_data.value, | |
| ); | |
| } | |
| Ok(price_params) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/domain/transaction/evm/price_calculator.rs (1)
588-614: Fix EIP-1559 detection and avoid double-conversion of value when recomputing total_cost.Use the finalized PriceParams to decide EIP‑1559, not tx_data (speed-based or handler overrides can flip the mode). Also use tx_data.value directly (it’s already U256).
Apply:
- let is_eip1559 = tx_data.is_eip1559(); + let is_eip1559 = price_params.max_fee_per_gas.is_some() + && price_params.max_priority_fee_per_gas.is_some(); @@ - price_params.total_cost = price_params.calculate_total_cost( - is_eip1559, - tx_data.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT), - U256::from(tx_data.value), - ); + price_params.total_cost = price_params.calculate_total_cost( + is_eip1559, + tx_data.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT), + tx_data.value, + );Please add a unit test where tx_data is speed-based (no 1559 fields) but handler returns 1559 params; assert total_cost uses 1559 path.
config/networks/polygon.json (1)
67-82: Rename polygon-zkevm-testnet → polygon-zkevm-cardona and fix explorer/RPC (chain_id 2442).config/networks/polygon.json (line 69) currently sets "network": "polygon-zkevm-testnet" but "chain_id" is 2442 (Cardona). Rename the entry to "polygon-zkevm-cardona", update explorer_urls to Cardona endpoints, add a couple public RPC fallbacks for resiliency, or mark the old name as deprecated if you must keep it temporarily.
"from": "polygon-zkevm", "type": "evm", - "network": "polygon-zkevm-testnet", + "network": "polygon-zkevm-cardona", "chain_id": 2442, "explorer_urls": [ - "https://api-testnet-zkevm.polygonscan.com/api", - "https://testnet-zkevm.polygonscan.com" + "https://api-cardona-zkevm.polygonscan.com/api", + "https://cardona-zkevm.polygonscan.com" ], "is_testnet": true, "rpc_urls": [ - "https://rpc.cardona.zkevm-rpc.com" + "https://rpc.cardona.zkevm-rpc.com", + "https://polygon-zkevm-cardona.drpc.org", + "https://polygon-zkevm-cardona.public.blastapi.io" ], "tags": [ - "polygon-zkevm-based" + "polygon-zkevm-based" ]If retaining the old name temporarily, add the deprecated tag:
- "tags": [ - "polygon-zkevm-based" - ] + "tags": [ + "polygon-zkevm-based", + "deprecated" + ]
🧹 Nitpick comments (1)
src/domain/transaction/evm/price_calculator.rs (1)
539-582: Harden constraints: error on empty pricing; cap priority by cap then by maxFee.
- If all fee fields are None, we silently set gas_price to 0. Better to error.
- When EIP‑1559 is provided, first cap priority by gas_price_cap, then ensure it does not exceed capped maxFee.
Proposed changes:
fn apply_gas_price_cap_and_constraints( &self, - price_params: &mut PriceParams, + price_params: &mut PriceParams, relayer: &RelayerRepoModel, ) -> Result<(), TransactionError> { @@ - if let (Some(max_fee), Some(max_priority)) = ( + if let (Some(max_fee), Some(max_priority)) = ( price_params.max_fee_per_gas, price_params.max_priority_fee_per_gas, ) { // Cap the maxFeePerGas let capped_max_fee = Self::cap_gas_price(max_fee, gas_price_cap); price_params.max_fee_per_gas = Some(capped_max_fee); - // Ensure maxPriorityFeePerGas < maxFeePerGas to avoid client errors - price_params.max_priority_fee_per_gas = - Some(Self::cap_gas_price(max_priority, capped_max_fee)); + // Cap priority by cap, then ensure <= maxFee + let capped_priority_by_cap = Self::cap_gas_price(max_priority, gas_price_cap); + price_params.max_priority_fee_per_gas = + Some(std::cmp::min(capped_priority_by_cap, capped_max_fee)); // For EIP1559 transactions, gas_price should be None price_params.gas_price = None; - } else { + } else if let Some(gp) = price_params.gas_price { // Handle legacy transaction - price_params.gas_price = Some(Self::cap_gas_price( - price_params.gas_price.unwrap_or_default(), - gas_price_cap, - )); + price_params.gas_price = Some(Self::cap_gas_price(gp, gas_price_cap)); // For legacy transactions, EIP1559 fields should be None price_params.max_fee_per_gas = None; price_params.max_priority_fee_per_gas = None; + } else { + return Err(TransactionError::NotSupported( + "Missing gas price parameters".to_string(), + )); } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/networks/polygon.json(1 hunks)src/constants/network_tags.rs(1 hunks)src/domain/transaction/evm/price_calculator.rs(3 hunks)src/services/gas/handlers/polygon_zkevm.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/constants/network_tags.rs
- src/services/gas/handlers/polygon_zkevm.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/domain/transaction/evm/price_calculator.rs (2)
src/models/transaction/repository.rs (3)
is_eip1559(421-421)is_eip1559(430-432)from(1046-1053)src/models/relayer/repository.rs (2)
from(85-97)from(101-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: test
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Analyze (rust)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
🔇 Additional comments (3)
config/networks/polygon.json (1)
61-64: Tagging zkEVM mainnet looks good.Adding "polygon-zkevm-based" to the zkEVM mainnet entry is correct and will help route handler logic.
src/domain/transaction/evm/price_calculator.rs (2)
183-193: Pipeline step ordering LGTM.Capping before finalization, then re-applying caps post-handler, is sound for safety.
263-266: Finalize after bump LGTM.Deferring handler application and total-cost finalization until after bumping is correct.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
=======================================
Coverage 92.8% 92.9%
=======================================
Files 233 238 +5
Lines 83197 84503 +1306
=======================================
+ Hits 77233 78512 +1279
- Misses 5964 5991 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/domain/transaction/evm/price_calculator.rs (1)
544-582: Fix EIP-1559 detection during finalization; use PriceParams, not tx_data.Speed-based txs can yield EIP-1559 params even when
tx_data.is_eip1559()is false. Usingtx_datahere can miscomputetotal_cost.
🧹 Nitpick comments (15)
src/domain/transaction/evm/price_calculator.rs (1)
589-614: Consider removing the second cap application after handlerThe gas price caps are already applied in
apply_gas_price_cap_and_constraintsbefore the handler is invoked (line 188). Re-applying them after the handler (line 602) may be redundant unless the handler intentionally exceeds caps. If handlers are expected to respect caps, this second application could be removed for clarity. If handlers may exceed caps for specific networks (e.g., special pricing rules), document this behavior.Consider simplifying to:
async fn finalize_price_params( &self, relayer: &RelayerRepoModel, tx_data: &EvmTransactionData, mut price_params: PriceParams, ) -> Result<PriceParams, TransactionError> { - let is_eip1559 = tx_data.is_eip1559(); + // Determine pricing type from actual params, not tx_data + let is_eip1559 = price_params.max_fee_per_gas.is_some() + && price_params.max_priority_fee_per_gas.is_some(); // Apply price params handler if available if let Some(handler) = &self.price_params_handler { price_params = handler.handle_price_params(tx_data, price_params).await?; - // Re-apply cap after handler in case it changed fee fields - self.apply_gas_price_cap_and_constraints(&mut price_params, relayer)?; + // Note: If handler needs to exceed caps for specific networks, + // document this behavior and keep the re-application } if price_params.total_cost == U256::ZERO { price_params.total_cost = price_params.calculate_total_cost( is_eip1559, tx_data.gas_limit.unwrap_or(DEFAULT_GAS_LIMIT), U256::from(tx_data.value), ); } Ok(price_params) }src/services/gas/evm_gas_price.rs (1)
303-305: Consider deduplicating the gas price fetching logic.The gas price fetching logic is duplicated between the cached and non-cached paths. Consider extracting this into a helper method to reduce code duplication.
+ async fn fetch_gas_price_with_factory(&self) -> Result<u128, TransactionError> { + GasPriceFetcherFactory::fetch_gas_price(&self.provider, &self.network) + .await + .map_err(|e| TransactionError::NetworkConfiguration(e.to_string())) + } async fn get_legacy_prices_from_json_rpc(&self) -> Result<SpeedPrices, TransactionError> { let base = if let Some(cache) = &self.cache { // ... cache logic ... - GasPriceFetcherFactory::fetch_gas_price(&self.provider, &self.network) - .await - .map_err(|e| TransactionError::NetworkConfiguration(e.to_string()))? + self.fetch_gas_price_with_factory().await? } } else { - GasPriceFetcherFactory::fetch_gas_price(&self.provider, &self.network) - .await - .map_err(|e| TransactionError::NetworkConfiguration(e.to_string()))? + self.fetch_gas_price_with_factory().await? };src/services/gas/fetchers/default.rs (2)
64-64: Fix test name typo.The test is named
test_default_estimator_failurebut should betest_default_fetcher_failureto maintain consistency with the implementation naming.- async fn test_default_estimator_failure() { + async fn test_default_fetcher_failure() {
92-116: Fix inconsistent test names.Several test methods use "estimator" instead of "fetcher" in their names, which is inconsistent with the implementation.
- async fn test_default_estimator_network_error() { + async fn test_default_fetcher_network_error() { - async fn test_default_estimator_ethereum() { + async fn test_default_fetcher_ethereum() { - async fn test_default_estimator_polygon() { + async fn test_default_fetcher_polygon() {src/services/gas/cache.rs (1)
477-502: Test placement could be improved.The
test_polygon_zkevm_network_detectiontest is validatingEvmNetwork::is_polygon_zkevm()functionality, which belongs to the network model. Consider moving this test to the appropriate network model test file for better organization.This test should be moved to
src/models/network/evm/network.rstest module or a dedicated test file for theEvmNetworkmodel, as it's testing network detection logic rather than cache functionality.src/services/gas/fetchers/polygon_zkevm.rs (5)
41-46: Consider documenting RPC provider requirements.Since not all RPC providers support the
zkevm_estimateGasPricemethod, consider adding documentation (either in code comments or external docs) about which providers are known to support this method and how users can verify support.
76-79: Consider using debug level for successful price fetches.The successful gas price fetch currently logs at
infolevel. Since this happens frequently during normal operation, consider usingdebuglevel to reduce log noise, similar to the factory's logging pattern.- log::info!( + log::debug!( "zkEVM gas price estimated for chain_id {}: {} wei",
101-105: Align fallback logging with zkEVM success logging.For consistency with the previous suggestion, consider using
debuglevel for the fallback success case as well.- log::info!( + log::debug!( "Using standard gas price fallback for zkEVM chain_id {}: {} wei",
164-193: Test naming inconsistency.The test is named
test_zkevm_estimator_method_not_availablebut should betest_zkevm_fetcher_method_not_availableto maintain consistency with the implementation naming.- async fn test_zkevm_estimator_method_not_available() { + async fn test_zkevm_fetcher_method_not_available() {
195-271: Fix inconsistent test names throughout.Multiple test methods use "estimator" instead of "fetcher" in their names.
- async fn test_zkevm_estimator_invalid_response() { + async fn test_zkevm_fetcher_invalid_response() { - async fn test_zkevm_estimator_invalid_hex_response() { + async fn test_zkevm_fetcher_invalid_hex_response() { - async fn test_zkevm_estimator_other_error() { + async fn test_zkevm_fetcher_other_error() { - async fn test_zkevm_estimator_both_methods_fail() { + async fn test_zkevm_fetcher_both_methods_fail() { - async fn test_zkevm_estimator_hex_with_0x_prefix() { + async fn test_zkevm_fetcher_hex_with_0x_prefix() { - async fn test_zkevm_estimator_hex_without_0x_prefix() { + async fn test_zkevm_fetcher_hex_without_0x_prefix() {src/services/gas/fetchers/mod.rs (2)
16-25: Missing documentation for thePolygonZkEvmvariantThe
PolygonZkEvmvariant lacks a documentation comment similar to theDefaultvariant on line 23.pub enum GasPriceFetcher { /// Default EVM gas price fetcher using standard `eth_gasPrice` Default(default::DefaultGasPriceFetcher), + /// Polygon zkEVM gas price fetcher with custom estimation methods PolygonZkEvm(polygon_zkevm::PolygonZkEvmGasPriceFetcher), }
72-80: Consider consolidating error handling patternsThe error handling for
Ok(None)creates a new error with generic text. Consider using a more specific error type or constant to improve maintainability and consistency across the codebase.Ok(None) => { log::warn!( "Fetcher returned None for supported network chain_id {}", network.chain_id ); - Err(ProviderError::Other( - "Fetcher failed to provide gas price for supported network".to_string(), - )) + Err(ProviderError::GasPriceUnavailable(format!( + "No gas price available for chain_id {}", + network.chain_id + ))) }Note: This assumes
ProviderError::GasPriceUnavailableis or can be added to the error enum.src/services/gas/handlers/polygon_zkevm.rs (2)
18-31: Consider validating thetoaddress formatThe function defaults to
"0x"for missingtoaddress on line 21, which represents a contract creation. However, there's no validation that existingtoaddresses are properly formatted hex strings with0xprefix.Consider adding address validation or documenting that callers are responsible for providing valid addresses:
fn build_zkevm_transaction_params(tx: &EvmTransactionData) -> serde_json::Value { + // Note: Assumes `from` and `to` (if present) are valid hex addresses with 0x prefix serde_json::json!({ "from": tx.from, "to": tx.to.as_ref().unwrap_or(&"0x".to_string()),
105-176: Mock tests duplicate production code logicThe
MockZkEvmHandlerimplementation (lines 147-175) duplicates the exact same logic as the productionhandle_price_paramsmethod. This creates maintenance burden and doesn't truly test the production code.Consider using a mock provider instead of reimplementing the handler logic:
- struct MockZkEvmHandler { - fee_estimate_result: MockFeeEstimateResult, - } - - impl MockZkEvmHandler { - // ... duplicate implementation ... - } + use crate::services::provider::evm::MockEvmProviderTrait; + use futures::FutureExt; + + async fn test_with_mock_provider( + fee_response: serde_json::Value, + ) -> Result<PriceParams, TransactionError> { + let mut mock_provider = MockEvmProviderTrait::new(); + mock_provider + .expect_raw_request_dyn() + .returning(move |_, _| { + let response = fee_response.clone(); + async move { Ok(response) }.boxed() + }); + + let handler = PolygonZKEvmPriceHandler::new(mock_provider); + // ... rest of test + }src/services/gas/price_params_handler.rs (1)
91-95: Consider extracting test environment setup to a shared test utilityThe
setup_test_env()function sets environment variables that are used across multiple test files. Consider moving this to a shared test utilities module.This would reduce duplication and ensure consistent test setup across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
config/networks/polygon.json(2 hunks)src/domain/transaction/evm/evm_transaction.rs(5 hunks)src/domain/transaction/evm/price_calculator.rs(19 hunks)src/domain/transaction/evm/replacement.rs(3 hunks)src/models/transaction/repository.rs(1 hunks)src/services/gas/cache.rs(3 hunks)src/services/gas/evm_gas_price.rs(4 hunks)src/services/gas/fetchers/default.rs(1 hunks)src/services/gas/fetchers/mod.rs(1 hunks)src/services/gas/fetchers/polygon_zkevm.rs(1 hunks)src/services/gas/handlers/optimism.rs(9 hunks)src/services/gas/handlers/polygon_zkevm.rs(1 hunks)src/services/gas/handlers/test_mock.rs(2 hunks)src/services/gas/l2_fee.rs(0 hunks)src/services/gas/mod.rs(1 hunks)src/services/gas/price_params_handler.rs(7 hunks)
💤 Files with no reviewable changes (1)
- src/services/gas/l2_fee.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- config/networks/polygon.json
🧰 Additional context used
🧬 Code graph analysis (8)
src/services/gas/fetchers/polygon_zkevm.rs (2)
src/services/gas/fetchers/default.rs (1)
fetch_gas_price(17-26)src/services/gas/fetchers/mod.rs (2)
fetch_gas_price(29-40)fetch_gas_price(57-90)
src/services/gas/fetchers/mod.rs (2)
src/services/gas/fetchers/default.rs (1)
fetch_gas_price(17-26)src/services/gas/fetchers/polygon_zkevm.rs (1)
fetch_gas_price(18-27)
src/domain/transaction/evm/price_calculator.rs (2)
src/models/transaction/repository.rs (3)
from(1046-1053)is_eip1559(421-421)is_eip1559(430-432)src/models/relayer/response.rs (6)
from(49-61)from(152-168)from(172-196)from(446-457)from(461-477)from(481-488)
src/services/gas/fetchers/default.rs (2)
src/services/gas/fetchers/mod.rs (2)
fetch_gas_price(29-40)fetch_gas_price(57-90)src/services/gas/fetchers/polygon_zkevm.rs (1)
fetch_gas_price(18-27)
src/services/gas/evm_gas_price.rs (3)
src/services/gas/fetchers/default.rs (1)
fetch_gas_price(17-26)src/services/gas/fetchers/mod.rs (2)
fetch_gas_price(29-40)fetch_gas_price(57-90)src/services/gas/fetchers/polygon_zkevm.rs (1)
fetch_gas_price(18-27)
src/services/gas/cache.rs (4)
src/services/gas/fetchers/default.rs (1)
fetch_gas_price(17-26)src/services/gas/fetchers/mod.rs (2)
fetch_gas_price(29-40)fetch_gas_price(57-90)src/services/gas/fetchers/polygon_zkevm.rs (1)
fetch_gas_price(18-27)src/models/network/evm/network.rs (1)
is_polygon_zkevm(131-133)
src/services/gas/handlers/polygon_zkevm.rs (3)
src/services/provider/mod.rs (9)
from(47-49)from(53-55)from(59-61)from(102-104)from(108-110)from(114-122)from(127-129)from(137-162)from(167-169)src/services/gas/handlers/optimism.rs (2)
new(31-36)handle_price_params(123-144)src/services/gas/price_params_handler.rs (1)
handle_price_params(46-63)
src/services/gas/price_params_handler.rs (4)
src/domain/transaction/evm/price_calculator.rs (1)
new(156-161)src/services/gas/handlers/optimism.rs (1)
new(31-36)src/services/gas/handlers/polygon_zkevm.rs (1)
new(45-47)src/services/provider/evm/mod.rs (2)
new(159-181)new(559-569)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: Analyze (rust)
🔇 Additional comments (29)
src/domain/transaction/evm/price_calculator.rs (1)
79-79: Clear semantic improvement: l1_fee field name is more descriptiveThe rename from
extra_feetol1_feebetter communicates the purpose of this field as Layer 1 fees for L2 networks like Optimism and Polygon zkEVM.src/models/transaction/repository.rs (1)
1229-1229: LGTM!Test correctly updated to use the renamed
l1_feefield.src/services/gas/mod.rs (1)
1-4: Documentation update accurately reflects module's purposeGood update to the module documentation - "fetching and calculation" is more accurate than "estimation and calculation" since the module retrieves existing gas prices from the network rather than estimating future prices.
src/domain/transaction/evm/evm_transaction.rs (5)
916-917: Test correctly uses renamed l1_fee fieldThe test has been properly updated to use
l1_feeinstead ofextra_feein the PriceParams struct.
1016-1017: Test correctly uses renamed l1_fee fieldThe test has been properly updated to use
l1_feeinstead ofextra_feein the PriceParams struct.
1173-1173: Test correctly uses renamed l1_fee field with L1 fee valueThe test has been properly updated to use
l1_feewith a non-None value (U256::ZERO), correctly reflecting L1 fee scenarios.
1353-1353: Test correctly uses renamed l1_fee field with L1 fee valueThe test has been properly updated to use
l1_feewith a non-None value (U256::ZERO) for replacement transaction pricing.
1838-1838: Test correctly uses renamed l1_fee fieldThe test has been properly updated to use
l1_feeinstead ofextra_feein the PriceParams struct for gas estimation tests.src/domain/transaction/evm/replacement.rs (3)
132-132: Test correctly uses renamed l1_fee fieldThe field rename from
extra_feetol1_feehas been properly applied in the validate_explicit_price_bump function.
401-401: Mock correctly uses renamed l1_fee fieldThe MockPriceCalculator test implementation has been properly updated to use
l1_feeinstead ofextra_fee.
420-420: Mock correctly uses renamed l1_fee fieldThe MockPriceCalculator's calculate_bumped_gas_price method has been properly updated to use
l1_feeinstead ofextra_fee.src/services/gas/evm_gas_price.rs (2)
298-300: LGTM! Clean integration of the gas price fetcher factory.The transition from direct
provider.get_gas_price()calls to usingGasPriceFetcherFactory::fetch_gas_price()is well-implemented. This abstraction enables network-specific gas pricing strategies (e.g., zkEVM's custom methods) while maintaining backward compatibility through the default fetcher.
488-492: Test comment update is accurate.The test comment correctly reflects that the default fetcher is now being used under the hood, maintaining the same expected behavior.
src/services/gas/fetchers/default.rs (1)
16-26: LGTM! Simple and effective default implementation.The
DefaultGasPriceFetchercorrectly delegates to the provider's standardget_gas_price()method, providing a clean fallback for non-zkEVM networks.src/services/gas/cache.rs (1)
263-266: LGTM! Good integration with the fetcher factory in cache refresh.The cache refresh logic correctly uses
GasPriceFetcherFactoryfor fetching gas prices, ensuring consistent behavior across the codebase.src/services/gas/handlers/test_mock.rs (1)
14-22: LGTM! Consistent with the broader refactoring.The changes correctly update the mock handler to use
EvmTransactionDataand the newl1_feefield, aligning with the PR's overall refactoring fromextra_feetol1_fee.src/services/gas/fetchers/polygon_zkevm.rs (1)
17-27: LGTM! Well-structured fetcher with proper fallback logic.The implementation follows a clear strategy: attempt zkEVM-specific method first, then fallback to standard gas price fetching. This ensures compatibility across different RPC providers.
src/services/gas/fetchers/mod.rs (2)
145-161: LGTM! Well-structured test for default fetcherThe test correctly validates the default gas price fetcher behavior and uses appropriate mocking.
163-185: LGTM! Comprehensive zkEVM fetcher testThe test properly validates the zkEVM-specific RPC method call and response parsing.
src/services/gas/handlers/optimism.rs (4)
1-11: LGTM! Import changes align with data model refactoringThe transition from
EvmTransactionRequesttoEvmTransactionDatais correctly implemented in the imports.
72-83: LGTM! Function signature update is consistentThe
calculate_compressed_tx_sizefunction correctly accepts the newEvmTransactionDatatype.
123-144: LGTM! Field rename fromextra_feetol1_feeis properly implementedThe handler correctly:
- Updates the
l1_feefield instead ofextra_fee(line 133)- Recalculates total cost with the L1 fee included
- Maintains backward compatibility with the calculation logic
164-179: Tests correctly updated for the new data modelThe test data structure properly includes all new required fields (
nonce,chain_id,hash,signature,raw) forEvmTransactionData.src/services/gas/handlers/polygon_zkevm.rs (2)
74-102: LGTM! Clean error handling for zkEVM methodsThe handler properly:
- Returns original params when methods are unavailable (graceful fallback)
- Maps provider errors to TransactionError
- Sets
total_costdirectly from the zkEVM estimate instead of usingl1_feefieldThis approach correctly handles the zkEVM case where the estimate already includes all costs.
513-600: LGTM! Comprehensive test coverage for transaction parameter buildingThe test thoroughly validates:
- Hex formatting for all numeric fields
- Proper handling of missing optional fields
- Automatic
0xprefix addition for data fieldssrc/services/gas/price_params_handler.rs (4)
17-22: LGTM! Clean enum design for handler variantsThe enum structure properly encapsulates different network-specific handlers with appropriate conditional compilation for the test mock.
28-40: LGTM! Correct network detection and handler instantiationThe factory method properly:
- Checks for Polygon zkEVM first (line 29)
- Falls back to Optimism check (line 33)
- Returns None for unsupported networks
46-63: LGTM! Clean delegation patternThe match arms properly delegate to each handler variant, maintaining a consistent interface.
97-109: LGTM! Test correctly validates Polygon zkEVM handler creationThe test properly verifies that networks tagged with
POLYGON_ZKEVM_TAGget the correct handler variant.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/gas/fetchers/polygon_zkevm.rs (1)
11-11: Fix unresolvedlogcrate (or align with repo’s logging).
GHAS flagged “use of unresolved modulelog.” Ensure the dependency is present or switch totracingif that’s the project standard.Add (if using log):
# Cargo.toml [dependencies] log = "0.4"Or switch imports/usages to
tracingconsistently.Run to verify what the repo uses:
#!/bin/bash rg -nP -C2 '^\s*use\s+(log|tracing)::' && rg -nP -C2 '\b(log|tracing)::(debug|info|warn|error)!\('
🧹 Nitpick comments (6)
src/services/gas/fetchers/polygon_zkevm.rs (6)
14-15: Derive Default for zero-field fetcher.
Minor ergonomics; enables construction via Default.-#[derive(Debug, Clone)] +#[derive(Debug, Clone, Default)] pub struct PolygonZkEvmGasPriceFetcher;
31-38: Avoid string literal drift: extract method name const.
Reduces typos and eases reuse/tests.+const ZKEVM_ESTIMATE_GAS_PRICE: &str = "zkevm_estimateGasPrice"; @@ - .raw_request_dyn("zkevm_estimateGasPrice", serde_json::Value::Array(vec![])) + .raw_request_dyn(ZKEVM_ESTIMATE_GAS_PRICE, serde_json::Value::Array(vec![]))
59-89: Make parsing more permissive; handle whitespace and0Xprefix.
Some RPCs are loose with casing/whitespace.- let Some(gas_price_hex) = response.as_str() else { + let Some(raw) = response.as_str() else { warn!( "Invalid zkEVM gas price response format for chain_id {}", chain_id ); return Ok(None); }; - - match u128::from_str_radix(gas_price_hex.trim_start_matches("0x"), 16) { + let s = raw.trim(); + let s = s.strip_prefix("0x").or_else(|| s.strip_prefix("0X")).unwrap_or(s); + match u128::from_str_radix(s, 16) { Ok(gas_price) => { - info!( + debug!( "zkEVM gas price estimated for chain_id {}: {} wei", chain_id, gas_price ); Ok(Some(gas_price))Also consider making this a free fn (no &self). Optional:
- fn parse_zkevm_response( - &self, + fn parse_zkevm_response(
73-79: Use debug instead of info to reduce log noise.
These are frequent/expected paths.- info!( + debug!( "zkEVM gas price estimated for chain_id {}: {} wei", chain_id, gas_price );- info!( + debug!( "Using standard gas price fallback for zkEVM chain_id {}: {} wei", network.chain_id, standard_price );Also applies to: 97-104
106-111: Include the underlying error in the log message.
Improves triage.- error!( - "Both zkEVM and standard gas price methods failed for chain_id {}", - network.chain_id - ); + error!( + "Both zkEVM and standard gas price methods failed for chain_id {}: {}", + network.chain_id, e + );
339-369: Add a couple more parse edge-case tests.
Uppercase0X, zero value, and surrounding whitespace.#[test] fn test_parse_zkevm_response_uppercase_prefix_and_spaces() { let fetcher = PolygonZkEvmGasPriceFetcher; let response = serde_json::Value::String(" 0X3b9aca00 ".to_string()); let result = fetcher.parse_zkevm_response(response, 1101); assert_eq!(result.unwrap(), Some(1_000_000_000u128)); } #[test] fn test_parse_zkevm_response_zero() { let fetcher = PolygonZkEvmGasPriceFetcher; let response = serde_json::Value::String("0x0".to_string()); let result = fetcher.parse_zkevm_response(response, 1101); assert_eq!(result.unwrap(), Some(0u128)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/gas/fetchers/mod.rs(1 hunks)src/services/gas/fetchers/polygon_zkevm.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/gas/fetchers/mod.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/gas/fetchers/polygon_zkevm.rs (2)
src/services/gas/fetchers/mod.rs (2)
fetch_gas_price(30-41)fetch_gas_price(58-89)src/services/gas/fetchers/default.rs (1)
fetch_gas_price(17-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
src/services/gas/fetchers/polygon_zkevm.rs (4)
1-6: Good module docs and clear intent.
Concise, explains fallback behavior.
19-28: Fetcher orchestration LGTM.
Early-return on zkEVM and clean fallback.
116-137: Test fixture LGTM.
Accurate Polygon zkEVM mainnet params for unit tests.
139-338: Great test coverage of success/fallback/error paths.
Covers method-not-available, invalid formats, and hex variants.
There was a problem hiding this comment.
Thanks for improving implementation.
I see that we now have option to extend gas price logic and logic to extend price calculations.
Recent change removes overhead from previous implementation.
LGTM
Remaining item to discuss is breaking change with field renaming.
@dylankilkenny If you find time it would be great to get second pair of eyes on this PR. Thanks
LuisUrrutia
force-pushed
the
polygon-zkevm-support-2
branch
from
422795f to
e4e9905
Compare
3 participants
Summary
zkevm_estimate*RPC methods. They recomends the usage of those methods for gas and fee estimation, but no all RPC server supports them.Testing Process
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores / Breaking