Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Unify transaction recovery rules committor/src/intent_executor/error.rs, committor/src/intent_executor/intent_execution_client.rs, committor/src/intent_executor/strategy_executor/{mod.rs,patcher.rs,recovery.rs}, committor/src/intent_executor/test_utils.rs |
The stage-specific patchers are removed and TransactionRecovery is added to handle selected action, commit-ID, unfinalized-account, and undelegation errors. Error tests cover split-recovery classification and transport-transience cases. |
Route execution through the shared loop committor/src/intent_executor/strategy_executor/{transaction.rs,transaction/tests.rs,utils.rs}, committor/src/intent_executor/intent_execution_client.rs, committor/src/intent_executor/test_utils.rs |
TransactionExecutor reconciles pending transactions, records prepared transactions before sending, and routes send failures through TransactionRecovery. The previous prepared-message retry path and stage-specific pending helpers are removed. Tests cover recovery progress, callbacks, and resource accounting. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Refactor
Sequence Diagram(s)
sequenceDiagram
participant TransactionExecutor
participant Outbox
participant IntentExecutionClient
participant TransactionRecovery
TransactionExecutor->>Outbox: Record pending transaction
TransactionExecutor->>IntentExecutionClient: Send signed transaction
IntentExecutionClient-->>TransactionExecutor: Return send result
TransactionExecutor->>TransactionRecovery: Recover send failure
TransactionRecovery-->>TransactionExecutor: Return recovery action or stop
Possibly related issues
#1840: The changes gate recovery actions using execution progress and current tasks, which overlaps with the issue’s retry-decision objectives.
Possibly related PRs
- magicblock-labs/magicblock-validator#1769: Introduced
TransactionExecutorand the shared transaction loop that this PR changes. - magicblock-labs/magicblock-validator#1848: Changes cleanup ownership in the recovery path introduced here.
- magicblock-labs/magicblock-validator#1843: Routes accepted and resumed intents through the
TransactionExecutorchanged in this PR.
Suggested reviewers: gabrielepicco, bmuddha
Merge Risk: 
974b3
-
This refactor merges the committor's transaction recovery and sending into one path. No concrete defect was found in the supplied changes, so merge risk appears minimal.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 13.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title "refactor(committor): unify transaction recovery and sending" matches the main change. The PR replaces the three patchers and repeated runner dispatch with one recovery path (new recovery.rs… Description check The description states what changed: the three patchers and repeated runner dispatch are replaced by one recovery path, and the unused send path that rebuilt transactions is removed. It also covers im… Linked Issues check Issue #1841requires one recovery path and one outbox-aware transaction loop. The PR deletes the three patchers and the old stage loop, addsTransactionRecovery, and moves preparation, outbox record…Out of Scope Changes check The changes stay within issue #1841. The removed error helper and send path support removal of the obsolete retry flow. The new recovery module, transaction loop, test utility, and recovery tests dire…Full details: Title check
-
Explanation
The title "refactor(committor): unify transaction recovery and sending" matches the main change. The PR replaces the three patchers and repeated runner dispatch with one recovery path (new recovery.rs with TransactionRecovery), and removes the unused send path that rebuilt transactions (execute_message_with_retries, send_prepared_message). The title is specific and uses a conventional-commit prefix.
Full details: Description check
-
Explanation
The description states what changed: the three patchers and repeated runner dispatch are replaced by one recovery path, and the unused send path that rebuilt transactions is removed. It also covers impact: commit repairs apply only before confirmation, follow-ups recover actions and undelegations, and pending-signature resolution, timeout limits, and the legacy DLP bridge remain. It references issue
#1841. This matches the changeset described in the raw summary.
Full details: Linked Issues check
-
Explanation
Issue
#1841requires one recovery path and one outbox-aware transaction loop. The PR deletes the three patchers and the old stage loop, addsTransactionRecovery, and moves preparation, outbox recording, sending, pending-signature resolution, and recovery into oneTransactionExecutorloop. The recovery code uses task presence and confirmed-commit progress to limit commit repair, undelegation recovery, and action-only retries. The transaction loop preserves nonce repair, bounded retries, callback timeout handling, and pending-transaction reconciliation. The tests cover action and undelegation failures for combined and follow-up execution, callback results and destinations, confirmed-commit progress, remaining tasks, and resource cleanup.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#1841. The removed error helper and send path support removal of the obsolete retry flow. The new recovery module, transaction loop, test utility, and recovery tests directly implement or verify the requested recovery and sending behavior. The legacy DLP finalization bridge remains in the recovery path as required.
-
-
Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
974b3ea to
e7b3578
Compare
Replace the three stage-specific patchers and their async trait with one recovery implementation. Use current tasks and confirmed-commit progress to permit nonce repair, legacy DLP finalization, action removal, and undelegation recovery only where they apply. Follow-ups cannot repair confirmed commits, and standalone action failures cannot start replacement transactions through recovery. Route the runner through one outbox-aware execution loop. Preserve pending signature resolution, callback timeout handling, attempt limits, and the combined-to-split fallback. Match stored outbox progress using the common pending-execution representation. Remove the unused prepare-and-execute/message-send chain and its redundant error mapper. Settlement retries use the existing signed-transaction path, which resends identical bytes rather than creating a fresh signature. Keep the legacy DLP finalization bridge pending #1801. Extend the existing resource-recovery test with combined, follow-up, and standalone action failures and undelegation failures. Check that remaining work, confirmed signatures, and ALT cleanup obligations survive recovery. Validation: make fmt; cargo check -p magicblock-committor-service --tests; cargo test -p magicblock-committor-service recovery_preserves_progress_and_resources -- --nocapture; git diff --check. Closes #1841.
Report failure callbacks before discarding an undelegation and its following actions. Extract callbacks through the existing authorization checks, retain the failure signature, and leave callbacks on retained actions for the eventual retry result. Move the sole transaction loop onto TransactionExecutor. Remove the borrowed ExecutionState adapter and closure parameters while preserving pending-signature resolution, outbox-before-send ordering, cancellation state, cleanup ownership, and retry limits. Remove the unused InternalError transaction-size wrapper and replace four small predicate tests with one table-driven test of the surviving send error mapper. Extend the existing recovery matrix to check discarded and retained callback identities, outcomes, and failure signatures. Validation: cargo fmt; cargo check -p magicblock-committor-service --tests; cargo test -p magicblock-committor-service recovery_preserves_progress_and_resources -- --nocapture; git diff --check.
e7b3578 to
61603f0
Compare
What changed
Replace the three patchers and repeated runner dispatch with one recovery path based on current tasks and execution progress. Remove the unused send path that rebuilt transactions.
Closes #1841.
Impact
Commit repairs apply only before confirmation. Follow-ups recover actions and undelegations; standalone actions do not gain replacement retries. Pending-signature resolution, timeout limits, and the legacy DLP bridge remain in place.