refactor(committor): simplify tasks and remove unused interfaces by snawaz · Pull Request #1849 · magicblock-labs/magicblock-validator

snawaz · 2026-10-26T21:27:34Z

What changed

Replace the task and builder traits with concrete Task and TaskBuilder types. Give buffer and recovery helpers clearer names, remove unused interfaces, and stop revisiting tasks already converted to buffer delivery.

Consolidate the task and scheduler test modules, sharing imports and setup while preserving all tests.

Closes #1850.

Impact

This changes Rust API names in the committor crate. Transaction formats, execution behavior, and resource limits stay the same.

coderabbitai · 2026-10-26T21:32:41Z

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

Source mode: Auto
Repository: magicblock-labs/magicblock-validator
Branch: refactor/committor-simplify-tasks
File: .coderabbit.yaml
Commit: 066c7b5
Layers: Repository YAML
Review profile: ASSERTIVE
Plan: Advanced
Run ID: e730c872-b53b-4161-9528-f541812af57f

Commits

Reviewing files that changed from the base of the PR and between a4fe064 and 066c7b5.

Files selected for processing: 30

committor/src/intent_engine/db.rs
committor/src/intent_engine/intent_execution_engine.rs
committor/src/intent_engine/intent_scheduler.rs
committor/src/intent_executor/error.rs
committor/src/intent_executor/executor.rs
committor/src/intent_executor/executor/tests.rs
committor/src/intent_executor/intent_execution_client.rs
committor/src/intent_executor/mod.rs
committor/src/intent_executor/strategy_executor/error.rs
committor/src/intent_executor/strategy_executor/recovery.rs
committor/src/intent_executor/strategy_executor/transaction.rs
committor/src/intent_executor/strategy_executor/transaction/tests.rs
committor/src/intent_executor/strategy_executor/utils.rs
committor/src/intent_executor/test_utils.rs
committor/src/intent_executor/utils.rs
committor/src/lib.rs
committor/src/outbox/mod.rs
committor/src/outbox/outbox_client.rs
committor/src/service.rs
committor/src/tasks/buffer_task.rs
committor/src/tasks/commit_finalize_task.rs
committor/src/tasks/intent_size_validator.rs
committor/src/tasks/mod.rs
committor/src/tasks/task_builder.rs
committor/src/tasks/task_info_fetcher.rs
committor/src/tasks/task_strategist.rs
committor/src/tasks/utils.rs
committor/src/transaction_preparator/cleanup_resources.rs
committor/src/transaction_preparator/delivery_preparator.rs
committor/src/transaction_preparator/mod.rs

No actionable comments were generated in the recent review.

Recent review info

Run configuration

Source mode: Auto
Repository: magicblock-labs/magicblock-validator
Branch: refactor/committor-simplify-tasks
File: .coderabbit.yaml
Commit: a4fe064
Layers: Repository YAML
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 7edad7a4-fb57-4c4a-8684-0c62d6bdcf6e

Commits

Reviewing files that changed from the base of the PR and between 24ab69d and a4fe064.

Files selected for processing: 2

committor/src/intent_engine/intent_scheduler.rs
committor/src/tasks/mod.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

This change replaces the base-task APIs with Task across intent task building, transaction planning, preparation, execution, and recovery. It also updates execution errors, nonce invalidation, backlog storage, buffer cleanup, and related intent and service interfaces.

Changes: Intent Execution and Task Refactor

Layer / File(s)	Summary
Task model and construction `committor/src/tasks/*`, `committor/src/intent_executor/utils.rs`	`Task` and `TaskBuilder` replace the base-task trait and builder APIs. Task creation, validation, and transaction utility functions use `Task`. Buffer preparation and cleanup types are renamed, and cleanup limits are constants.
Strategy planning and transaction preparation `committor/src/tasks/task_strategist.rs`, `committor/src/transaction_preparator/*`, `committor/src/intent_executor/test_utils.rs`	`TransactionStrategy` stores tasks in `tasks`. Planning and preparation use `Task`, `BufferPreparation`, and `BufferCleanup`. Cleanup batches use the declared transaction-count and compute-unit constants.
Transaction execution and recovery `committor/src/intent_executor/strategy_executor/*`, `committor/src/tasks/task_info_fetcher.rs`, `committor/src/intent_executor/executor.rs`	Execution and recovery use `Task` and `TransactionExecutionError`. CPI-limit handling splits work with `split_for_follow_up`. Nonce invalidation removes specified keys from the active and retiring caches.
Intent interfaces and executor integration `committor/src/intent_engine/`, `committor/src/intent_executor/`, `committor/src/outbox/*`, `committor/src/service.rs`, `committor/src/lib.rs`	The backlog storage API accepts batches. Intent execution and reporting use `TransactionExecutionError`; `close_intent` and selected crate re-exports are removed. The service uses `ProdChainlink` directly.

Priority: Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Refactor

Possibly related issues

magicblock-labs/magicblock-validator#1840 — The change updates TransactionStrategy and its task interface, but the issue’s execution-mode replacement remains outside this change.
magicblock-labs/magicblock-validator#1842 — The change adds BufferCleanup records and cleanup limits, but does not include the full cleanup redesign.

Possibly related PRs

magicblock-labs/magicblock-validator#1843 — This PR continues changes to the intent executor, recovery, client, task, and error interfaces.
magicblock-labs/magicblock-validator#1769 — This PR continues the transaction-execution refactor and updates task and error interfaces.
magicblock-labs/magicblock-validator#1847 — This PR changes the same transaction and recovery modules and uses the task-based execution flow.

Suggested reviewers: bmuddha

Merge Risk: Minimal · previous review a4fe0

No actionable issue is established for these changes. They are mergeable after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 26.28% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 156 functions across 27 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title "refactor(committor): simplify tasks and remove unused interfaces" matches the main change. The PR replaces the task and builder traits with concrete `Task` and `TaskBuilder` types, renames …
Description check		The description states that the PR replaces the task and builder traits with concrete `Task` and `TaskBuilder` types, renames buffer and recovery helpers, removes unused interfaces, and stops revisiti…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "refactor(committor): simplify tasks and remove unused interfaces" matches the main change. The PR replaces the task and builder traits with concrete Task and TaskBuilder types, renames helpers, and removes unused interfaces such as close_intent, ResetType, and TasksBuilder. The scope prefix names the affected crate. The title is concise and specific.

Full details: Description check

Explanation

The description states that the PR replaces the task and builder traits with concrete Task and TaskBuilder types, renames buffer and recovery helpers, removes unused interfaces, and stops revisiting tasks already converted to buffer delivery. It also notes the scheduler and task test consolidation and the Rust API name changes. This relates directly to the changeset.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

The task enum already selects the concrete instruction implementation, and TaskBuilder has no alternate implementation. Remove their traits and put the methods directly on Task, CommitFinalizeTask, and TaskBuilder. This also removes async-trait boxing from task construction. Use names that describe the current responsibilities: CommitInputs, BufferPreparation, BufferCleanup, TransactionExecutionError, try_use_buffer, split_for_follow_up, and finalize_legacy_commit. Rename TransactionStrategy.optimized_tasks to tasks because a strategy can keep inline tasks unchanged. Remove unused transaction assembly, outbox close, and single-item backlog interfaces, the committor Chainlink alias, and unused changeset re-exports. Keep the program's outbox close instruction and the existing completion notification path. Replace the nonce reset enum with invalidate_nonces, which still removes selected keys from both active and retiring caches. Stop measuring and reinserting a task after switching it to buffer delivery: that conversion can happen only once. Continue recalculating the transaction size after every conversion and processing the largest remaining instructions first. Express the existing buffer cleanup limits as constants and remove redundant return bindings and the one-use error classification wrapper. Update existing tests only for the changed APIs. Keep instruction formats, resource limits, recovery behavior, callback checks, and metric labels. Validation: - cargo fmt - cargo check -p magicblock-committor-service --tests - cargo clippy --workspace --all-targets -- -D warnings - cargo test -p magicblock-committor-service test_optimize_strategy_prioritizes_largest_tasks -- --nocapture

Merge the callback tests, undelegation test, and standalone buffer-limit test into one tasks test module with shared imports. Merge the scheduler's seven test modules into one and replace six identical setup helpers with a single helper. Keep the shared intent bundle factory at its existing scope for callers in other modules. Preserve all 25 test functions, assertions, and setup calls. This removes 76 lines without changing production code. Validation: - cargo fmt - cargo check -p magicblock-committor-service --tests - cargo test -p magicblock-committor-service test_callback_destination_bound_to_recorded_source -- --nocapture

Move create_test_intent_bundle above the consolidated tests module to satisfy Clippy's items_after_test_module lint. Keep the helper at the same module scope so its callers retain their existing imports. The helper body and test behavior are unchanged. Validation: - cargo fmt - cargo check -p magicblock-committor-service --tests - cargo clippy -p magicblock-committor-service --all-targets -- -D warnings - cargo test -p magicblock-committor-service test_completion_unblocks_intents -- --nocapture

snawaz added this pull request to stack #1844 October 26, 2026 21:27

snawaz self-assigned this Oct 26, 2026

snawaz marked this pull request as ready for review October 26, 2026 21:28

This was referenced Oct 26, 2026

Simplify committor tasks and remove unused interfaces #1850

Open

Simplify committor ownership around intent acceptance #1851

Open

coderabbitai Bot mentioned this pull request Oct 27, 2026

refactor(committor): separate intake and execution ownership #1852

Open

snawaz added 3 commits October 28, 2026 15:47

snawaz force-pushed the refactor/committor-simplify-tasks branch from a40634c to 066c7b5 Compare October 28, 2026 10:17

This was referenced Oct 28, 2026

refactor(committor): unify transaction execution #1769

Open

feat(committor): add v2 delegation reads and settlement builders #1860

Open

Navigation Menu

Repository navigation

refactor(committor): simplify tasks and remove unused interfaces - #1849

refactor(committor): simplify tasks and remove unused interfaces#1849
snawaz wants to merge 3 commits into
refactor/committor-cleanup-resourcesfrom
refactor/committor-simplify-tasks

snawaz commented Oct 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 26, 2026 •

edited

Loading

Review limit reached

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

snawaz commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Impact

Uh oh!

coderabbitai Bot commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

snawaz commented Oct 26, 2026 •

edited

Loading

coderabbitai Bot commented Oct 26, 2026 •

edited

Loading