fix: Drop transaction if we cannot resubmit it#7240
Conversation
|
📝 WalkthroughWalkthroughResubmission handling for Ethereum transactions changed: when the new gas price equals the existing one, the adapter now returns Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as SubmitAttempt
participant Adapter as EthereumAdapter
participant Store as TxStore
Caller->>Adapter: submit(tx, new_gas)
Adapter->>Store: fetch_existing_tx(tx.hash)
Store-->>Adapter: existing_tx(status, old_gas)
alt new_gas == old_gas
note right of Adapter #f9f0c1: decision depends on status
Adapter->>Adapter: match status
alt PendingInclusion or Dropped(_)
Adapter-->>Caller: Err(TxWontBeResubmitted)
else Mempool or Included or Finalized
Adapter-->>Caller: Err(TxAlreadyExists)
end
else new_gas != old_gas
Adapter-->>Caller: proceed with resubmission
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)rust/main/**/src/**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)rust/main/lander/src/dispatcher/db/transaction/loader.rs (2)
⏰ 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). (13)
🔇 Additional comments (5)
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)
rust/main/lander/src/adapter/chains/ethereum/adapter/tests.rs (2)
381-410: Legacy parity: add mempool/finalized casesYou covered Included and PendingInclusion for legacy. For symmetry with EIP‑1559, add:
- Legacy + Mempool → expect TxAlreadyExists
- Legacy + Finalized → expect TxAlreadyExists
This keeps the garden evenly trimmed across tx types.
Also applies to: 412-442
176-208: Reduce setup duplication with a small builder helperA tiny test helper will de‑mud the repeated “set existing gas price” blocks.
Here’s a sketch you can drop into the tests module:
+fn with_eip1559_price( + mut tx: crate::Transaction, + max_fee: u64, + max_prio: u64, +) -> crate::Transaction { + if let VmSpecificTxData::Evm(p) = &mut tx.vm_specific_data { + p.tx = TypedTransaction::Eip1559(Eip1559TransactionRequest { + from: Some(H160::random()), + to: Some(H160::random().into()), + nonce: Some(0u64.into()), + gas: Some(21_000u64.into()), + max_fee_per_gas: Some(max_fee.into()), + max_priority_fee_per_gas: Some(max_prio.into()), + value: Some(1u64.into()), + ..Default::default() + }); + } + tx +} + +fn with_legacy_price(mut tx: crate::Transaction, gas_price: u64) -> crate::Transaction { + if let VmSpecificTxData::Evm(p) = &mut tx.vm_specific_data { + p.tx = TypedTransaction::Legacy(TransactionRequest { + from: Some(H160::random()), + to: Some(H160::random().into()), + nonce: Some(0u64.into()), + gas: Some(21_000u64.into()), + gas_price: Some(gas_price.into()), + value: Some(1u64.into()), + ..Default::default() + }); + } + tx +}Then tests become one‑liners for setup, keeping the path clear.
Also applies to: 210-242, 244-276, 278-310, 312-346, 381-410, 412-442
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs(2 hunks)rust/main/lander/src/adapter/chains/ethereum/adapter/tests.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/lander/src/adapter/chains/ethereum/adapter/tests.rsrust/main/lander/src/adapter/chains/ethereum/adapter.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tests.rs (2)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (3)
tx(264-273)tx(651-657)check_if_resubmission_makes_sense(163-194)rust/main/lander/src/adapter/chains/ethereum/tests.rs (1)
dummy_evm_tx(116-138)
⏰ 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). (11)
- GitHub Check: infra-test
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: lander-coverage
- GitHub Check: lint-rs
- GitHub Check: test-rs
🔇 Additional comments (3)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (2)
167-168: Scoped import keeps things tidyPulling TransactionStatus variants into local scope is clean and keeps the outer namespace uncluttered. Nice and lean, just how I like my trails.
187-190: Status-based split is correct; optional logging enhancement suggestedThe enum mapping is exhaustive and sound:
- PendingInclusion | Dropped(_) → TxWontBeResubmitted
- Mempool | Included | Finalized → TxAlreadyExists
All five TransactionStatus variants are covered. Context logging already exists at line 182 with the transaction object. The suggested patch to log the specific error variant chosen is a reasonable optional refinement, particularly useful for metrics/triage if you're tracking error types downstream.
Since the sandbox environment prevents running
cargo clippyandcargo test, you'll want to execute these linting commands locally to catch any missed enum uses or other Rust idioms across the crate.rust/main/lander/src/adapter/chains/ethereum/adapter/tests.rs (1)
177-208: Good coverage for EIP‑1559 same‑price outcomes across statusesAssertions line up with the new split (TxAlreadyExists vs TxWontBeResubmitted). Solid guardrails, no fuss.
Consider adding one negative case where the estimator yields a lower gas price to ensure the submission path never decreases price (if that’s an invariant). If the escalate logic already clamps, a unit test here will keep it honest.
Also applies to: 210-242, 244-276, 278-310, 312-346
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7240 +/- ##
=====================================
Coverage 0.00% 0
=====================================
Files 1 0 -1
Lines 14 0 -14
=====================================
+ Misses 14 0 -14
🚀 New features to boost your workflow:
|
2 participants
Description
Drop transaction if we cannot resubmit it when we reached our limit in gas fee escalation.
Backward compatibility
Yes
Testing
Manual
Summary by CodeRabbit