Use a fixed priority_fee for gas-oracle tx #824
Conversation
Walkthrougheip1559 estimator now ignores on-chain rewards and uses a fixed priority fee; blob base fee is fetched from L1 via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
gas-oracle/app/src/signer.rs (2)
93-101: Fixed-fee estimator matches intent; consider extracting a named constant and cleaning unused staticsThe fixed
max_priority_fee_per_gas = 10^6(0.001 Gwei) and ignoring_rewardsalign with the PR objective and the EIP-1559 API shape looks correct.Two small cleanups you might consider:
- Replace
10u32.pow(6)with a named constant (and/or env-configured value) for readability and easier future tuning.- Now that the estimator no longer scales based on rewards,
PRIORITY_FEE_INCREASE_MULTIPLIERandPRIORITY_FEE_INCREASE_DIVISORappear unused and could be dropped to avoid confusion.For example:
-lazy_static::lazy_static! { - static ref PRIORITY_FEE_INCREASE_MULTIPLIER: i32= read_env_var("PRIORITY_FEE_INCREASE_MULTIPLIER", 11); - static ref PRIORITY_FEE_INCREASE_DIVISOR: i32= read_env_var("PRIORITY_FEE_INCREASE_DIVISOR", 10); - static ref EIP1559_FEE_ESTIMATION_MAX_FEE: u64= read_env_var("EIP1559_FEE_ESTIMATION_MAX_FEE", 200_000_000_000); -} - -fn eip1559_estimator(base_fee_per_gas: U256, _rewards: Vec<Vec<U256>>) -> (U256, U256) { - let max_priority_fee_per_gas = U256::from(10u32.pow(6)); //0.001Gwei +lazy_static::lazy_static! { + static ref EIP1559_FEE_ESTIMATION_MAX_FEE: u64 = + read_env_var("EIP1559_FEE_ESTIMATION_MAX_FEE", 200_000_000_000); +} + +const PRIORITY_FEE_PER_GAS_WEI: u64 = 1_000_000; // 0.001 Gwei + +fn eip1559_estimator(base_fee_per_gas: U256, _rewards: Vec<Vec<U256>>) -> (U256, U256) { + let max_priority_fee_per_gas = U256::from(PRIORITY_FEE_PER_GAS_WEI); let max_fee_per_gas = std::cmp::min( U256::from(*EIP1559_FEE_ESTIMATION_MAX_FEE), base_fee_per_gas + max_priority_fee_per_gas, );
105-116: Hard-coded MorphL2 QuickNode RPC in test; consider making it configurableUsing a concrete MorphL2 QuickNode URL in this ignored test is fine for local/manual runs and matches the PR aim of exercising the L2 path. If you expect others (or CI) to run this occasionally, consider reading the RPC URL from an env var with a sensible default so the test isn’t tied to a specific provider endpoint:
- let l2_provider = Provider::<Http>::try_from("https://rpc-quicknode.morphl2.io").unwrap(); + let rpc_url = + std::env::var("MORPH_L2_RPC_URL").unwrap_or_else(|_| "https://rpc-quicknode.morphl2.io".into()); + let l2_provider = Provider::<Http>::try_from(rpc_url).unwrap();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gas-oracle/app/src/signer.rs(2 hunks)
⏰ 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). (3)
- GitHub Check: check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gas-oracle/app/src/signer.rs (1)
104-133: QuickNode RPC in ignored integration test – confirm endpoint & hard-coded credentials are acceptableSwitching the test RPC to
https://rpc-quicknode.morphl2.iomakes this ignored test talk to a real L2 endpoint instead of a placeholder/local node. That’s fine for a manual integration test, but please double‑check:
- That this public URL is stable and appropriate to bake into source (or consider moving it to an env var if you expect it to change).
- That the hard-coded dev private key and from/to addresses are safe to use against this network (e.g., only ever funded with test/dev funds), so running
cargo test -- --ignoredcannot accidentally spend anything valuable.If both are intentional, no code changes are strictly needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gas-oracle/app/src/signer.rs(2 hunks)
⏰ 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). (3)
- GitHub Check: check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
gas-oracle/app/src/signer.rs (2)
91-99: Fixed-priority EIP-1559 estimator looks correct and aligned with PR intent
PRIORITY_FEE_PER_GAS_WEI = 1_000_000matches the 0.001 Gwei comment (1e9 * 0.001 = 1e6), and the estimator computesmax_fee_per_gas = min(EIP1559_FEE_ESTIMATION_MAX_FEE, base_fee + priority_fee)as expected. Renaming the second arg to_rewardsis an idiomatic way to keep the required callback signature while explicitly ignoring it.From a gas-policy perspective, you may just want to validate on staging that a flat 0.001 Gwei tip is sufficient for timely inclusion under your expected L2 conditions, now that you no longer adapt to rewards.
102-102: Whitespace-only changeThis appears to be a formatting/blank-line change with no behavioral impact; nothing to address here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
gas-oracle/app/src/lib.rs (1)
31-42:contract_errorimplementation is fine; consider small style cleanupThe logic for decoding revert data and formatting a clear error string looks solid and type-correct; no functional issues here.
If you want to simplify slightly, you can drop the
error_msgbinding and return the expression directly:-pub fn contract_error<M: Middleware>(e: ContractError<M>) -> String { - let error_msg = if let Some(contract_err) = e.as_revert() { +pub fn contract_error<M: Middleware>(e: ContractError<M>) -> String { + if let Some(contract_err) = e.as_revert() { if let Some(data) = GasPriceOracleErrors::decode_with_selector(contract_err.as_ref()) { format!("exec error: {:#?}", data) } else { format!("unknown contract error: {:#?}", contract_err) } } else { format!("error: {:#?}", e) - }; - error_msg + } }Purely optional; current version is correct and readable.
gas-oracle/app/src/l1_base_fee.rs (1)
234-234: RedundantU256::from()conversion.
latest_blob_feeis already of typeU256from the RPC response, so the wrapping is unnecessary.- Ok((l1_base_fee, U256::from(latest_blob_fee), gas_price)) + Ok((l1_base_fee, latest_blob_fee, gas_price))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gas-oracle/app/src/l1_base_fee.rs(3 hunks)gas-oracle/app/src/lib.rs(1 hunks)
⏰ 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). (3)
- GitHub Check: check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
gas-oracle/app/src/l1_base_fee.rs (1)
214-221: Good refactor to use authoritative L1 data.Fetching
eth_blobBaseFeedirectly from the L1 node is more reliable than local calculation and avoids maintenance burden when L1 configs change.
| #[tokio::test] | ||
| async fn test_eth_blob_base_fee() -> Result<(), OracleError> { | ||
| let l1_provider = Provider::<Http>::try_from("https://ethereum-rpc.publicnode.com").unwrap(); | ||
| let blob_base_fee = l1_provider | ||
| .request::<(), U256>("eth_blobBaseFee", ()) | ||
| .await | ||
| .map_err(|e| OracleError::L1BaseFeeError(anyhow!(format!("eth_blobBaseFee: {:#?}", e))))?; | ||
|
|
||
| println!("eth_blobBaseFee: {:?}", blob_base_fee); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Test relies on external network call without #[ignore].
This test makes a live RPC call to a public endpoint, which can cause CI flakiness due to rate limits, network issues, or endpoint unavailability. Consider:
- Adding
#[ignore]to exclude from regular test runs:
#[tokio::test]
+#[ignore] // Requires network access to external RPC
async fn test_eth_blob_base_fee() -> Result<(), OracleError> {- Adding an assertion to validate the result (e.g.,
assert!(!blob_base_fee.is_zero())).
🤖 Prompt for AI Agents
In gas-oracle/app/src/l1_base_fee.rs around lines 237 to 247, the test performs
a live RPC call which can make CI flaky; mark the test with #[ignore] to exclude
it from regular runs and add a simple assertion to validate the response (for
example assert that blob_base_fee is not zero) so the test actually verifies
behavior when run manually; optionally document in a comment that the test is
intended for manual/local/integration use or replace the live call with a mocked
Provider in unit tests if you want deterministic CI runs.
Use a fixed priority_fee(0.001 Gwei) for gas-price-oracle tx.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.