feat: check evm tx actually succeeded before determining its tx status#7016
feat: check evm tx actually succeeded before determining its tx status#7016
Conversation
|
📝 WalkthroughWalkthroughFetches and logs Ethereum transaction receipts, maps provider errors or missing receipts to Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Checker as TxStatusChecker
participant Node as Ethereum Node
Caller->>Checker: get_tx_hash_status(tx_hash)
Checker->>Node: eth_getTransactionReceipt(tx_hash)
alt Node returns Ok(Some(receipt))
Node-->>Checker: receipt (with status)
alt receipt.status == Some(0)
Note over Checker #ffecec: status 0 → Dropped (RevertedByChain)
Checker-->>Caller: Dropped(TransactionDropReason::RevertedByChain)
else receipt.status == Some(1) or status None
Note over Checker #eef9ff: delegate to block_number_result_to_tx_status
Checker->>Node: (maybe) fetch block number / confirmations
Node-->>Checker: block number / confirmations
Checker-->>Caller: Included or Finalized
end
else Node returns Ok(None)
Node-->>Checker: no receipt
Checker->>Node: fetch block number / confirmations
Node-->>Checker: block number / confirmations
Checker-->>Caller: Included or Finalized (via block_number_result_to_tx_status)
end
opt Node error
Node-->>Checker: Err(_)
Checker-->>Caller: LanderError::TxHashNotFound
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📜 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:
⏰ 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)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.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/tx_status_checker.rs
⏰ 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). (8)
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
- GitHub Check: test-rs
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmosnative)
🔇 Additional comments (2)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (2)
7-7: Import looks good.Bringing
TransactionDropReasoninto scope is fine here.
1-69: Run Clippy on the workspace
I couldn’t verify linting here—please runcargo clippy --workspace --all-targets -- -D warningsand resolve any warnings after the match refactor.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (1)
53-54: status=0 ≠ Dropped; it means executed-and-reverted. Treat explicitly as a failure, not “DroppedByChain”.Mapping reverts to “dropped” will mislead upstream logic (retries, user messaging). Also handle unexpected non {0,1} with a warn and non-terminal status.
Apply:
- _ => TransactionStatus::Dropped(TransactionDropReason::DroppedByChain), + Some(0) => { + warn!(?hash, "Tx executed but reverted (receipt.status=0 per EIP-658)"); + // Prefer a dedicated failure status if available: + TransactionStatus::Failed + // …or, if you model failures via drop reasons: + // TransactionStatus::Dropped(TransactionDropReason::ExecutionFailed) + }, + Some(other) => { + warn!(?hash, status = other, "Unexpected receipt.status; treating as pending"); + TransactionStatus::PendingInclusion + },If
TransactionStatus::FailedorExecutionFaileddoesn’t exist yet, I can help wire it through enums, serde, and callers.
🧹 Nitpick comments (3)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (3)
47-48: Nit: reword the EIP-658 comment.“Assuming all chains…” sounds absolute. Suggest clarifying the fallback behavior when
statusis absent.Apply:
- // Assuming all chains nowadays support this EIP: https://eips.ethereum.org/EIPS/eip-658 - // 1 = Success, 0 = Failure + // EIP-658: 1 = success, 0 = failure (revert). If `status` is absent, fall back to block-number logic.
56-57: Don’t squash provider errors into “TxHashNotFound”.Differentiate lookup miss vs provider/RPC error to avoid noisy “not found” handling upstream.
Consider:
- Err(err) => Err(LanderError::TxHashNotFound(err.to_string())), + Err(err) => Err(LanderError::Provider(err.to_string())),(or another variant that represents RPC failure).
60-172: Add coverage forstatus=Noneand unexpectedstatusvalues.Guard against edge-y chains and future quirks; assert the fallback and warn paths.
I can add:
- status=None → Included/Finalized per block-number logic
- status=5 → PendingInclusion + warn!(status=5)
Want me to push those tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.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/tx_status_checker.rs
⏰ 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 (4)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (4)
46-51: Good call: comparestatusviaas_u64()and keep theNonefallback.Much cleaner than peeking into
U64internals. Aye, tidy.
1-173: Run Clippy on the Lander crate
I couldn’t invoke Clippy in this environment—please run locally:cd rust/main/lander cargo clippy --all-features -- -D warnings
129-134: Swap MockProvider push order
Looks likeget_tx_hash_statusfetches the receipt first, then the block number. Push your mocked receipt beforeU64::from(23327790u64)(intx_status_checker.rsat lines 129–134) so they line up and avoid any deserialization mix-ups. Run the tests locally to double-check.
156-159: Swap pushes: receipt before block
Inrust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs(lines 156–159), push the tx receipt before the finalized block:- let _ = mock_provider.push(U64::from(23328000u64)); - let tx_receipt = test_tx_receipt(transaction_hash, Some(U64::from(1))); - let _ = mock_provider.push(tx_receipt); + let tx_receipt = test_tx_receipt(transaction_hash, Some(U64::from(1))); + let _ = mock_provider.push(tx_receipt); + let _ = mock_provider.push(U64::from(23328000u64));Due to sandbox limitations, please verify locally with:
cargo test -p lander -- tx_status_checker
There was a problem hiding this comment.
Actionable comments posted: 1
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/tests.rs (1)
349-370: Another test with mismatched expectations.Same story here - you're pattern-matching for
RevertedByChain(lines 349-351) but the metrics assertion expects"FailedSimulation"strings (lines 366-367). These need to line up.Apply this diff to fix the metrics assertion:
let metrics_assertion = MetricsAssertion { domain: entrypoint.inner.domain.clone(), finalized_txs: 0, building_stage_queue_length: 0, inclusion_stage_pool_length: 0, finality_stage_pool_length: 0, dropped_payloads: 1, dropped_transactions: 1, - dropped_payload_reason: "DroppedInTransaction(FailedSimulation)".to_string(), - dropped_transaction_reason: "FailedSimulation".to_string(), + dropped_payload_reason: "DroppedInTransaction(RevertedByChain)".to_string(), + dropped_transaction_reason: "RevertedByChain".to_string(), transaction_submissions: 0, };
🧹 Nitpick comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (1)
46-54: Pattern matching looks solid but could be more explicit.The use of
.as_u64()avoids peeking into the type's internals - that's proper. DelegatingSome(1) | Noneto the block finality check follows EIP-658 semantics.However, the catch-all
_pattern lumps togetherSome(0)(explicit failure) with any unexpected values. Consider being more explicit:- Ok(match receipt.status.as_ref().map(|s| s.as_u64()) { - // Assuming all chains nowadays support this EIP: https://eips.ethereum.org/EIPS/eip-658 - // 1 = Success, 0 = Failure - Some(1) | None => { - block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) - .await - } - _ => TransactionStatus::Dropped(TransactionDropReason::RevertedByChain), - }) + Ok(match receipt.status.as_ref().map(|s| s.as_u64()) { + // EIP-658: 1 = Success, 0 = Failure + Some(1) | None => { + block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) + .await + } + Some(0) => { + // Explicit EIP-658 failure + TransactionStatus::Dropped(TransactionDropReason::RevertedByChain) + } + Some(other) => { + warn!(?hash, status = other, "Unexpected receipt.status value; treating as reverted"); + TransactionStatus::Dropped(TransactionDropReason::RevertedByChain) + } + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs(2 hunks)rust/main/lander/src/dispatcher/tests.rs(3 hunks)rust/main/lander/src/transaction/types.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/dispatcher/tests.rsrust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rsrust/main/lander/src/transaction/types.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (3)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (2)
new(74-127)get_tx_hash_status(607-612)rust/main/lander/src/adapter/chains/sealevel/adapter.rs (3)
new(77-118)get_tx_hash_status(247-310)get_tx_hash_status(419-424)rust/main/lander/src/adapter/core.rs (2)
tx_status(62-79)get_tx_hash_status(58-58)
⏰ 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: lander-coverage
- GitHub Check: lint-rs
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmwasm)
🔇 Additional comments (4)
rust/main/lander/src/transaction/types.rs (1)
105-106: Good addition of the RevertedByChain variant.The new variant clearly distinguishes on-chain execution failures from reorg-induced drops. Nice and tidy.
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (2)
123-146: Test for successful transaction looks good.The mock setup and assertions for
status=1yieldingIncludedare spot on.
148-171: Finalized transaction test checks out.Properly tests the
status=1with sufficient finality to returnFinalized. All clear.rust/main/lander/src/dispatcher/tests.rs (1)
269-322: Critical mismatch between mock return value and test assertions.The test sets up a mock that returns
TransactionDropReason::RevertedByChain(line 282), but then asserts that the metrics will show"FailedSimulation"(lines 317-318). The comment at lines 307-308 mentions hardcoding toFailedSimulation, but that doesn't match what your mock is doing here.Either update the mock to return what the system actually produces, or fix the metrics assertion to match the
RevertedByChaindrop reason. Right now this test will fail or pass for the wrong reasons.Apply this diff to align the metrics assertion with the mock:
- let metrics_assertion = MetricsAssertion { - domain: entrypoint.inner.domain.clone(), - finalized_txs: 0, - building_stage_queue_length: 0, - inclusion_stage_pool_length: 0, - finality_stage_pool_length: 0, - dropped_payloads: 1, - dropped_transactions: 1, - dropped_payload_reason: "DroppedInTransaction(FailedSimulation)".to_string(), - dropped_transaction_reason: "FailedSimulation".to_string(), - transaction_submissions: 0, - }; + let metrics_assertion = MetricsAssertion { + domain: entrypoint.inner.domain.clone(), + finalized_txs: 0, + building_stage_queue_length: 0, + inclusion_stage_pool_length: 0, + finality_stage_pool_length: 0, + dropped_payloads: 1, + dropped_transactions: 1, + dropped_payload_reason: "DroppedInTransaction(RevertedByChain)".to_string(), + dropped_transaction_reason: "RevertedByChain".to_string(), + transaction_submissions: 0, + };Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (2)
46-54: Nice fix on the U64 handling and EIP-658 semantics.The logic properly distinguishes successful transactions (status=1 or None) from reverted ones (status=0), returning
RevertedByChainfor the latter. This addresses all the previous feedback.One small thing to consider: the wildcard at line 53 catches everything that's not 1 or None. Per EIP-658, you should only see 0 or 1, but if some chain returns, say, 2 or 42, you'd silently treat it as reverted. Might be worth logging unexpected values for debugging.
If you want to add that safety net, something like this:
- Ok(match receipt.status.as_ref().map(|s| s.as_u64()) { + let status_value = receipt.status.as_ref().map(|s| s.as_u64()); + Ok(match status_value { // Assuming all chains nowadays support this EIP: https://eips.ethereum.org/EIPS/eip-658 // 1 = Success, 0 = Failure Some(1) | None => { block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) .await } - _ => TransactionStatus::Dropped(TransactionDropReason::RevertedByChain), + Some(0) => TransactionStatus::Dropped(TransactionDropReason::RevertedByChain), + Some(other) => { + warn!(?hash, status = other, "Unexpected receipt.status value per EIP-658"); + TransactionStatus::Dropped(TransactionDropReason::RevertedByChain) + } })
98-173: These tests look solid.All three test cases properly validate the updated logic:
- Failed tx (status=0) correctly asserts
RevertedByChain- Successful txs (status=1) correctly assert
IncludedorFinalizedbased on block finalityThe test assertions now match the implementation, which resolves the previous mismatch.
If you're feeling thorough, you could add one more test for the
status=Nonecase (pre-EIP-658 chains that don't report status). Your comment assumes all chains support EIP-658 these days, so this is more of a belt-and-suspenders thing.Something like:
#[tokio::test] async fn test_get_tx_hash_status_no_status_field() { let transaction_hash = H256::from_str("575841942e0de82d3129cccf53e4e9c75b6d8a163f8a83d330a2e8d574820a4d") .unwrap(); let mock_provider = MockProvider::new(); let _ = mock_provider.push(U64::from(23327790u64)); let tx_receipt = test_tx_receipt(transaction_hash, None); let _ = mock_provider.push(tx_receipt); let ethers_provider = Provider::new(mock_provider); let evm_provider: Arc<dyn EvmProviderForLander> = Arc::new(EthereumProvider::new( Arc::new(ethers_provider), HyperlaneDomain::Known(KnownHyperlaneDomain::Ethereum), )); let reorg_period = EthereumReorgPeriod::Blocks(15); let tx_status = get_tx_hash_status(&evm_provider, transaction_hash.into(), &reorg_period) .await .unwrap(); assert_eq!(tx_status, TransactionStatus::Included); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.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/tx_status_checker.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (2)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (2)
new(74-127)get_tx_hash_status(607-612)rust/main/lander/src/adapter/chains/sealevel/adapter.rs (3)
new(77-118)get_tx_hash_status(247-310)get_tx_hash_status(419-424)
⏰ 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: infra-test
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: agent-configs (mainnet3)
- GitHub Check: test-rs
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
- GitHub Check: build-and-push-to-gcr
🔇 Additional comments (2)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (2)
7-7: LGTM!The
TransactionDropReasonimport is spot on for the newRevertedByChainhandling.
73-94: LGTM!Clean helper function for building test receipts. Makes the test setup much easier to follow.
|
It looks like e2e tests for EVM do not pass. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (2)
45-56: Much better now.The previous concerns about destructuring
U64internals and misclassifyingstatus=0have been properly addressed. The code now correctly uses.as_u64()and maps a failed transaction toRevertedByChain.One thing though - the wildcard pattern treats
Some(1),None, and any unexpected values (likeSome(2)) all the same. Per EIP-658, only 0 and 1 are defined, so unexpected values shouldn't happen... but if they do, you won't know about it.You could make the handling more explicit if you want:
- Ok(Some(receipt)) => match receipt.status.as_ref().map(|s| s.as_u64()) { - // https://eips.ethereum.org/EIPS/eip-658 - Some(0) => Ok(TransactionStatus::Dropped( - TransactionDropReason::RevertedByChain, - )), - _ => { - let res = - block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) - .await; - Ok(res) - } - }, + Ok(Some(receipt)) => match receipt.status.as_ref().map(|s| s.as_u64()) { + // https://eips.ethereum.org/EIPS/eip-658 + Some(0) => { + warn!(?hash, "Transaction reverted on-chain (status=0)"); + Ok(TransactionStatus::Dropped( + TransactionDropReason::RevertedByChain, + )) + } + Some(1) | None => { + let res = + block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) + .await; + Ok(res) + } + Some(other) => { + warn!(?hash, status = other, "Unexpected receipt status; treating as success"); + let res = + block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) + .await; + Ok(res) + } + },
61-175: Nice coverage.The tests cover the main paths: failed tx (status=0), successful included tx, and successful finalized tx. The mock setup looks solid.
You could add one more test for backward compatibility with pre-EIP-658 chains (where
statusisNone). Just to make sure that path doesn't break:#[tokio::test] async fn test_get_tx_hash_status_pre_eip658() { let transaction_hash = H256::from_str("575841942e0de82d3129cccf53e4e9c75b6d8a163f8a83d330a2e8d574820a4d") .unwrap(); let mock_provider = MockProvider::new(); let _ = mock_provider.push(U64::from(23327790u64)); let tx_receipt = test_tx_receipt(transaction_hash, None); let _ = mock_provider.push(tx_receipt); let ethers_provider = Provider::new(mock_provider); let evm_provider: Arc<dyn EvmProviderForLander> = Arc::new(EthereumProvider::new( Arc::new(ethers_provider), HyperlaneDomain::Known(KnownHyperlaneDomain::Ethereum), )); let reorg_period = EthereumReorgPeriod::Blocks(15); let tx_status = get_tx_hash_status(&evm_provider, transaction_hash.into(), &reorg_period) .await .unwrap(); assert_eq!(tx_status, TransactionStatus::Included); }Also heads up - the PR objectives mention that e2e tests for EVM don't pass. Might want to check what's going on there, though these unit tests look fine to me.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.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/tx_status_checker.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (3)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (2)
new(74-127)get_tx_hash_status(607-612)rust/main/lander/src/adapter/chains/sealevel/adapter.rs (3)
new(77-118)get_tx_hash_status(247-310)get_tx_hash_status(419-424)rust/main/lander/src/adapter/core.rs (2)
tx_status(62-79)get_tx_hash_status(58-58)
⏰ 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: lint-rs
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: test-rs
- GitHub Check: lander-coverage
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmosnative)
🔇 Additional comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (1)
7-7: Good add.The
TransactionDropReasonimport is needed for the newRevertedByChainvariant. Clean.
Fixed |
…-monorepo into jeff/evm-confirm
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (2)
48-59: Consider more explicit pattern matching for status values.Right now the catch-all
_lumps togetherSome(1), unexpected values likeSome(2), andNone. While it works, you're missing a chance to catch weird status values that shouldn't exist per EIP-658.Here's a more defensive approach:
- match receipt.status.as_ref().map(|s| s.as_u64()) { - // https://eips.ethereum.org/EIPS/eip-658 - Some(0) => Ok(TransactionStatus::Dropped( - TransactionDropReason::RevertedByChain, - )), - _ => { - let res = - block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) - .await; - Ok(res) - } - } + match receipt.status.as_ref().map(|s| s.as_u64()) { + // https://eips.ethereum.org/EIPS/eip-658 + Some(0) => Ok(TransactionStatus::Dropped( + TransactionDropReason::RevertedByChain, + )), + Some(1) => { + let res = + block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) + .await; + Ok(res) + } + Some(other) => { + warn!(?hash, status = other, "Unexpected receipt status value; treating as successful"); + let res = + block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) + .await; + Ok(res) + } + None => { + // Pre-EIP-658 chain or status not available + let res = + block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) + .await; + Ok(res) + } + }This way you'll spot any chains doing something funky with status codes.
62-176: Consider adding test coverage for None status.You've got tests for status=0 and status=1, but nothing for when
receipt.statusisNone. That's a valid scenario for pre-EIP-658 chains or when status isn't available, and it'd be good to verify it falls back to the block-number logic correctly.Add a test like:
#[tokio::test] async fn test_get_tx_hash_status_no_status_field() { let transaction_hash = H256::from_str("575841942e0de82d3129cccf53e4e9c75b6d8a163f8a83d330a2e8d574820a4d") .unwrap(); let mock_provider = MockProvider::new(); let _ = mock_provider.push(U64::from(23327774u64)); let tx_receipt = test_tx_receipt(transaction_hash, None); let _ = mock_provider.push(tx_receipt); let ethers_provider = Provider::new(mock_provider); let evm_provider: Arc<dyn EvmProviderForLander> = Arc::new(EthereumProvider::new( Arc::new(ethers_provider), HyperlaneDomain::Known(KnownHyperlaneDomain::Ethereum), )); let reorg_period = EthereumReorgPeriod::Blocks(15); let tx_status = get_tx_hash_status(&evm_provider, transaction_hash.into(), &reorg_period) .await .unwrap(); assert_eq!(tx_status, TransactionStatus::Included); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.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/tx_status_checker.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (3)
rust/main/lander/src/adapter/chains/sealevel/adapter.rs (3)
new(78-119)get_tx_hash_status(248-311)get_tx_hash_status(420-425)rust/main/lander/src/adapter/chains/ethereum/adapter.rs (2)
new(74-127)get_tx_hash_status(607-612)rust/main/lander/src/adapter/core.rs (2)
tx_status(62-79)get_tx_hash_status(58-58)
⏰ 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 (evm)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: lint-rs
- GitHub Check: test-rs
- GitHub Check: lander-coverage
🔇 Additional comments (4)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (4)
7-7: Good addition of proper error handling.The import and receipt fetching logic look solid. Mapping provider errors and None receipts to
TxHashNotFoundis the right move here.Also applies to: 41-47
75-96: Clean test helper.The
test_tx_receiptfunction is well-structured and makes the tests readable. Good stuff.
152-175: Third test looks good.The finalized test is correct—transaction in block 23327789 with finalized block at 23328000 properly returns
Finalized.
127-150: Test assertion doesn't match the logic.Look, in this test you're setting the finalized block to 23327790 and the transaction block to 23327789. The code checks
if finalized_block >= block_number, which is23327790 >= 23327789= true. That means it returnsFinalized, notIncluded.Fix it like this:
- let _ = mock_provider.push(U64::from(23327790u64)); + let _ = mock_provider.push(U64::from(23327774u64)); let tx_receipt = test_tx_receipt(transaction_hash, Some(U64::from(1)));This sets finalized to 23327774, which is 15 blocks before the tx block (23327789), so the tx will be Included but not yet Finalized.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (1)
48-59: EIP-658 handling is correct, but consider logging unexpected values.You've addressed the past feedback properly—using
.as_u64()instead of destructuring, and mapping status=0 toRevertedByChain. That's the right call.The catch-all
_at line 53 treats all non-zero statuses (including status=1 and unexpected values like status=2) the same way. Per EIP-658, only 0 and 1 should appear, but if something weird shows up, you might want to know about it.Consider adding a warning for unexpected status values:
match receipt.status.as_ref().map(|s| s.as_u64()) { // https://eips.ethereum.org/EIPS/eip-658 Some(0) => Ok(TransactionStatus::Dropped( TransactionDropReason::RevertedByChain, )), + Some(1) => { + let res = + block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) + .await; + Ok(res) + } + Some(other) => { + warn!(?hash, status = other, "Unexpected receipt.status per EIP-658; treating as success"); + let res = + block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) + .await; + Ok(res) + } - _ => { + None => { let res = block_number_result_to_tx_status(provider, receipt.block_number, reorg_period) .await; Ok(res) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.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/tx_status_checker.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (2)
rust/main/lander/src/adapter/chains/sealevel/adapter.rs (3)
new(78-119)get_tx_hash_status(248-311)get_tx_hash_status(420-425)rust/main/lander/src/adapter/chains/ethereum/adapter.rs (2)
new(74-127)get_tx_hash_status(607-612)
⏰ 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: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: build-and-push-to-gcr
- GitHub Check: lint-rs
- GitHub Check: test-rs
- GitHub Check: lander-coverage
🔇 Additional comments (3)
rust/main/lander/src/adapter/chains/ethereum/adapter/tx_status_checker.rs (3)
7-7: Import looks good.The addition of
TransactionDropReasonto the imports is necessary for the newRevertedByChainhandling. Well done addressing the previous feedback.
41-47: Receipt retrieval and error handling look solid.The error mapping to
TxHashNotFoundfor both provider errors and missing receipts is appropriate. The debug logging gives you visibility into what's going on.
62-176: Tests cover the key scenarios.You've got good coverage here:
- Failed tx (status=0) correctly maps to
Dropped(RevertedByChain)- Successful tx (status=1) maps to
Includedwhen not finalized- Successful tx (status=1) maps to
Finalizedwhen past the reorg periodThe test setup with
MockProvideris clean and the assertions match the implementation. Nice work.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7016 +/- ##
=====================================
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
Assuming all evm chains nowadays support https://eips.ethereum.org/EIPS/eip-658.
We should check whether the tx actually successfully landed or not.
Related issues
Summary by CodeRabbit
Enhancements
Bug Fixes
Tests
Chores / Public API