feat: Lander EVM, periodically update finalized nonce and detect any messages that need reprocessing#7197
feat: Lander EVM, periodically update finalized nonce and detect any messages that need reprocessing#7197
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7197 +/- ##
=====================================
Coverage 0.00% 0
=====================================
Files 1 0 -1
Lines 14 0 -14
=====================================
+ Misses 14 0 -14
🚀 New features to boost your workflow:
|
kamiyaa
force-pushed
the
jeff/nonce-update
branch
from
8730010 to
3cd2438
Compare
📝 WalkthroughWalkthroughThis PR exposes Changes
Sequence Diagram(s)sequenceDiagram
participant InclusionStage as Inclusion Stage
participant Adapter as Ethereum Adapter
participant NonceDB as Nonce State DB
participant Pool as Inclusion Pool
Note over InclusionStage,Adapter: receive_reprocess_txs loop (polling)
loop every poll_rate
InclusionStage->>Adapter: get_reprocess_txs()
Adapter->>NonceDB: get_finalized_nonce() (old)
NonceDB-->>Adapter: old_nonce
Adapter->>Adapter: run NonceUpdater / scan chain
Adapter->>NonceDB: get_finalized_nonce() (new)
NonceDB-->>Adapter: new_nonce
alt new_nonce < old_nonce
Adapter->>NonceDB: get_tracked_tx(nonce) for each nonce in (new+1..old)
NonceDB-->>Adapter: tx / missing
Adapter-->>InclusionStage: Vec<Transaction>
InclusionStage->>Pool: insert txs by UUID
else new_nonce >= old_nonce
Adapter-->>InclusionStage: empty Vec
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 2
🧹 Nitpick comments (3)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (1)
54-62: The background task has no graceful shutdown.The spawned task runs indefinitely with no way to stop it. If the NonceManager is dropped or the application needs to shut down cleanly, this task will keep running until the process terminates.
Consider holding a JoinHandle and implementing Drop or adding a shutdown method that uses a cancellation token.
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (2)
39-44: Consider adding observability for successful updates.Right now there's no way to tell if the periodic updater is working correctly. Adding a debug log or metric on successful updates would help with monitoring and troubleshooting.
Example:
pub async fn run(&self) { loop { match self.update_latest_finalized_nonce().await { Ok(()) => { tracing::debug!( address = ?self.address, "Updated finalized nonce" ); } Err(e) => { tracing::warn!( address = ?self.address, error = ?e, "Failed to update finalized nonce" ); } } tokio::time::sleep(self.poll_rate).await; } }
53-57: The underflow case might be worth logging.When
next_nonceis zero (possible on a fresh chain or after a reset), the update is silently skipped. While this is correct behavior, logging it could help with debugging unexpected nonce states.Example:
let finalized_nonce = next_nonce.checked_sub(U256::one()); if let Some(finalized_nonce) = finalized_nonce { self.state.update_boundary_nonces(&finalized_nonce).await?; +} else { + tracing::debug!( + address = ?self.address, + "Skipped nonce update: next_nonce is zero" + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust/main/lander/src/adapter/chains/ethereum/nonce.rs(1 hunks)rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs(3 hunks)rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs(1 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/nonce/manager.rsrust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rsrust/main/lander/src/adapter/chains/ethereum/nonce.rs
🧬 Code graph analysis (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
new(23-37)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (2)
new(32-71)address(120-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). (11)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: yarn-install
- GitHub Check: build-and-push-to-gcr
- GitHub Check: test-rs
- GitHub Check: lander-coverage
- GitHub Check: lint-rs
🔇 Additional comments (1)
rust/main/lander/src/adapter/chains/ethereum/nonce.rs (1)
6-6: Looks solid.The module declaration follows the existing pattern and integrates cleanly with the other internal modules.
📝 WalkthroughWalkthroughAdds a periodic nonce updater module, wires it into the Ethereum nonce manager with a capped poll interval based on estimated block time (max 5s), and spawns its async loop. The updater repeatedly fetches the latest finalized nonce via the provider and updates shared nonce state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Manager
participant PeriodicNonceUpdater as Periodic Updater
participant Provider as EVM Provider
participant State as NonceManagerState
Manager->>PeriodicNonceUpdater: new(address, reorg_period, poll_rate, provider, state)
Manager->>PeriodicNonceUpdater: spawn run()
rect rgb(240, 248, 255)
loop every poll_rate (≤ 5s)
PeriodicNonceUpdater->>Provider: get next nonce (finalized block)
Provider-->>PeriodicNonceUpdater: next_nonce | error
alt success
PeriodicNonceUpdater->>State: update finalized nonce boundary
State-->>PeriodicNonceUpdater: ack
else error
Note right of PeriodicNonceUpdater: Map to NonceError::ProviderError
end
PeriodicNonceUpdater-->>PeriodicNonceUpdater: sleep(poll_rate)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (3)
13-13: Tidy up the import
{self, PeriodicNonceUpdater}brings the module name into scope but it’s unused. Trim the swampy bits:-use crate::adapter::chains::ethereum::nonce::periodic_updater::{self, PeriodicNonceUpdater}; +use crate::adapter::chains::ethereum::nonce::periodic_updater::PeriodicNonceUpdater;As per coding guidelines, clippy will also nudge you here.
54-57: Comment semantics: it’s a 5s floor, not a capCode uses
.max(5s), which ensures a minimum 5s interval (slower polling on faster chains). The comment says “cap at 5s”, which reads like a maximum. Suggest rewording to “floor at 5s (minimum interval)” to avoid head‑scratching later.
60-62: Consider keeping a JoinHandle or naming the taskRight now the periodic task is fire‑and‑forget. Keeping a handle (or naming the task) helps with shutdown and observability.
Example:
- tokio::spawn(async move { - periodic_updater.run().await; - }); + let _periodic_task = tokio::spawn(async move { + periodic_updater.run().await; + }); + // Optionally store `_periodic_task` on `NonceManager` for controlled shutdown.rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (3)
8-10: Drop unused importsThese aren’t used; let’s keep it clean and odor‑free.
-use tokio::sync::Mutex; -use tokio::time::Instant;Also run clippy; it’ll bark at these too.
39-44: Don’t swallow errors; log them (and consider interval to avoid drift)Right now errors vanish into the bog. At least log them; optionally switch to
tokio::time::intervalto keep a steady beat.- pub async fn run(&self) { - loop { - let _ = self.update_latest_finalized_nonce().await; - tokio::time::sleep(self.poll_rate).await; - } - } + pub async fn run(&self) { + let mut ticker = tokio::time::interval(self.poll_rate); + loop { + ticker.tick().await; + if let Err(e) = self.update_latest_finalized_nonce().await { + tracing::warn!("periodic nonce update failed for {}: {e}", self.address); + } else { + tracing::debug!("periodic nonce update succeeded for {}", self.address); + } + } + }Optional: add a small randomized initial jitter to avoid herd effects when many updaters start together.
46-59: Verify monotonic boundary semantics; guard against regressionsYou compute
finalized = next - 1and update state ifSome. Please confirmstate.update_boundary_noncesnever regresses the highest finalized nonce (e.g., transient provider/reorg quirks). If it doesn’t enforce monotonicity internally, add a guard.Would you like a patch that reads current finalized from
state(if exposed) and only updates when newer?Also, add unit tests covering:
- next=0 (no update)
- next>0 (updates to next-1)
- provider error path (logged, loop continues)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust/main/lander/src/adapter/chains/ethereum/nonce.rs(1 hunks)rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs(3 hunks)rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs(1 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/nonce.rsrust/main/lander/src/adapter/chains/ethereum/nonce/manager.rsrust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs
🧬 Code graph analysis (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
new(23-37)rust/main/lander/src/adapter/chains/ethereum/nonce/updater.rs (1)
new(24-49)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (2)
new(32-71)address(120-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). (11)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: yarn-install
- GitHub Check: build-and-push-to-gcr
- GitHub Check: test-rs
- GitHub Check: lander-coverage
- GitHub Check: lint-rs
🔇 Additional comments (1)
rust/main/lander/src/adapter/chains/ethereum/nonce.rs (1)
6-6: Module wiring looks good
mod periodic_updater;is correctly added to the module tree. No concerns.
…e-monorepo into jeff/nonce-update
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
37-42: Errors are getting lost in the swamp.The
run()loop silently swallows all errors fromupdate_latest_finalized_nonce()with thatlet _ = .... When updates fail—provider hiccups, network issues, whatever—there's no way to know about it. That's a visibility gap that'll make debugging harder down the road.Add some logging so you know when things go sideways:
pub async fn run(&self) { loop { - let _ = self.update_latest_finalized_nonce().await; + if let Err(e) = self.update_latest_finalized_nonce().await { + tracing::warn!( + address = ?self.address, + error = ?e, + "Failed to update finalized nonce" + ); + } tokio::time::sleep(self.poll_rate).await; } }
🧹 Nitpick comments (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (1)
20-20: Remove unusedselfimportAt rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs:20, replace:
- use super::periodic_updater::{self, PeriodicNonceUpdater};
- use super::periodic_updater::PeriodicNonceUpdater;
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
44-57: Consolidate nonce boundary update logic
Bothperiodic_updater.rs:update_latest_finalized_nonceandupdater.rs:update_boundaries_immediatelyshare identical code – extract it into a shared helper to cut duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs(3 hunks)rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs(1 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/nonce/periodic_updater.rsrust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs
🧬 Code graph analysis (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (2)
new(32-71)address(120-128)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (3)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
new(21-35)rust/main/lander/src/adapter/chains/ethereum/adapter.rs (1)
new(74-127)rust/main/lander/src/adapter/chains/ethereum/nonce/updater.rs (1)
new(24-49)
⏰ 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 (cosmwasm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: test-rs
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
🔇 Additional comments (4)
rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (2)
46-52: Provider cloning is correct here.Using
provider.clone()instead of movingproviderdirectly is necessary since the provider is reused for the periodic updater below. The Arc clone is cheap and maintains shared ownership.
54-62: The spawned task will run, no worries.Despite the past comment's concern,
tokio::spawnstarts the task immediately—you don't need to await the handle for it to run. The task will keep going in the background as long as the runtime's alive. Sinceperiodic_updater.run()is an infinite loop, it'll keep ticking until shutdown.That said, you're not capturing any panics or errors from the spawned task since the
JoinHandleis dropped. But looking at therun()implementation, it won't panic (errors are swallowed), so this approach should work fine for a fire-and-forget background updater.rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (2)
1-18: Struct and imports look solid.The field order and types are consistent with the
NonceUpdaterpattern. No unused imports visible in the current version.
21-35: Constructor follows the right pattern.Matches the style of
NonceUpdater::newand properly initializes all fields. Straightforward and correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
38-49: Errors are silently swallowed in the update loop.The
run()loop ignores all errors fromupdate_state_boundaries_immediately()withlet _ = .... When nonce updates fail (provider errors, network issues, etc.), there's no visibility into these failures.Add error logging:
pub async fn run(&self) { loop { - let _ = NonceUpdater::update_state_boundaries_immediately( + if let Err(e) = NonceUpdater::update_state_boundaries_immediately( &self.provider, &self.state, &self.address, &self.reorg_period, ) - .await; + .await + { + tracing::warn!( + address = ?self.address, + error = ?e, + "Failed to update finalized nonce" + ); + } tokio::time::sleep(self.poll_rate).await; } }
🧹 Nitpick comments (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (1)
22-36: Match the field order in struct initialization.The constructor initializes fields in a different order than they're declared in the struct. Lines 33-34 have
statebeforeprovider, but the struct definition hasproviderbeforestate(lines 17-18).Apply this diff to match the struct order:
Self { address, reorg_period, poll_rate, - state, provider, + state, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust/main/lander/src/adapter/chains/ethereum/nonce.rs(1 hunks)rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs(1 hunks)rust/main/lander/src/adapter/chains/ethereum/nonce/updater.rs(1 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/nonce/updater.rsrust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rsrust/main/lander/src/adapter/chains/ethereum/nonce.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/updater.rs (2)
new(24-49)update_state_boundaries_immediately(71-89)rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (2)
new(32-71)address(120-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). (11)
- GitHub Check: infra-test
- GitHub Check: test-rs
- GitHub Check: lander-coverage
- GitHub Check: lint-rs
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: build-and-push-to-gcr
🔇 Additional comments (6)
rust/main/lander/src/adapter/chains/ethereum/nonce.rs (2)
2-2: LGTM! Making NonceUpdater crate-visible.The change properly exposes
NonceUpdaterat crate level, which is needed by the new periodic updater. Clean and straightforward.
7-7: LGTM! Adding periodic updater module.The new module fits well into the existing nonce subsystem structure.
rust/main/lander/src/adapter/chains/ethereum/nonce/periodic_updater.rs (2)
1-11: LGTM! Import declarations are clean.All imports are used and necessary for the implementation.
13-19: LGTM! Struct definition is straightforward.Fields are well-chosen for the periodic update task.
rust/main/lander/src/adapter/chains/ethereum/nonce/updater.rs (2)
62-69: LGTM! Clean refactoring.Extracting the boundary update logic into a static method makes it reusable by the periodic updater while keeping the existing behavior intact.
71-89: LGTM! Extracted method is well-structured.The new static method properly encapsulates the boundary update logic. Taking references to
Arcparameters is fine here since callers already haveArcs. The error handling withmap_erris appropriate, and the conditional update whenfinalized_nonceis present is correct.
kamiyaa
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rust/main/lander/src/dispatcher/stages/inclusion_stage.rs (1)
210-213: Silent error handling could hide problems.The wildcard pattern here swallows all errors from
get_reprocess_txs()without any logging. When things go sideways, you'll be scratching your head wondering why reprocessing isn't happening.Add some logging so you can actually see what's going wrong:
let txs = match state.adapter.get_reprocess_txs().await { Ok(s) => s, - _ => continue, + Err(err) => { + tracing::warn!(?err, domain, "Failed to get reprocess txs, will retry"); + continue; + } };
🧹 Nitpick comments (2)
rust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rs (1)
562-625: Consider verifying mock invocations to strengthen test reliability.The test checks that transactions end up in the pool, but doesn't verify that
get_reprocess_txswas actually called. Since the mock uses anOptionwrapper that gets consumed, the test could theoretically pass even if the reprocess path never executed.You could add an expectation on the mock to verify it's called at least once:
+ use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc as StdArc; + + let call_count = StdArc::new(AtomicUsize::new(0)); + let call_count_clone = call_count.clone(); + let mut txs_created_option = Some(txs_created.clone()); mock_adapter.expect_get_reprocess_txs().returning(move || { + call_count_clone.fetch_add(1, Ordering::SeqCst); if let Some(txs) = txs_created_option.take() { Ok(txs) } else { Ok(Vec::new()) } });Then assert the call count after the test:
assert!(are_all_txs_in_pool(txs_created.clone(), &pool).await); + assert!(call_count.load(Ordering::SeqCst) > 0, + "get_reprocess_txs should be called at least once");rust/main/lander/src/dispatcher/stages/inclusion_stage.rs (1)
187-224: Function signature implies it can return an error, but it never does.This function returns
Result<(), LanderError>but the only actual return is the earlyOk(())on line 196. After that, it loops forever. The signature suggests error handling that doesn't really exist in practice.Consider changing the signature to better reflect the behavior:
-pub async fn receive_reprocess_txs( +pub async fn receive_reprocess_txs( domain: String, pool: InclusionStagePool, state: DispatcherState, -) -> Result<(), LanderError> { +) { let poll_rate = match state.adapter.reprocess_txs_poll_rate() { Some(s) => s, - None => return Ok(()), + None => return, };This makes it clear that the function either returns early if there's no polling configured, or runs indefinitely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs(1 hunks)rust/main/lander/src/adapter/core.rs(1 hunks)rust/main/lander/src/dispatcher/stages/inclusion_stage.rs(2 hunks)rust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rs(1 hunks)rust/main/lander/src/tests/test_utils.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/main/lander/src/adapter/core.rs
- rust/main/lander/src/adapter/chains/ethereum/adapter.rs
🧰 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/dispatcher/stages/inclusion_stage.rsrust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rsrust/main/lander/src/tests/test_utils.rs
🧬 Code graph analysis (3)
rust/main/lander/src/dispatcher/stages/inclusion_stage.rs (1)
rust/main/lander/src/dispatcher/core.rs (3)
tokio(77-77)tokio(79-79)spawn(73-184)
rust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rs (4)
rust/main/lander/src/tests/test_utils.rs (3)
tmp_dbs(43-53)create_random_txs_and_store_them(73-89)are_all_txs_in_pool(95-101)rust/main/lander/src/dispatcher/stages/inclusion_stage.rs (1)
new(43-56)rust/main/lander/src/dispatcher/stages/state.rs (1)
new(40-54)rust/main/lander/src/dispatcher/metrics.rs (2)
new(60-199)dummy_instance(313-317)
rust/main/lander/src/tests/test_utils.rs (2)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (2)
reprocess_txs_poll_rate(641-645)get_reprocess_txs(647-693)rust/main/lander/src/adapter/core.rs (2)
reprocess_txs_poll_rate(122-124)get_reprocess_txs(133-135)
⏰ 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: test-rs
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmwasm)
🔇 Additional comments (2)
rust/main/lander/src/tests/test_utils.rs (1)
38-39: Mock extensions look good.These additions properly extend the
MockAdapterto support the newAdaptsChaintrait methods for reprocess functionality.rust/main/lander/src/dispatcher/stages/inclusion_stage.rs (1)
67-70: Task spawning follows established patterns.The new
receive_reprocess_txstask is properly spawned alongside the existing tasks, using the same instrumentation and joining approach.
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)
rust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rs (1)
49-70: Add missing mock expectation in two tests.The
InclusionStageimplementation callsstate.adapter.reprocess_txs_poll_rate()duringrun(). Two tests are missing this mock expectation:
test_unincluded_txs_reach_mempool(lines 49-87)test_transaction_not_ready_for_resubmission(lines 235-259)Both need to add:
mock_adapter .expect_reprocess_txs_poll_rate() .returning(|_| None);Without this, the tests will panic with "No matching expectation found" when the stage runs.
♻️ Duplicate comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (1)
695-716: Critical: Infinite loop at U256::MAX boundary.When
old_finalized_nonceequalsU256::MAX, the loop at line 697 never terminates. Here's why:
- Line 697:
while nonce <= old_finalized_nonceallowsnonceto reachU256::MAX- Line 713:
saturating_add(U256::one())returnsU256::MAXwhen already at MAX (saturating arithmetic)- Loop condition remains true forever, hanging the reprocess task
Add an explicit break before the saturating_add to handle the boundary:
} else { debug!( ?nonce, ?tx_uuid, "No transaction found for nonce in reorg range" ); } + if nonce == old_finalized_nonce { + break; + } nonce = nonce.saturating_add(U256::one()); }Consider adding a test case that covers nonce values near
U256::MAXto prevent regressions.
🧹 Nitpick comments (1)
rust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rs (1)
583-646: Test coverage could be more thorough.This test verifies that transactions from
get_reprocess_txs()are inserted into the pool, but doesn't validate:
- Whether the finalized nonce is actually updated
- The nonce comparison logic (new < old scenario)
- Edge cases like empty transaction ranges or boundary conditions
- Whether reprocessed transactions are eventually processed (they stay pending here)
Consider adding assertions or separate tests to cover:
- Verify the mock's
get_reprocess_txsis actually called during the test run- Test the nonce boundary logic more directly (mock different old/new finalized nonces)
- Add a test where reprocessed transactions transition through the inclusion stage to the finality stage
Example enhancement to verify the mock was called:
let call_count = Arc::new(AtomicU32::new(0)); let call_count_clone = call_count.clone(); mock_adapter.expect_get_reprocess_txs().returning(move || { call_count_clone.fetch_add(1, Ordering::SeqCst); // ... existing logic }); // ... after test runs assert!(call_count.load(Ordering::SeqCst) > 0, "get_reprocess_txs should be called");
📜 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(1 hunks)rust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rs(8 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.rsrust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rs
🧬 Code graph analysis (2)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (3)
rust/main/lander/src/adapter/core.rs (3)
reprocess_txs_poll_rate(122-124)get_reprocess_txs(133-135)tx(69-73)rust/main/lander/src/adapter/chains/ethereum/nonce/updater.rs (1)
new(24-49)rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (1)
new(31-60)
rust/main/lander/src/dispatcher/stages/inclusion_stage/tests.rs (4)
rust/main/lander/src/tests/test_utils.rs (3)
tmp_dbs(43-53)create_random_txs_and_store_them(73-89)are_all_txs_in_pool(95-101)rust/main/lander/src/dispatcher/stages/inclusion_stage.rs (1)
new(43-56)rust/main/lander/src/dispatcher/stages/state.rs (1)
new(40-54)rust/main/lander/src/dispatcher/metrics.rs (2)
new(60-199)dummy_instance(313-317)
⏰ 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). (1)
- GitHub Check: build-and-push-to-gcr
🔇 Additional comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (1)
664-668: Solid rate limiting for reprocess polling.The logic caps the poll rate at 5 seconds minimum to avoid hammering the nonce queries on fast chains, while still being responsive enough for reorg detection. Clean implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (1)
674-720: Reprocessing logic looks solid, but there's a tiny edge case lurking.The flow here makes sense—grab the old nonce, update boundaries, check if there's been a reorg (new < old), then walk through the reverted range collecting transactions. The sentinel UUID check at line 703 and the debug logs are good touches.
One thing though: if
old_finalized_noncesomehow ends up atU256::MAXand the loop reaches that value,saturating_addwill keep returning MAX, so thewhile nonce <= old_finalized_noncecondition never breaks. Yeah, U256::MAX is astronomically unlikely for a real nonce (we're talking 2^256 - 1 here), but it's an easy fix if you want to be extra safe.You could add a break before the increment:
} else { debug!( ?nonce, ?tx_uuid, "No transaction found for nonce in reorg range" ); } + if nonce == old_finalized_nonce { + break; + } nonce = nonce.saturating_add(U256::one()); }But honestly, this is more defensive programming than anything you'd actually hit in the real world.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs(1 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.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (3)
rust/main/lander/src/adapter/core.rs (3)
reprocess_txs_poll_rate(122-124)get_reprocess_txs(133-135)tx(69-73)rust/main/lander/src/adapter/chains/ethereum/nonce/updater.rs (1)
new(24-49)rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (1)
new(31-60)
⏰ 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). (12)
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: yarn-install
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: agent-configs (mainnet3)
- GitHub Check: lander-coverage
- GitHub Check: test-rs
- GitHub Check: lint-rs
🔇 Additional comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (1)
668-672: Nice polling rate logic here.The cap at 5 seconds makes sense—keeps the nonce queries from going wild on chains with real short block times. The comment explains the reasoning well, and the implementation's clean.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rust/main/lander/src/dispatcher/stages/inclusion_stage.rs (1)
210-213: Swallowing errors here is gonna make troubleshooting a proper headache.When
get_reprocess_txsfails, the wildcard match on line 212 just keeps trucking along without a peep. Could be a network hiccup, could be corrupted state—you won't know until things get really sideways.Log the error so you've got breadcrumbs when things go wrong:
let txs = match state.adapter.get_reprocess_txs().await { Ok(s) => s, - _ => continue, + Err(err) => { + warn!(?err, "Failed to fetch reprocess transactions, will retry"); + continue; + } };
🧹 Nitpick comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (1)
707-724: Nonce loop has a theoretical edge case, though it's about as likely as finding dragon eggs in your swamp.If
old_finalized_nonceever reachesU256::MAXand your loop counter hits it too, thesaturating_addon line 723 will keep returningMAXinstead of overflowing. The conditionnonce <= old_finalized_noncestays true forever, and you've got yourself an infinite loop.Now, reaching a nonce of 2^256 is... let's just say you'd need more transactions than atoms in the observable universe. So this is more about keeping the code theoretically sound than fixing something that'll actually bite you.
If you want the extra layer of protection, add a break before incrementing:
} + if nonce == old_finalized_nonce { + break; + } nonce = nonce.saturating_add(U256::one()); }
📜 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(1 hunks)rust/main/lander/src/dispatcher/stages/inclusion_stage.rs(2 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.rsrust/main/lander/src/dispatcher/stages/inclusion_stage.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (2)
rust/main/lander/src/adapter/core.rs (3)
reprocess_txs_poll_rate(122-124)get_reprocess_txs(133-135)tx(69-73)rust/main/lander/src/adapter/chains/ethereum/nonce/manager.rs (1)
new(31-60)
⏰ 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 (evm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
- GitHub Check: test-rs
🔇 Additional comments (3)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (1)
674-678: Poll rate logic looks solid.Capping at 5 seconds keeps you from hammering the nonce queries during reorgs. Clean and sensible.
rust/main/lander/src/dispatcher/stages/inclusion_stage.rs (2)
67-70: Task spawning looks good.Follows the same pattern as the other tasks, properly instrumented and all that.
187-197: Early exit logic is clean.If there's no poll rate configured, you bail out early. Makes sense for chains that don't support this yet.
…messages that need reprocessing (hyperlane-xyz#7197)
2 participants
Description
Add 2 new methods to AdaptsChain
reprocess_txs_poll_rate()Related issues
Summary by CodeRabbit
New Features
Chores
Tests