chore: Bump price for EVM noop transactions #581
Conversation
WalkthroughThis PR introduces noop-aware gas price bumping by adding a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/transaction/evm/price_calculator.rs (1)
55-67:force_bumpcurrently does not bypass the gas price cap due to re‑capping infinalize_price_paramsRight now
calculate_bumped_gas_priceonly usesforce_bumpwhen choosing the cap passed intohandle_eip1559_bump/handle_legacy_bump:
- For
force_bump = false,relayer_gas_price_capis the relayer policy cap.- For
force_bump = true,relayer_gas_price_cap = u128::MAX, so the helpers don’t cap and computeis_min_bumpedagainst uncapped values.However, after those helpers return, you always call:
self.finalize_price_params(relayer, tx_data, bumped_price_params).awaitand
finalize_price_paramsunconditionally callsapply_gas_price_cap_and_constraints, which re‑readsgas_price_capfrom the relayer policy and caps the prices again. As a result:
- Final
PriceParams.gas_price/max_fee_per_gasare still bounded by the relayer’s cap, even whenforce_bump = true.- The new doc comment (“apply caps unless
force_bumpis true”) and the testtest_calculate_bumped_gas_price_force_bump_bypasses_capare not actually satisfied.- NOOP resubmissions invoked with
force_bump = truewill still be limited by the relayer gas price cap, so the PR’s goal (being able to bump NOOPs above the cap to make them replaceable) is not met.I’d recommend threading
force_bumpthrough tofinalize_price_params(and into the internal cap application) so that the cap is truly skipped for forced bumps, while still applied for normal pricing. Conceptually:- pub async fn calculate_bumped_gas_price( + pub async fn calculate_bumped_gas_price( &self, tx_data: &EvmTransactionData, relayer: &RelayerRepoModel, - force_bump: bool, + force_bump: bool, ) -> Result<PriceParams, TransactionError> { // ... existing logic ... - self.finalize_price_params(relayer, tx_data, bumped_price_params) - .await + self.finalize_price_params(relayer, tx_data, bumped_price_params, force_bump) + .await } @@ - async fn finalize_price_params( + async fn finalize_price_params( &self, relayer: &RelayerRepoModel, tx_data: &EvmTransactionData, mut price_params: PriceParams, - ) -> Result<PriceParams, TransactionError> { + force_bump: bool, + ) -> Result<PriceParams, TransactionError> { let is_eip1559 = tx_data.is_eip1559(); @@ - if let Some(handler) = &self.price_params_handler { + 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)?; + // Re-apply cap after handler in case it changed fee fields, + // unless we’re in a force_bump path (e.g., NOOP resubmission). + if !force_bump { + self.apply_gas_price_cap_and_constraints(&mut price_params, relayer)?; + } } @@ - if price_params.total_cost == U256::ZERO { + 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), ); }and adjust the callers:
- self.finalize_price_params(relayer, tx_data, price_final_params) - .await + self.finalize_price_params(relayer, tx_data, price_final_params, false) + .awaitThis way:
- Normal pricing (
get_transaction_price_params) continues to honour the relayer gas price cap.- Bump flows with
force_bump = truecan exceed the cap as intended (including your new test), while still respecting min‑bump logic and any extra validations you might add later.Also consider updating
apply_gas_price_cap_and_constraints’s doc comment (or adding a short note nearcalculate_bumped_gas_price) to clarify that caps are intentionally skipped whenforce_bumpis used.Also applies to: 136-151, 197-245, 559-595
🧹 Nitpick comments (4)
src/domain/transaction/evm/utils.rs (1)
23-27: NOOPspeednormalization looks good; consider confirming overwrite semantics and extending testsSetting
evm_data.speed = Some(DEFAULT_TRANSACTION_SPEED)ensuresis_noopreliably detects NOOPs (since it requiresspeed.is_some()) and gives pricing a deterministic speed. This does overwrite any existing speed (e.g.,Speed::Fast) when converting to a NOOP, which is probably what you want but is worth double‑checking for all networks. The standard‑network test now asserts the default speed; you might optionally add similar assertions in the Arbitrum tests to lock in the behaviour there as well.Also applies to: 58-64, 274-304
src/domain/transaction/evm/replacement.rs (1)
383-410: MockPriceCalculator updated correctly for newforce_bumpparameterThe mock implementation’s signature now matches
PriceCalculatorTrait::calculate_bumped_gas_price, and ignoring_force_bumpis fine for current tests. If you later add tests that depend onforce_bumpsemantics, you can adapt thereturningclosure to assert on the flag.src/domain/transaction/evm/evm_transaction.rs (2)
13-22: NOOP‑awareforce_bumpwiring inresubmit_transactionlooks correct; check backwards compatibility ofis_noopUsing
is_noop(&evm_data)to drive theforce_bumpargument intocalculate_bumped_gas_priceis a good fit with the new pricing API and keeps cap‑bypassing tightly scoped to genuine NOOP transactions. One subtle point:is_noopalso requiresspeed.is_some(), so any pre‑existing NOOP records that were created beforemake_noopstarted settingspeedwill not benefit fromforce_bumpand will still be capped. If those can exist in production, you may want a migration/backfill or a slightly more relaxed NOOP predicate for the resubmit path.Also applies to: 766-786
1113-1130: Mocks updated forforce_bump; consider asserting on the flag in a dedicated resubmit testThe test
MockPriceCalculatornow accepts theforce_bump: boolargument, and existing tests simply ignore it via_in the closure, which keeps them compiling. For extra confidence that NOOP resubmissions actually passforce_bump = true, you could add a focused test where the mockcalculate_bumped_gas_priceasserts on the boolean (e.g., panics ifforce_bumpis false) when resubmitting a known NOOP transaction. This would catch any future regressions in the wiring.Also applies to: 2728-2740
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/domain/transaction/evm/evm_transaction.rs(4 hunks)src/domain/transaction/evm/price_calculator.rs(22 hunks)src/domain/transaction/evm/replacement.rs(1 hunks)src/domain/transaction/evm/utils.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust_standards.mdc)
**/*.rs: Follow official Rust style guidelines using rustfmt (edition = 2021, max_width = 100)
Follow Rust naming conventions: snake_case for functions/variables, PascalCase for types, SCREAMING_SNAKE_CASE for constants and statics
Order imports alphabetically and group by: std, external crates, local crates
Include relevant doc comments (///) on public functions, structs, and modules. Use comments for 'why', not 'what'. Avoid redundant doc comments
Document lifetime parameters when they're not obvious
Avoid unsafe code unless absolutely necessary; justify its use in comments
Write idiomatic Rust: Prefer Result over panic, use ? operator for error propagation
Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations
Prefer header imports over function-level imports of dependencies
Prefer borrowing over cloning when possible
Use &str instead of &String for function parameters
Use &[T] instead of &Vec for function parameters
Avoid unnecessary .clone() calls
Use Vec::with_capacity() when size is known in advance
Use explicit lifetimes only when necessary; infer where possible
Always use Tokio runtime for async code. Await futures eagerly; avoid blocking calls in async contexts
Streams: Use futures::StreamExt for processing streams efficiently
Always use serde for JSON serialization/deserialization with #[derive(Serialize, Deserialize)]
Use type aliases for complex types to improve readability
Implement common traits (Debug, Clone, PartialEq) where appropriate, using derive macros when possible
Implement Display for user-facing types
Prefer defining traits when implementing services to make mocking and testing easier
For tests, prefer existing utils for creating mocks instead of duplicating code
Use assert_eq! and assert! macros appropriately
Keep tests minimal, deterministic, and fast
Use tracing for structured logging, e.g., tracing::info! for request and error logs
When optimizing, prefer clarity first, then performance
M...
Files:
src/domain/transaction/evm/price_calculator.rssrc/domain/transaction/evm/utils.rssrc/domain/transaction/evm/evm_transaction.rssrc/domain/transaction/evm/replacement.rs
🧬 Code graph analysis (2)
src/domain/transaction/evm/price_calculator.rs (4)
src/domain/transaction/evm/replacement.rs (1)
calculate_bumped_gas_price(404-422)src/services/gas/evm_gas_price.rs (3)
new(163-173)default(33-40)default(52-58)src/models/relayer/response.rs (1)
default(97-112)src/utils/mocks.rs (1)
create_mock_relayer(51-73)
src/domain/transaction/evm/evm_transaction.rs (1)
src/domain/transaction/evm/utils.rs (1)
is_noop(59-64)
⏰ 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: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: msrv
- GitHub Check: Analyze (rust)
- GitHub Check: semgrep/ci
🔇 Additional comments (1)
src/domain/transaction/evm/price_calculator.rs (1)
197-216:force_bumpdocumentation and tests align with intended behaviour once cap skipping is fixedThe extended docs for
calculate_bumped_gas_price(steps 4–5 and the newforce_bumpargument description) plustest_calculate_bumped_gas_price_force_bump_bypasses_capclearly describe the desired semantics: forced bumps should ignore the relayer gas price cap while still satisfying min‑bump constraints. Once you adjustfinalize_price_params/ cap application as suggested, this test becomes an excellent regression guard to ensure NOOP resubmissions really can bypass the cap when required.Also applies to: 2161-2249
Summary
This PR will extend logic to allow bumping gas price for NOOP transactions. This is required in order to make sure relayer stays healthy and able to process new txs.
Use case: tx expires but system is not able to replace properly with NOOP and submit due to price was not bumped by 10% due to gas price cap policy.
Testing Process
Checklist
Note
If you are using Relayer in your stack, consider adding your team or organization to our list of Relayer Users in the Wild!
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.