Trigger SentCommit on failure #643
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis change refactors intent result processing to use a new result type (BroadcastedIntentExecutionResult) and adds error message propagation throughout the processing pipeline. It introduces signature extraction methods across multiple error types (IntentExecutorError, TaskInfoFetcherError, TaskBuilderError, DeliveryPreparatorError, and TransactionPreparatorError). The SentCommit structure is enhanced with an optional error_message field, and DeliveryPreparatorError is promoted to a public enum. Error handling is unified to extract chain signatures from both execution outputs and error results, replacing previous separate error pathways. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (7)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-21T10:22:07.520ZApplied to files:
📚 Learning: 2025-10-21T14:00:54.642ZApplied to files:
🧬 Code graph analysis (6)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (6)
magicblock-committor-service/src/tasks/task_builder.rs (3)
magicblock-committor-service/src/transaction_preparator/error.rs (4)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (5)
magicblock-committor-service/src/intent_executor/error.rs (5)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
🔇 Additional comments (7)
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: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
magicblock-accounts/src/scheduled_commits_processor.rs(4 hunks)magicblock-committor-service/src/intent_executor/error.rs(4 hunks)magicblock-committor-service/src/intent_executor/mod.rs(3 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(2 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(2 hunks)magicblock-committor-service/src/tasks/task_builder.rs(2 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(2 hunks)magicblock-committor-service/src/transaction_preparator/error.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs(5 hunks)test-integration/programs/flexi-counter/src/processor.rs(2 hunks)test-integration/programs/flexi-counter/src/state.rs(1 hunks)test-integration/test-committor-service/tests/test_intent_executor.rs(17 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(2 hunks)test-integration/test-committor-service/tests/utils/instructions.rs(1 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/programs/flexi-counter/src/state.rstest-integration/test-committor-service/tests/utils/instructions.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/programs/flexi-counter/src/processor.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-11-18T08:47:39.681Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.681Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rs
🧬 Code graph analysis (10)
magicblock-committor-service/src/tasks/task_builder.rs (3)
magicblock-committor-service/src/intent_executor/error.rs (1)
signature(29-34)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
signature(279-285)magicblock-rpc-client/src/lib.rs (1)
signature(82-90)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
magicblock-committor-service/src/intent_executor/error.rs (1)
signature(29-34)magicblock-rpc-client/src/lib.rs (1)
signature(82-90)
magicblock-committor-service/src/transaction_preparator/error.rs (1)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
signature(529-534)signature(546-551)from(503-507)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
test-integration/test-committor-service/tests/utils/instructions.rs (1)
init_account_and_delegate_ixs(20-51)
magicblock-committor-service/src/intent_executor/error.rs (8)
magicblock-rpc-client/src/lib.rs (2)
error(193-195)signature(82-90)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
signature(279-285)magicblock-committor-service/src/tasks/task_builder.rs (1)
signature(211-216)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
signature(529-534)signature(546-551)magicblock-committor-service/src/transaction_preparator/error.rs (1)
signature(20-25)magicblock-committor-service/src/tasks/buffer_task.rs (1)
task_type(122-126)magicblock-committor-service/src/tasks/args_task.rs (1)
task_type(146-153)magicblock-committor-service/src/tasks/mod.rs (1)
task_type(89-89)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (5)
magicblock-committor-service/src/intent_executor/error.rs (1)
signature(29-34)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
signature(279-285)magicblock-committor-service/src/tasks/task_builder.rs (1)
signature(211-216)magicblock-committor-service/src/transaction_preparator/error.rs (1)
signature(20-25)magicblock-rpc-client/src/lib.rs (2)
signature(82-90)error(193-195)
test-integration/test-committor-service/tests/test_intent_executor.rs (3)
test-integration/programs/flexi-counter/src/state.rs (2)
pda(34-37)new(16-22)test-integration/test-tools/src/conversions.rs (1)
err(8-8)test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
test-integration/programs/flexi-counter/src/processor.rs (2)
programs/magicblock/src/magic_context.rs (1)
deserialize(18-26)programs/magicblock/src/task_context.rs (1)
deserialize(82-90)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
magicblock-committor-service/src/intent_executor/error.rs (1)
signature(29-34)
⏰ 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). (2)
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (18)
test-integration/programs/flexi-counter/src/state.rs (1)
12-13: Undelegation failure constants look appropriate for shared test usageThe new label/code pair is simple, explicit, and being
pubmakes it easy to coordinate failure injection across the flexi-counter processor and integration tests; this is fine for a test-only program. Based on learningsmagicblock-committor-service/src/tasks/task_builder.rs (1)
10-10: TaskBuilderError::signature() is consistent with the broader error-signature patternDelegating to the inner
TaskInfoFetcherError::signature()for both commit and finalize variants cleanly exposes an optional transaction signature without changing existing control flow.Also applies to: 210-217
magicblock-committor-service/src/intent_executor/error.rs (1)
43-47: UndelegationError wiring across executor and mapping logic looks coherentAdding
UndelegationErrorto bothIntentExecutorErrorandTransactionStrategyExecutionError, mapping it throughfrom_strategy_execution_error, and classifying per-taskInstructionErrorinto the new variant whenTaskType::Undelegategives you a clear, dedicated error path for undelegation failures without changing existing behavior for other task types.Also applies to: 99-103, 153-161, 192-231
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
13-13: TaskInfoFetcherError::signature() cleanly surfaces RPC signaturesThe helper correctly returns
Nonefor metadata/decoding issues and delegates toMagicBlockRpcClientError::signature()for RPC failures, which keeps signature propagation uniform for callers likeTaskBuilderError.Also applies to: 278-286
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (1)
178-184: UndelegationError patch handling is consistent with existing retry patternsThe new
UndelegationErrorarm mirrors the existing Actions/CommitID handling by patching the strategy viahandle_undelegation_errorand returningContinue(to_cleanup), which fits the current single‑stage retry flow without altering CPI-limit or internal-error semantics.magicblock-committor-service/src/transaction_preparator/error.rs (1)
1-26: Delivery preparation error wiring andsignature()helper look correctUsing
DeliveryPreparatorErroras the inner type with#[from]and delegatingsignature()to it is consistent with the rest of the error stack and enables signature propagation without changing callers. No issues spotted.test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
113-118: Calls updated correctly for optional labelBoth call sites now pass
Nonetoinit_and_delegate_account_on_chain, matching the new(counter_auth, bytes, label)signature while preserving the previous default label behavior. Concurrency pattern increate_bundlesremains unchanged.Also applies to: 395-403
test-integration/test-committor-service/tests/utils/instructions.rs (1)
20-32: Optional label handling for init instruction is sound
init_account_and_delegate_ixsnow threads anOption<String>label intocreate_init_ix, defaulting to"COUNTER"whenNone. This cleanly enables labeled test setups without changing existing behavior. Implementation is straightforward and correct.test-integration/test-committor-service/tests/utils/transactions.rs (1)
126-147: Label propagation into init/delegate helper looks correctExtending
init_and_delegate_account_on_chainwithlabel: Option<String>and forwarding it toinit_account_and_delegate_ixsis consistent with the utilities ininstructions.rs. The rest of the transaction flow remains untouched, so existing behavior is preserved when passingNone.programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs (1)
19-31:error_messageaddition toSentCommitis well-integratedAdding
error_message: Option<String>to SentCommit/SentCommitPrintable, threading it through theFromimpl, and conditionally logging it inprocess_scheduled_commit_sentis consistent and non-breaking. Tests correctly initialize it toNone, so existing scenarios remain unchanged while failures can now surface additional context.Also applies to: 35-45, 47-75, 216-222, 245-261
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
167-196:UndelegationErrorhandling in two‑stage executor is consistent with flow semantics
- Commit phase: treating
UndelegationErroras unexpected and returningBreak(())after logging is appropriate, since undelegation should not happen there.- Finalize phase: delegating to
handle_undelegation_errorand returningContinue(to_cleanup)mirrors the existingActionsErrorretry‑with‑cleanup path and enables targeted recovery.ControlFlow behavior and logging look correct; no further issues spotted.
Also applies to: 210-238
magicblock-committor-service/src/intent_executor/mod.rs (2)
550-553: Undelegation errors treated as non-persisted status looks correctGrouping
UndelegationErrorwithCommitIDError/ActionsError/CpiLimitErrorso thatpersist_resultreturnsNoneavoids clobbering more precise intermediate statuses and matches the “recoverable / retried-internally” semantics those other variants have. This aligns well with the new undelegation‑recovery tests.
791-795: ResettingTaskInfoFetcheron any undelegate intent is a good conservative choiceThe added comment and condition
if result.is_err() || is_undelegatemake it explicit that caches are reset even when undelegation tasks are dropped and the overall execution succeeds. Given undelegated accounts shouldn’t be committed again, this conservative reset is reasonable and keeps cache invariants simple.test-integration/test-committor-service/tests/test_intent_executor.rs (2)
38-40: New undelegation error-parsing test and imports look solidImporting
FAIL_UNDELEGATION_LABELand using it intest_undelegation_error_parsingto force an undelegation failure exercises the newTransactionStrategyExecutionError::UndelegationErrorpath end‑to‑end. The assertions on the variant and the error message string provide good coverage of both classification and formatting, in line with the existing CommitID/Actions/CPI tests.Also applies to: 161-208
121-121:setup_counterrefactor and callsite updates are cleanRefactoring
setup_counterto take an optionallabel: Option<String>and wiring all existing callsites withNone(plus the failure scenarios withSome(FAIL_UNDELEGATION_LABEL.to_string())) keeps the tests DRY while enabling targeted undelegation failures. The helper’s use ofinit_and_delegate_account_on_chainand explicit owner override toprogram_flexi_counter::id()matches the previous patterns and looks correct.Also applies to: 222-222, 283-283, 340-341, 436-437, 488-488, 547-547, 635-635, 831-842
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (1)
78-83: Structured error mapping inprepare_for_deliveryis clear and composableWrapping task-preparation failures into
FailedToPrepareBufferAccountsand ALT preparation failures intoFailedToCreateALTErrorcleanly separates the two concerns and gives upstream code a precise failure surface, while still allowingInternalErrorto carry the concrete root cause. This is a good alignment with the rest of the new error hierarchy.magicblock-accounts/src/scheduled_commits_processor.rs (2)
19-20: Unified result handling now reliably emitsSentCommiton failuresSwitching the result loop to operate on
BroadcastedIntentExecutionResultand funnelling bothOkandErrcases throughprocess_intent_resultguarantees that on-chain triggered intents always produce aSentCommit, even when execution fails. Derivingchain_signaturesfrom eitherExecutionOutputorerr.signatures()and logging the error before building the commit keeps the behavior consistent with prior logging while satisfying the “trigger SentCommit on failure” requirement.Also applies to: 175-249, 252-305
307-323:SentCommitextension witherror_messageis consistent and non-breaking at call sitesUpdating
build_sent_committo accept and seterror_message: Option<String>cleanly threads failure context into theSentCommitpayload while leaving the rest of the structure unchanged. All call sites in this module now pass the appropriate value (alwaysSomeon error,Noneon success), which should make downstream consumers simpler.
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
Show resolved
Hide resolved
taco-paco
changed the base branch from
master
to
feat/base-layer-ix/strip-failed-undelegation
taco-paco
force-pushed
the
feat/base-layer-ix/trigger-sent-commit-on-failure
branch
from
cb44d0c to
d02cc05
Compare
feat: trigger SentCommit on intent failure and failed retry
taco-paco
force-pushed
the
feat/base-layer-ix/trigger-sent-commit-on-failure
branch
from
d02cc05 to
e4f241d
Compare
taco-paco
force-pushed
the
feat/base-layer-ix/strip-failed-undelegation
branch
from
ff9d2a5 to
150554e
Compare
…/base-layer-ix/trigger-sent-commit-on-failure # Conflicts: # test-integration/test-committor-service/tests/test_ix_commit_local.rs
Summary by CodeRabbit
Release Notes
Bug Fixes
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.