refactor: clamp legacy gas price and improve nonce management#378
refactor: clamp legacy gas price and improve nonce management#378nahimterrazas wants to merge 2 commits into
Conversation
nahimterrazas
requested review from
pepebndc,
shahnami and
tirumerla
as code owners
|
Warning Review limit reached
More reviews will be available in 55 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR hardens fee safety and nonce cache integrity in the EVM solver-delivery module. It introduces legacy gas price clamping to cap fallback gas prices against configured policy limits, applies this clamping in fee parameter resolution, and strengthens nonce allocation to prevent rollback from reissuing already-handed-out nonces while improving error classification for rollback decisions. ChangesFee and Nonce Safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
crates/solver-delivery/src/implementations/evm/fees.rs (1)
582-596: ⚡ Quick winConsider adding test cases for uncapped scenarios.
The current test validates the capping behavior when
gas_price > cap, but it doesn't cover:
- When
gas_price <= cap(should returngas_priceunchanged)- When
gas_price_capisNone(should returngas_priceunchanged)🧪 Suggested additional test cases
#[test] fn gas_price_cap_passes_through_when_below_cap() { let policy = ChainFeePolicy { speed: FeeSpeed::Fast, min_priority_fee_per_gas: Some(100_000_000), priority_fee_fallback: 100_000_000, quote_cost_strategy: FeeCostStrategy::BufferedEffective125, gas_price_cap: Some(2_500_000_000), }; assert_eq!( super::clamp_legacy_gas_price_to_cap(1_000_000_000, &policy), 1_000_000_000 ); } #[test] fn gas_price_cap_passes_through_when_none() { let policy = ChainFeePolicy { speed: FeeSpeed::Fast, min_priority_fee_per_gas: Some(100_000_000), priority_fee_fallback: 100_000_000, quote_cost_strategy: FeeCostStrategy::BufferedEffective125, gas_price_cap: None, }; assert_eq!( super::clamp_legacy_gas_price_to_cap(10_000_000_000, &policy), 10_000_000_000 ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/solver-delivery/src/implementations/evm/fees.rs` around lines 582 - 596, Add two unit tests to cover uncapped scenarios for clamp_legacy_gas_price_to_cap: (1) a test (e.g., gas_price_cap_passes_through_when_below_cap) that constructs a ChainFeePolicy with gas_price_cap = Some(2_500_000_000) and asserts clamp_legacy_gas_price_to_cap(1_000_000_000, &policy) returns 1_000_000_000, and (2) a test (e.g., gas_price_cap_passes_through_when_none) that sets gas_price_cap = None and asserts clamp_legacy_gas_price_to_cap(10_000_000_000, &policy) returns 10_000_000_000; keep other policy fields identical to existing tests so you exercise the pass-through behavior when below the cap and when no cap is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/solver-delivery/src/implementations/evm/fees.rs`:
- Around line 582-596: Add two unit tests to cover uncapped scenarios for
clamp_legacy_gas_price_to_cap: (1) a test (e.g.,
gas_price_cap_passes_through_when_below_cap) that constructs a ChainFeePolicy
with gas_price_cap = Some(2_500_000_000) and asserts
clamp_legacy_gas_price_to_cap(1_000_000_000, &policy) returns 1_000_000_000, and
(2) a test (e.g., gas_price_cap_passes_through_when_none) that sets
gas_price_cap = None and asserts clamp_legacy_gas_price_to_cap(10_000_000_000,
&policy) returns 10_000_000_000; keep other policy fields identical to existing
tests so you exercise the pass-through behavior when below the cap and when no
cap is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0e4ad63-1ae7-4079-bf08-51343e40f9d7
📒 Files selected for processing (3)
crates/solver-delivery/src/implementations/evm/alloy.rscrates/solver-delivery/src/implementations/evm/fees.rscrates/solver-delivery/src/implementations/evm/nonce.rs
pepebndc
left a comment
pepebndc
left a comment
There was a problem hiding this comment.
Validated against scan findings H-18, H-27, H-30. H-30 and H-18 are correctly fixed, but H-27 is only partially resolved — requesting changes.
H-30 (legacy gas cap) — ✅ fixed. clamp_legacy_gas_price_to_cap is wired into both legacy fallback branches in get_fee_params (alloy.rs ~2721 and ~2753).
H-18 (nonce-reuse safety) — ✅ fixed. reset_next_nonce now returns local.max(next), so a stale-pending rollback can't move the cache below the local high-water mark.
H-27 (fee-cap/base-fee misclassification → nonce wedge) — DefinitelyRejected is correct, but the rollback it now triggers is neutralized by this same PR's H-18 clamp. For the common single-in-flight-tx case (cache at N+1, chain pending=N because the tx never entered the pool), reset_next_nonce returns max(N+1, N) = N+1, so nonce N is never reclaimed. next_nonce_for uses the forward-only set_next_nonce and the drift monitor only logs — nothing frees the leaked nonce, so the permanent nonce gap / liveness wedge the finding describes still occurs. The PR's own renamed test apply_nonce_cache_action_rollback_with_pending_preserves_high_water_cache demonstrates the rollback is now a no-op (cache stays 101 when pending=100). The added H-27 tests only assert classification strings, not end-to-end nonce reclamation.
Asks:
- Reconcile H-18 vs H-27: after a
DefinitelyRejected(never-broadcast) outcome, the leaked nonce must actually be reclaimed for the lone-in-flight case. Consider scoping the rollback to the rejected attempt's nonce rather than relying on the high-water clamp, and add a test asserting the nextnext_nonce_forreturns N (not N+1).
Minor (non-blocking):
- H-18's clamp removes the backward-reset that the nonce-too-low resync path still depends on; the comments at
alloy.rs:1851-1854/723and the resync trace log now contradict actual behavior — please update or rescope. - H-27 matches 3 substring phrasings rather than structured RPC error codes; other providers' fee-cap strings fall back to
Ambiguous.
2 participants
Summary
Testing Process
Checklist
Summary by CodeRabbit