feat: Lander Transaction Submission Observability #7393
Conversation
|
📝 WalkthroughWalkthroughThis change adds a Submit stage and a PendingOperationStatus::ReadyToSubmit path, refactors disposition mapping into Submit/PostSubmitSuccess/PostSubmitFailure, routes operations through Prepare → Submit → Confirm via new filtering functions, updates signatures to accept a submit queue, and expands tests and test utilities. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,200,0.3)
participant LP as LanderTask
participant PREP as Prepare
participant SQ as SubmitQueue
participant SUB as Submit
participant CQ as ConfirmQueue
participant EP as Entrypoint/DB
end
LP->>PREP: batch -> filter_operations_for_preparation
PREP->>EP: query payload/tx status
alt PreSubmit
PREP->>LP: return ops for payload resubmit
else Submit
PREP->>SQ: enqueue op as ReadyToSubmit
else PostSubmitSuccess/PostSubmitFailure
PREP->>CQ: enqueue for confirmation or retry/linkage removal
end
SQ->>SUB: ReadyToSubmit batch -> filter_operations_for_submit
SUB->>EP: re-query payload/tx status
alt PreSubmit
SUB->>SUB: return ops for immediate resubmit
else Submit
SUB->>SQ: re-queue op as ReadyToSubmit
else PostSubmitSuccess/PostSubmitFailure
SUB->>CQ: enqueue for confirmation or retry handling
end
CQ->>EP: confirmation checks / finalize
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (10)
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 (5)
rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs (1)
22-70: Submit-stage routing matches disposition model; consider optional parallelismThis function does what it says on the tin: it reuses
operation_disposition_by_payload_statusand cleanly routesPreSubmitops back to the caller, keepsSubmitones in the submit queue withReadyToSubmit, and pushesPostSubmitthroughconfirm_op. Nice and easy to follow.If batches ever get large or payload status calls are slow, you might eventually want to fan these lookups out (e.g.
futures::future::join_allover the ops) instead of awaiting each one in sequence, but that’s polish rather than a blocker.rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs (1)
18-915: Submit-stage tests nicely pin down all status pathsThis suite hammers
filter_operations_for_submitfrom every angle: errors, all drop reasons, retry, the three Submit‑class statuses, included/finalized, empty/mixed batches, and queue state after routing. The expectations match the production logic (PreSubmit → returned, Submit → re‑queued, PostSubmit → confirm), so behavior is well‑pinned.If you ever get tired of repeating the same setup dance, you could pull some of the “one op + status X → expected queue/vec state” patterns into helpers, but as it stands it’s readable and does the job.
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs (2)
245-312: Submitted vs non‑submitted mixed batches align with disposition mappingThe “all submitted”, “none submitted”, and mixed‑batch tests correctly exercise Finalized vs not‑found vs PendingInclusion cases, and the asserts on result lengths and confirm/submit queue sizes match the
OperationDispositionmapping. There’s some repetition across these setups, but nothing that hurts readability; factoring a small helper for common batch construction is optional.Also applies to: 314-356, 359-461
744-1050: New tests for Submit/PostSubmit variants thoroughly exercise routingThe ReadyToSubmit, Mempool, Included, “all to submit”, and mixed‑Submit‑variant tests nicely prove that Submit dispositions stay in the submit queue and PostSubmit ones move to confirm. If you ever want to tighten things further, a tiny helper to peek IDs or statuses in the queues (not just lengths) could catch mis‑routing bugs early, but as is this already covers the main behavior well.
rust/main/agents/relayer/src/msg/message_processor.rs (1)
557-594: Lander submit loop and submit_via_lander keep ops in the submit pipeline until inclusionThe new
submit_lander_taskpattern—pop batch, filter viafilter_operations_for_submit, sleep when nothing is PreSubmit, then callsubmit_via_landeron the rest—gives a clear separation between “needs payload” and “waiting on inclusion”. Havingsubmit_via_landerstore the payload mapping and push the op back withReadyToSubmitensures the operation sticks in the submit queue until status flips to Included/Finalized andfilter_operations_for_submithands it off toconfirm_op. Only small thought: if you ever want to track “sent to Lander” separately from “moved to confirm queue”, an extra metric hook here could help, but functionally this flow looks sound.Also applies to: 596-602, 652-659
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
rust/main/agents/relayer/src/msg/message_processor.rs(6 hunks)rust/main/agents/relayer/src/msg/message_processor/disposition.rs(2 hunks)rust/main/agents/relayer/src/msg/message_processor/disposition/tests.rs(4 hunks)rust/main/agents/relayer/src/msg/message_processor/stage.rs(1 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs(3 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs(17 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs(30 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs(1 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs(1 hunks)rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-02T13:58:07.164Z
Learnt from: daniel-savu
Repo: hyperlane-xyz/hyperlane-monorepo PR: 6668
File: rust/main/lander/src/dispatcher/stages/building_stage/building.rs:41-41
Timestamp: 2025-07-02T13:58:07.164Z
Learning: In Rust lander BuildingStage, the update_metrics() call should be positioned after checking if payloads are empty to avoid unnecessary metrics updates during idle polling loops. The queue.len() operation is lightweight for VecDeque-based queues.
Applied to files:
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rsrust/main/agents/relayer/src/msg/message_processor.rs
🧬 Code graph analysis (9)
rust/main/agents/relayer/src/msg/message_processor/stage.rs (1)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (1)
submit(105-107)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (1)
id(59-61)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (3)
rust/main/agents/relayer/src/msg/message_processor.rs (2)
new(106-147)new(991-1003)rust/main/hyperlane-core/src/chain.rs (1)
new_test_domain(333-341)rust/main/hyperlane-base/src/metrics/core.rs (1)
processor_queue_length(594-596)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
rust/main/agents/relayer/src/msg/message_processor.rs (2)
batch(466-466)batch(805-811)
rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs (2)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (6)
submit(105-107)create_test_metrics(185-214)create_test_queue(164-182)new(33-39)id(59-61)with_first_prepare(41-44)rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs (1)
filter_operations_for_submit(22-70)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs (3)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (3)
create_test_queue(164-182)new(33-39)with_first_prepare(41-44)rust/main/hyperlane-core/src/chain.rs (1)
new_test_domain(333-341)rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
filter_operations_for_preparation(24-63)
rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs (2)
rust/main/agents/relayer/src/msg/message_processor.rs (1)
confirm_op(712-737)rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
operation_disposition_by_payload_status(25-73)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs (3)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (4)
create_test_queue(164-182)new(33-39)with_first_prepare(41-44)id(59-61)rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
filter_operations_for_preparation(24-63)rust/main/hyperlane-core/src/chain.rs (1)
new_test_domain(333-341)
rust/main/agents/relayer/src/msg/message_processor.rs (2)
rust/main/agents/relayer/src/msg/op_batch.rs (1)
submit(30-52)rust/main/agents/relayer/src/msg/pending_message.rs (1)
submit(375-422)
⏰ 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). (10)
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: lander-coverage
- GitHub Check: test-rs
- GitHub Check: lint-rs
🔇 Additional comments (15)
rust/main/agents/relayer/src/msg/message_processor/stage.rs (1)
1-2: Submit stage wiring looks fineThe new
submitmodule is wired in cleanly alongsideprepare, matching the new stage implementation understage::submit.rust/main/agents/relayer/src/msg/message_processor/disposition/tests.rs (1)
225-228: Disposition expectations now line up with routing semanticsThe updated expectations for
PendingInclusion,ReadyToSubmit,Retry, andMempoolmap neatly onto the new disposition logic (Submitfor in‑pipeline,PreSubmitfor retryable states). That keeps these tests in step withoperation_disposition_by_payload_statusand the new submit/post‑submit routing.Also applies to: 380-383, 417-420, 454-457
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs (1)
7-714: Thorough disposition coverage around prepare stageThese tests now walk through pretty much every path: manual vs non‑manual retry, DB failures, all payload and transaction drop reasons, reorgs, in‑flight submission states, included/finalized cases, multiple UUIDs, and entrypoint errors. The expectations around “returned vs queued (submit/confirm)” line up with the disposition semantics and the updated
filter_operations_for_preparation.Nothing stands out as off; this is solid safety net for the swampy edge cases.
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs (4)
8-10: Submit queue wiring and empty‑batch behavior look consistentBringing in
PendingOperationStatusand the extrasubmit_queueparameter, plus asserting both queues are empty for an empty batch, lines up cleanly with the new routing semantics and keeps the base case honest. Nothing funky hiding in this setup.Also applies to: 26-45
51-98: Manual retry and DB‑failure scenarios are well coveredThe manual‑retry tests (including store failures and mixed batches) match the
determine_operation_dispositionbehavior: manual ops are forced back to PreSubmit even when DB calls fail, while submitted ops flow to the confirm queue. The expectations on DB call counts and queue lengths look accurate and give good confidence in this corner of the swamp.Also applies to: 100-152, 155-243
463-507: Error, dropped, and non‑manual retry paths behave as intendedDB errors, dropped payloads/transactions, entrypoint errors, and non‑manual retries are all explicitly tested and end up either back in PreSubmit or in the confirm queue when finalized, matching the documented behavior. This gives solid coverage of the messier failure paths without any obvious mismatches.
Also applies to: 509-601, 603-700
703-742: Empty payload UUIDs edge case is handled cleanlyTreating
Some(vec![])as PreSubmit and asserting that no queues are touched is a good little edge‑case guard, and mirrors the disposition logic exactly. No tweaks needed here.rust/main/agents/relayer/src/msg/message_processor.rs (2)
22-26: Importing ReadyToSubmit and submit‑stage filter fits the new pipelinePulling in
PendingOperationStatus::ReadyToSubmitandfilter_operations_for_submitwires this module into the new three‑stage flow cleanly, without changing existing classic behavior. Names and placement make it easy to follow how the submit stage now owns the “keep in submit queue vs move to confirm” decision.Also applies to: 38-40
408-425: Using filter_operations_for_preparation in Lander prepare task matches disposition semanticsPassing both
submit_queueandconfirm_queueintoprepare::filter_operations_for_preparationlets the Lander prepare loop offload already‑submitted ops and queue Submit ones, while only handing true PreSubmit operations toprocess_batch. The manual‑retry cleanup indetermine_operation_dispositionplus this call ensure we don’t keep stale payload mappings hanging around.rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (2)
20-21: Extending MockQueueOperation with destination makes tests more faithfulAdding a
destination: HyperlaneDomain, plumbing it throughnew, and havingwith_first_prepare/with_manual_retryusenew_test_domain("test")lets tests exercise destination‑aware logic without extra boilerplate. Returning&self.destinationfromdestination_domainfinally lines the mock up with the real trait, which is exactly what you want here.Also applies to: 29-38, 41-53, 77-79
183-214: create_test_metrics mirrors production metrics layout cleanlyThe
create_test_metricshelper sets up gauges and counters with the same label shapeMessageProcessorMetricsexpects, and cloningops_processedinto the different phases keeps the test wiring simple while still realistic. Handy little utility for future tests poking at metrics behavior.rust/main/agents/relayer/src/msg/message_processor/disposition.rs (2)
3-4: Adding Submit disposition and docs clarifies the operation lifecycleIntroducing the
Submitvariant and spelling out in the doc comment how PreSubmit/Submit/PostSubmit are interpreted makes the routing rules a lot easier to reason about. The import changes are minimal and keep all the relevant status types in one place.Also applies to: 7-7, 10-17, 19-25
30-40: Disposition logic for payload status is reasonable and defensiveThe function now gracefully falls back to PreSubmit on DB errors, missing/empty UUIDs, or payload‑status lookup failures, while mapping ReadyToSubmit/PendingInclusion/Mempool into Submit and Included/Finalized into PostSubmit. That matches how the prepare and submit stages are using it and avoids losing ops when the DB or entrypoint stumbles, at the cost of a harmless extra round of work.
Also applies to: 42-47, 49-56, 58-72
rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (2)
7-8: filter_operations_for_preparation cleanly splits Pre/Submit/Post pathsTaking
submit_queuealongsideconfirm_queueand switching to a two‑phase disposition+routing loop means PreSubmit ops go back to preparation while Submit ones stay in the submit queue withReadyToSubmitand PostSubmit ones move straight to confirm. The comments match the behavior, and the use of theConfirm(AlreadySubmitted)status ties in nicely with the rest of the processor.Also applies to: 15-21, 24-32, 41-63
70-88: Manual retry handling avoids stale payload linkage before dispositionClearing the message‑to‑payload mapping when the status is
Retry(ReprepareReason::Manual)ensures manual interventions don’t get stuck behind old payload state, and the warning on store failure keeps some visibility without changing behavior. Letting the sharedoperation_disposition_by_payload_statuslogic run after that keeps the rest of the story simple.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (2)
89-97: Multi-payload behavior: consider logging or asserting when more than one UUID existsYou early‑return
PreSubmitonNone/empty and then blindly takepayload_uuids[0], with a comment noting only a single payload per message is supported. If multiple UUIDs ever show up, that’s a bit of a swampy edge case that will be silently ignored. Optional: log whenuuids.len() > 1or assert that we only ever see one, so it’s easier to spot if assumptions change.
97-103: Entry-point RPC errors also mapped toPreSubmit– double-check desired behaviorSame as with the DB error path, any
payload_statusRPC failure hitsPreSubmit. In the Submit stage that again means “caller should re‑prepare / re‑submit now”, which could be a bit aggressive if the failure is a transient RPC hiccup and the original tx is actually fine. If you’d rather “wait it out” for this case, mapping RPC failures toSubmit(or a future explicit “retry later” disposition) might better match intent; if you’re comfortable re-prepping here, then this is good as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (1)
id(59-61)
⏰ 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 (evm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: yarn-install
- GitHub Check: build-and-push-to-gcr
- GitHub Check: lander-coverage
- GitHub Check: lint-rs
- GitHub Check: test-rs
🔇 Additional comments (3)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (3)
9-63: Enum docs read clearly and match the three-stage flowThe expanded docs make the pipeline behavior much less murky and clarify how the same disposition is interpreted in Prepare vs Submit. Nothing smells off in the variant semantics; naming plus comments feel sufficient now for multi-stage usage.
66-87: Confirm that treating DB errors asPreSubmitis intentional for Submit stageRight now any failure to read payload UUIDs (
retrieve_payload_uuids_by_message_idreturningErr) causes aPreSubmitdisposition, which in the Submit stage means “go ahead and re‑prepare / re‑submit”. That’s a reasonable fallback if duplicates are tolerated, but if a transient DB issue could lead to unnecessary re-prep or extra tx attempts, you might instead want to keep the op in the submit queue (or add a distinct “error/retry later” path).
105-118: Status → disposition mapping looks consistent with pipeline semanticsThe mapping from
PayloadStatus/TransactionStatusintoPreSubmit/Submit/PostSubmitlines up with the docs: dropped/retry →PreSubmit, ready/pending/mempool →Submit, included/finalized →PostSubmit. Nothing funky here; this should give clear observability for where each op sits in the pipeline.
rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs
Outdated
Show resolved
Hide resolved
|
These changes will also affect classical submitter right? Its not only limited to lander or am I wrong? |
|
@yjamin, why do you think that this change impacts Classic submitter as well? |
I don't know about the details, I just see that a lot of code has changed in the relayer and not in the lander. So I assumed that this code is shared between lander and classical as I would expect lander specific code to live in lander. |
|
I see what you mean, @yjamin. I have changed only
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs (1)
297-347: Function name doesn't match what it actually tests, mate.The function is named
test_determine_disposition_returns_presubmit_for_dropped_states, but it's actually verifying that dropped states returnPostSubmitFailure, notPreSubmit. The comment and assertion are spot on—it's just the function name that's sending mixed signals.Consider renaming it:
-async fn test_determine_disposition_returns_presubmit_for_dropped_states() { - // Test that dropped states return PostSubmitFailure disposition - // which causes the operation to be prepared with linkage removed +async fn test_determine_disposition_returns_postsubmit_failure_for_dropped_states() { + // Test that dropped states return PostSubmitFailure disposition + // which causes the operation to be prepared with linkage removed
🧹 Nitpick comments (1)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
19-27: Doc still mentions old PostSubmit variantThe stage docs still talk about a
PostSubmitbucket, but the enum now splits that path intoPostSubmitSuccessandPostSubmitFailure. Let’s tweak the wording so the comments line up with the actual variants, else future readers will be wandering the swamp looking for a type that no longer exists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs(2 hunks)rust/main/agents/relayer/src/msg/message_processor/disposition/tests.rs(9 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs(3 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs(8 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs(32 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs(1 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T12:20:42.663Z
Learnt from: yjamin
Repo: hyperlane-xyz/hyperlane-monorepo PR: 7414
File: rust/main/chains/hyperlane-aleo/src/provider/traits.rs:252-0
Timestamp: 2025-11-20T12:20:42.663Z
Learning: In rust/main/chains/hyperlane-aleo/src/provider/traits.rs, the `get_state_paths_for_commitments` and `get_state_paths_for_commitments_async` methods intentionally use `.unwrap_or_default()` to return an empty Vec on any error. This behavior matches SnarkVM's internal QueryTrait implementation and should not be changed to propagate errors.
Applied to files:
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs
🧬 Code graph analysis (6)
rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs (2)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (5)
create_test_metrics(185-214)create_test_queue(164-182)new(33-39)id(59-61)with_first_prepare(41-44)rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs (1)
filter_operations_for_submit(38-84)
rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs (2)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
operation_disposition_by_payload_status(96-144)rust/main/agents/relayer/src/msg/message_processor.rs (5)
confirm_op(712-737)batch(466-466)batch(805-811)new(106-147)new(991-1003)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (2)
id(59-61)status(96-98)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
operation_disposition_by_payload_status(96-144)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs (3)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
determine_operation_disposition(98-114)rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (2)
new(33-39)with_first_prepare(41-44)rust/main/hyperlane-core/src/chain.rs (1)
new_test_domain(333-341)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs (3)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (4)
create_test_queue(164-182)new(33-39)id(59-61)with_first_prepare(41-44)rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
filter_operations_for_preparation(32-77)rust/main/hyperlane-core/src/chain.rs (1)
new_test_domain(333-341)
⏰ 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). (10)
- 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: e2e-matrix (cosmosnative)
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
- GitHub Check: test-rs
🔇 Additional comments (6)
rust/main/agents/relayer/src/msg/message_processor/disposition/tests.rs (1)
143-147: Tests line up with the new failure dispositionNice to see the dropped-payload case now asserting
PostSubmitFailure; that keeps the safety net tight around the new enum split.rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs (1)
224-311: Great coverage across the submit routing pathsAppreciate how these cases walk through retries, drops, and finalized payloads—makes it a breeze to reason about the new stage wiring.
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs (1)
420-461: Solid assertions on where each disposition landsThese checks on confirm vs submit queue lengths keep the prepare stage honest with the new Ready/Mempool paths. Nicely done.
rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (3)
32-77: Nice routing logic here—clear as day.The disposition-based routing in
filter_operations_for_preparationhandles all four cases cleanly. Sequential async processing could be a bottleneck with large batches, but for typical use cases, this should be just fine.
98-114: Solid decision point for routing operations.The manual retry handling followed by delegation to
operation_disposition_by_payload_statusis clean. Documentation spells out exactly what happens for each disposition type.
135-144: Error handling keeps things moving even when the DB acts up.Swallowing the error with a warning is the right call here, as documented. Transient DB issues shouldn't block message reprocessing.
...layer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs
Outdated
Show resolved
Hide resolved
...layer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs
Outdated
Show resolved
Hide resolved
...layer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs
Outdated
Show resolved
Hide resolved
...layer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs
Outdated
Show resolved
Hide resolved
rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs
Outdated
Show resolved
Hide resolved
ameten
force-pushed
the
ameten/lander-tx-submission-observability
branch
from
2580ca0 to
0f7bcb5
Compare
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/agents/relayer/src/msg/message_processor/disposition/tests.rs (1)
232-266: Update assertion messages to sayPostSubmitSuccessinstead ofConfirm.These three tests now assert
OperationDisposition::PostSubmitSuccess, but the failure messages still say "Should return Confirm…", which can confuse future swamp‑dwellers reading test output.Consider updating the strings to match the enum name (or say “PostSubmitSuccess / confirm stage”) to keep semantics clear.
Also applies to: 305-347, 461-495
♻️ Duplicate comments (1)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs (1)
134-165: DuplicatePayloadNotFound→PreSubmitcoverage.
test_determine_disposition_returns_presubmit_when_payload_not_foundandtest_determine_disposition_returns_presubmit_for_entrypoint_error_payload_not_foundboth set up a payload UUID, have the entrypoint returnLanderError::PayloadNotFound, and assertPreSubmit.Unless there’s a subtle distinction you want to keep, you can probably drop or reshape one of these (e.g., cover multiple UUIDs or a different error) to avoid testing the same swamp twice.
Also applies to: 530-562
🧹 Nitpick comments (5)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs (2)
196-243: Align test name/comment withPostSubmitSuccessvariant.This test is now asserting
OperationDisposition::PostSubmitSuccess, but both the function name (returns_postsubmit_for_various_transaction_states) and the inner comment still speak in terms of a generic “PostSubmit”.Given we now have both
PostSubmitSuccessandPostSubmitFailure, it’d be clearer to rename the test (and tweak the comment) to mentionPostSubmitSuccessexplicitly.
135-143: Broaden the log message inremove_linkage_between_payload_and_message.The helper is documented for manual retries and
PostSubmitFailure/reorg cases, but the warning string says “for manual operation” only. That’s a bit misleading once this runs for non‑manualPostSubmitFailuredispositions too.Consider rewording the log to something like “Failed to remove payload UUID mapping for operation” so it matches all call sites.
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
9-30: Top‑level enum docs still reference aPostSubmitvariant that no longer exists.The stage‑specific comments talk about a generic
PostSubmithandling in Prepare/Submit, but the enum has been split intoPostSubmitSuccessandPostSubmitFailure, each with different behavior.Might be worth rewriting that header comment to:
- Refer explicitly to
PostSubmitSuccessandPostSubmitFailure, and- Briefly explain how each maps to stages, instead of the old single “PostSubmit” story.
That’ll keep future ogres from scratching their heads when they read the docs.
Also applies to: 65-78
rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
135-143: Make the linkage‑removal warning message less “manual”-specific.Same note as in the tests: this helper is used for manual retries and generic
PostSubmitFailurecases, but the log text still says “for manual operation”.Tweaking the message to something like “Failed to remove payload UUID mapping for operation” would better reflect all the paths that land here.
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs (1)
22-1064: Consider extracting common mock setup patterns into helper functions.There's quite a bit of repeated boilerplate across tests - creating queues, setting up mock DB expectations, configuring mock entrypoints. While the current approach is clear and explicit, helper functions could reduce duplication and make tests easier to maintain. For example:
fn setup_mock_db_with_payload( mock_db: &mut MockHyperlaneDb, message_id: H256, payload_uuid: UniqueIdentifier, ) { let payload_uuid_clone = payload_uuid.clone(); mock_db .expect_retrieve_payload_uuids_by_message_id() .returning(move |id| { if *id == message_id { Ok(Some(vec![payload_uuid_clone.clone()])) } else { Ok(None) } }); }This would make each test more focused on what makes it unique. But honestly, the current explicit approach ain't terrible either - sometimes seeing all the setup right there in the test makes it easier to understand what's going on.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs(2 hunks)rust/main/agents/relayer/src/msg/message_processor/disposition/tests.rs(9 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs(3 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs(8 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs(32 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs(1 hunks)rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T12:20:42.663Z
Learnt from: yjamin
Repo: hyperlane-xyz/hyperlane-monorepo PR: 7414
File: rust/main/chains/hyperlane-aleo/src/provider/traits.rs:252-0
Timestamp: 2025-11-20T12:20:42.663Z
Learning: In rust/main/chains/hyperlane-aleo/src/provider/traits.rs, the `get_state_paths_for_commitments` and `get_state_paths_for_commitments_async` methods intentionally use `.unwrap_or_default()` to return an empty Vec on any error. This behavior matches SnarkVM's internal QueryTrait implementation and should not be changed to propagate errors.
Applied to files:
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs
🧬 Code graph analysis (5)
rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs (2)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (5)
create_test_metrics(185-214)create_test_queue(164-182)new(33-39)id(59-61)with_first_prepare(41-44)rust/main/agents/relayer/src/msg/message_processor/stage/submit.rs (1)
filter_operations_for_submit(38-84)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (2)
id(59-61)status(96-98)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
operation_disposition_by_payload_status(96-144)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs (2)
rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (4)
create_test_queue(164-182)new(33-39)id(59-61)with_first_prepare(41-44)rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
filter_operations_for_preparation(32-77)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs (3)
rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
determine_operation_disposition(98-114)rust/main/agents/relayer/src/msg/message_processor/tests/tests_common.rs (2)
new(33-39)with_first_prepare(41-44)rust/main/hyperlane-core/src/chain.rs (1)
new_test_domain(333-341)
⏰ 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). (10)
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
- GitHub Check: test-rs
🔇 Additional comments (10)
rust/main/agents/relayer/src/msg/message_processor/disposition/tests.rs (1)
112-495: Disposition mapping tests look aligned with the new 3‑stage pipeline.The scenarios for Dropped/Retry, PendingInclusion, ReadyToSubmit, Mempool, Included, and Finalized all line up with
PostSubmitFailure,Submit, andPostSubmitSuccessas implemented inoperation_disposition_by_payload_status, and the “early return” cases (DB error / no UUIDs / entrypoint error) correctly stick toPreSubmit. Nothing smelly here.rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_determine_operation_disposition.rs (1)
22-193: Manual vs non‑manual retry coverage looks solid.The tests around manual retries (clearing linkage then falling back to
PreSubmit) and non‑manual retries (e.g.ErrorSubmittingmapping toPostSubmitSuccess) match thedetermine_operation_disposition+operation_disposition_by_payload_statusflow and exercise both DB‑failure and happy paths nicely. No dragons hiding in this part.rust/main/agents/relayer/src/msg/message_processor/stage/submit/tests.rs (1)
19-938: Submit‑stage routing tests thoroughly cover the new dispositions.The suite hits all the interesting branches—
PreSubmitresubmission,Submitre‑queueing (ReadyToSubmit/PendingInclusion/Mempool), andPostSubmit{Success,Failure}flowing into the confirm queue—plus DB/entrypoint errors and multi‑payload/variant cases. Behavior matchesfilter_operations_for_submitand the disposition mapping cleanly; nothing obviously off here.rust/main/agents/relayer/src/msg/message_processor/disposition.rs (1)
81-143: Status→disposition mapping and logging look correct and conservative.The function safely falls back to
PreSubmiton DB/entrypoint errors or missing UUIDs, and the mappings for allPayloadStatus/TransactionStatusvariants line up with how the prepare and submit stages consumePreSubmit/Submit/PostSubmitSuccess/PostSubmitFailure. The addedwarn!logs include message ID and payload UUID, which should make debugging nice and muddy in the right ways.rust/main/agents/relayer/src/msg/message_processor/stage/prepare.rs (1)
32-77: Prepare‑stage routing matches the new disposition semantics.
filter_operations_for_preparationanddetermine_operation_dispositionhook together cleanly: manual retries clear linkage then drop intoPreSubmit,Submitops are funneled to the submit queue withReadyToSubmit, finalized/included ops head to confirm withConfirm(AlreadySubmitted), andPostSubmitFailureclears linkage before handing the op back for re‑prep. That lines up with the intended three‑stage flow.Also applies to: 98-114
rust/main/agents/relayer/src/msg/message_processor/stage/prepare/tests/tests_filter_operations_for_preparation.rs (5)
22-98: Tests correctly updated for new submit queue parameter.The early tests (empty batch and manual retry) have been properly updated to include the submit_queue parameter and verify both queue states. The logic is sound - manual retry operations go to pre-submit, leaving both queues empty.
358-461: Well-structured mixed batch test validates new routing logic.This test nicely demonstrates the three-way split: manual retry + not submitted → pre-submit, Finalized → confirm queue, PendingInclusion → submit queue. The assertions clearly verify each routing path.
509-615: PostSubmitFailure disposition correctly tested for dropped payloads and transactions.Both tests properly validate that when payloads or transactions are dropped, the linkage is removed (empty UUID list stored) and operations return to pre-submit. The
withfmatchers on lines 529 and 581 ensure the removal logic is called correctly.
758-903: Solid coverage for new status routing paths.These three focused tests validate that ReadyToSubmit and Mempool route to submit_queue, while Included routes to confirm_queue. Each test is simple and clearly demonstrates one routing path. Nice work on the test isolation.
905-1064: Comprehensive batch tests for Submit disposition variants.These final tests validate that multiple operations with Submit disposition (PendingInclusion, ReadyToSubmit, Mempool) correctly route to the submit queue, both in homogeneous and heterogeneous batches. The coverage is thorough.
…or messages which payload and corresponding transactions are dropped
ameten
force-pushed
the
ameten/lander-tx-submission-observability
branch
from
0f7bcb5 to
1b5361d
Compare
Description
Lander Transaction Submission Observability:
Related issues
Backward compatibility
Yes
Testing
Unit tests
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.