refactor: Simplify Task types

[snawaz]

This PR redesigns the task types without re-implementing any heavy or critical logic and without adding/modifying/removing any feature. It's pure refactoring and redesign, to achieve some level of simplicity and better readability.

• Since we work with a closed set of tasks, we don’t need dynamic dispatch (trait BaseTask, dyn BaseTask, Box, etc.). Instead, we now have a single enum representing all task variants (a closed set):

  pub enum Task {
    Commit(CommitTask),
    Finalize(FinalizeTask),
    Undelegate(UndelegateTask),
    BaseAction(BaseActionTask),
  }

• CommitTask now owns both "what to commit" (committed_account) and "how to commit"(strategy). Both are intimate details of the task and therefore should be owned by the task itself. Also, the strategy owns the BufferLifecycle when applicable.

  pub enum CommitStrategy {
    StateInArgs,
    StateInBuffer {
        lifecycle: BufferLifecycle,
    },
    DiffInArgs {
        base_account: Account,
    },
    DiffInBuffer {
        base_account: Account,
        lifecycle: BufferLifecycle,
    },
  }
  
  #[derive(Debug, Clone)]
  pub struct CommitTask {
    pub commit_id: u64,
    pub allow_undelegation: bool,
    pub committed_account: CommittedAccount,
    pub strategy: CommitStrategy,
  }

• With this redesign, we no longer need the layers of indirection such as BaseTask, Box<dyn BaseTask>, ArgsTask, ArgsTaskType, BufferTask, BufferTaskType, etc

In short, we don’t need dyn BaseTask because our tasks are a closed set. All task types are known upfront, so an enum works best — simpler, clearer, and cheaper.

Summary by CodeRabbit

• New Features

  • Adaptive commit delivery that auto-optimizes for transaction size and uses buffer-backed delivery for large payloads.
  • Full buffer lifecycle: create, write, resize and cleanup flows for staged deliveries.

• Improvements

  • Unified Task model consolidating commit/finalize/undelegate/action flows for simpler strategy, error mapping, and execution.
  • Streamlined preparation, cleanup, and compute budgeting for more reliable delivery and testing.

• Documentation

  • Text updated to refer to Tasks and delivery strategies.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR replaces the trait-object task model with a concrete public Task enum (Commit, Finalize, Undelegate, BaseAction) and a TaskInstruction trait. It adds a public CommitTask and DataDeliveryStrategy (StateInArgs, StateInBuffer, DiffInArgs, DiffInBuffer), a BufferLifecycle module (BufferLifecycle, CreateBufferTask, DestroyTask) and buffer-based commit flows with try_optimize_tx_size. ArgsTask and BufferTask modules were removed. Many APIs and signatures were converted from Box to Task, and task-builder calls were renamed (e.g., commit_tasks → create_commit_tasks).

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[snawaz]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• refactor: Simplify Task types #665 👈 (View in Graphite)
• feat: Execute CommitDiff as BufferTask #616 : 1 other dependent PR (#617 )
• feat: Add support for CommitDiff to efficiently commit small changes in large delegated accounts #575
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

Actionable comments posted: 15

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)

magicblock-committor-service/src/tasks/task_builder.rs (2)

50-83: Avoid calling IntentPersister methods directly on &Option<P>

In create_commit_tasks you call:

commit_ids
    .iter()
    .for_each(|(pubkey, commit_id)| {
        if let Err(err) = persister.set_commit_id(
            base_intent.id,
            pubkey,
            *commit_id,
        ) {
            error!(...);
        }
    });

Given the function signature persister: &Option<P>, this will only compile if you’ve explicitly implemented IntentPersister for Option<P>. Even if that exists, it hides the “no‑persister” case and makes the control flow harder to reason about.

I’d strongly recommend aligning this with TaskStrategist::build_strategy and handling the option explicitly:

- commit_ids
-     .iter()
-     .for_each(|(pubkey, commit_id) | {
-         if let Err(err) = persister.set_commit_id(base_intent.id, pubkey, *commit_id) {
-             error!("Failed to persist commit id: {}, for message id: {} with pubkey {}: {}", commit_id, base_intent.id, pubkey, err);
-         }
-     });
+ if let Some(persister) = persister {
+     for (pubkey, commit_id) in &commit_ids {
+         if let Err(err) =
+             persister.set_commit_id(base_intent.id, pubkey, *commit_id)
+         {
+             error!(
+                 "Failed to persist commit id: {}, for message id: {} with pubkey {}: {}",
+                 commit_id, base_intent.id, pubkey, err
+             );
+         }
+     }
+ }

This makes the intent explicit and avoids relying on any special trait impl for Option<P>.

---

100-185: Minor: keep finalize/undelegate helpers symmetric and self‑contained

The small helpers finalize_task, undelegate_task, and process_commit nicely encapsulate the Task construction. To make the behavior easier to audit in the future, consider:

• Adding a short doc comment on process_commit clarifying that base actions are appended after finalize tasks (ordering matters for execution semantics).
• Keeping the helper visibility strictly local (fn is fine) and avoiding any reuse elsewhere so that all finalize‑stage behavior is centralized here.

No functional change required, but tightening and documenting this pattern will help when additional Task variants or strategies get introduced.

magicblock-committor-service/src/tasks/task_strategist.rs (3)

59-69: Make persistor handling consistent between “no ALTs” and “with ALTs” paths

In the first branch (no lookup tables), you only persist strategy when a persistor is present:

if let Some(persistor) = persistor {
    let mut persistor_visitor = PersistorVisitor { persistor, context: ... };
    tasks.iter().for_each(|task| task.visit(&mut persistor_visitor));
}

In the ALT path you instead construct PersistorVisitor unconditionally with persistor:

let mut persistor_visitor = PersistorVisitor {
    persistor,
    context: PersistorContext::PersistStrategy { uses_lookup_tables: true },
};
tasks.iter().for_each(|task| task.visit(&mut persistor_visitor));

Depending on PersistorVisitor’s field type, this is either:

• A type error (&Option<P> vs &P), or
• A behavioral change where PersistorVisitor now has to internally handle None.

To keep behavior obvious and symmetric, I’d recommend mirroring the first branch:

-        else if Self::attempt_lookup_tables(&tasks) {
-            // Persist tasks strategy
-            let mut persistor_visitor = PersistorVisitor {
-                persistor,
-                context: PersistorContext::PersistStrategy {
-                    uses_lookup_tables: true,
-                },
-            };
-            tasks
-                .iter()
-                .for_each(|task| task.visit(&mut persistor_visitor));
+        else if Self::attempt_lookup_tables(&tasks) {
+            // Persist tasks strategy
+            if let Some(persistor) = persistor {
+                let mut persistor_visitor = PersistorVisitor {
+                    persistor,
+                    context: PersistorContext::PersistStrategy {
+                        uses_lookup_tables: true,
+                    },
+                };
+                tasks
+                    .iter()
+                    .for_each(|task| task.visit(&mut persistor_visitor));
+            }

This keeps the “no persistor” case trivial and avoids encoding Option logic inside the visitor.

Also applies to: 80-89

---

157-237: Clarify that the temporary Finalize task in optimization is purely a swap placeholder

Inside try_optimize_tx_size_if_needed you use:

let tmp_task = Task::Finalize(FinalizeTask {
    delegated_account: Pubkey::new_unique(),
});
let task = std::mem::replace(&mut tasks[index], tmp_task);

followed by match task.try_optimize_tx_size() and then restoring either the optimized or original task.

This is functionally fine—the temporary finalize task is never inspected—but it’s a non‑obvious pattern. A short inline comment noting that this is just a dummy placeholder to allow moving out of tasks[index] (and that it is always overwritten before any further use) would prevent future refactors from accidentally relying on the placeholder’s contents.

No behavioral change required.

---

251-503: Tests cover mixed task types; consider one assertion on lookup_tables_keys for the “buffer-only” scenarios

The test suite here does a good job exercising:

• Single small commit (test_build_strategy_with_single_small_task),
• Buffer optimization for large single commits and multiple commits,
• ALT usage when optimization alone isn’t enough,
• Priority of larger tasks in optimization,
• Mixed task types.

One refinement that could tighten regressions around the new Task enum would be to add a quick check in test_build_strategy_creates_multiple_buffers that lookup_tables_keys remains empty (as expected when buffer optimization alone suffices). The behavior is implied today but not asserted, making future changes to ALT heuristics slightly easier to miss.

Something like:

-        for optimized_task in strategy.optimized_tasks {
+        for optimized_task in &strategy.optimized_tasks {
             assert!(matches!(optimized_task.strategy(), TaskStrategy::Buffer));
         }
-        assert!(strategy.lookup_tables_keys.is_empty());
+        assert!(strategy.lookup_tables_keys.is_empty());

so both properties are explicitly validated.

Not strictly required, but it makes the intended “buffers but no ALTs” scenario more robust under future refactors.

magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (1)

164-176: Avoid cloning full PreparationState / PreparationTask on retry path

In prepare_task_handling_errors, task.preparation_state().clone() followed by destructuring into Required(preparation_task) means we clone the entire PreparationTask (including chunks and payload) just to flip the state Required → Cleanup → Required when handling AccountAlreadyInitializedError. For large buffers this is potentially expensive on a relatively hot error path.

Consider exposing a helper on Task/PreparationState that toggles between Required and Cleanup without cloning the inner payload, or adding an accessor like fn preparation_task(&self) -> Option<&PreparationTask> so you can derive CleanupTask from a borrow instead of a cloned PreparationTask. That keeps behaviour identical while reducing allocations and copies on retries.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3beda6e and cb6b8c8.

📒 Files selected for processing (18)

• magicblock-committor-service/README.md (1 hunks)
• magicblock-committor-service/src/intent_executor/error.rs (3 hunks)
• magicblock-committor-service/src/intent_executor/mod.rs (5 hunks)
• magicblock-committor-service/src/tasks/args_task.rs (0 hunks)
• magicblock-committor-service/src/tasks/buffer_lifecycle.rs (1 hunks)
• magicblock-committor-service/src/tasks/buffer_task.rs (0 hunks)
• magicblock-committor-service/src/tasks/commit_task.rs (1 hunks)
• magicblock-committor-service/src/tasks/commit_task_builder.rs (1 hunks)
• magicblock-committor-service/src/tasks/mod.rs (5 hunks)
• magicblock-committor-service/src/tasks/task.rs (1 hunks)
• magicblock-committor-service/src/tasks/task_builder.rs (6 hunks)
• magicblock-committor-service/src/tasks/task_strategist.rs (16 hunks)
• magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2 hunks)
• magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (2 hunks)
• magicblock-committor-service/src/tasks/utils.rs (5 hunks)
• magicblock-committor-service/src/tasks/visitor.rs (1 hunks)
• magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (6 hunks)
• magicblock-committor-service/src/transaction_preparator/mod.rs (4 hunks)

💤 Files with no reviewable changes (2)

• magicblock-committor-service/src/tasks/buffer_task.rs
• magicblock-committor-service/src/tasks/args_task.rs

🧰 Additional context used

🧠 Learnings (7)

📓 Common learnings

Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.

📚 Learning: 2025-11-21T10:22:07.520Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

Applied to files:

• magicblock-committor-service/src/tasks/commit_task_builder.rs
• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/utils.rs
• magicblock-committor-service/src/transaction_preparator/mod.rs
• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/intent_executor/error.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/intent_executor/mod.rs
• magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
• magicblock-committor-service/src/tasks/task_strategist.rs

📚 Learning: 2025-10-26T08:49:31.543Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.

Applied to files:

• magicblock-committor-service/src/tasks/utils.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/tasks/utils.rs

📚 Learning: 2025-11-07T13:20:13.793Z

Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.

Applied to files:

• magicblock-committor-service/src/tasks/buffer_lifecycle.rs

📚 Learning: 2025-11-12T09:46:27.553Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.

Applied to files:

• magicblock-committor-service/src/tasks/mod.rs

📚 Learning: 2025-11-20T17:25:23.444Z

Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.

Applied to files:

• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
• magicblock-committor-service/src/tasks/task_strategist.rs

🧬 Code graph analysis (10)

magicblock-committor-service/src/tasks/task.rs (2)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (2)

• instruction (180-186)
• compute_units (189-191)

magicblock-committor-service/src/tasks/commit_task.rs (3)

• try_optimize_tx_size (251-279)
• lifecycle (90-100)
• reset_commit_id (113-133)

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (2)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)

• visit_task (22-48)

magicblock-committor-service/src/tasks/visitor.rs (1)

• visit_task (4-4)

magicblock-committor-service/src/tasks/task_builder.rs (3)

test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)

• base_intent (546-551)

magicblock-committor-service/src/tasks/commit_task_builder.rs (1)

• create_commit_task (11-51)

test-integration/test-committor-service/tests/common.rs (1)

• create_commit_task (163-181)

magicblock-committor-service/src/tasks/visitor.rs (2)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)

• visit_task (22-48)

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)

• visit_task (15-26)

magicblock-committor-service/src/tasks/commit_task.rs (2)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (4)

• instruction (180-186)
• new (29-56)
• buffer_pda (156-163)
• buffer_pda (207-214)

magicblock-committor-service/src/tasks/task.rs (4)

• instruction (33-69)
• lifecycle (129-136)
• reset_commit_id (152-157)
• try_optimize_tx_size (71-92)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (5)

magicblock-committor-program/src/instruction_builder/close_buffer.rs (1)

• create_close_ix (15-45)

magicblock-committor-program/src/instruction_builder/realloc_buffer.rs (1)

• create_realloc_buffer_ixs (26-43)

magicblock-committor-program/src/instruction_builder/write_buffer.rs (1)

• create_write_ix (17-51)

magicblock-committor-service/src/tasks/commit_task.rs (1)

• new (48-88)

magicblock-committor-program/src/state/chunks.rs (1)

• from_data_length (46-49)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)

• visit_task (15-26)

magicblock-committor-service/src/tasks/visitor.rs (1)

• visit_task (4-4)

magicblock-committor-service/src/tasks/mod.rs (4)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (2)

• instruction (180-186)
• new (29-56)

magicblock-committor-service/src/tasks/task.rs (3)

• instruction (33-69)
• strategy (120-127)
• lifecycle (129-136)

magicblock-committor-service/src/tasks/commit_task.rs (2)

• new (48-88)
• lifecycle (90-100)

magicblock-committor-service/src/tasks/task_builder.rs (2)

• finalize_task (106-110)
• undelegate_task (113-122)

magicblock-committor-service/src/intent_executor/mod.rs (1)

magicblock-committor-service/src/tasks/task_builder.rs (3)

• finalize_task (106-110)
• create_commit_tasks (25-29)
• create_commit_tasks (45-98)

magicblock-committor-service/src/tasks/task_strategist.rs (1)

magicblock-committor-service/src/tasks/commit_task.rs (1)

• new (48-88)

🪛 LanguageTool

magicblock-committor-service/README.md

[grammar] ~26-~26: Ensure spelling is correct
Context: ... Tasks are ready we need to prepare eveything for their successful execution. **Deliv...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~37-~37: For conciseness, consider replacing this expression with an adverb.
Context: ...es instruction using buffer. BufferTask at the moment supports only commits ### TaskInfoFetc...

(AT_THE_MOMENT)

🪛 markdownlint-cli2 (0.18.1)

magicblock-committor-service/README.md

28-28: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🔇 Additional comments (3)

magicblock-committor-service/src/tasks/utils.rs (1)

34-52: Verify that Task::compute_units still upper-bounds real TX compute usage

unique_involved_pubkeys, tasks_instructions, and assemble_tasks_tx are now fully driven by Task’s involved_accounts / instruction / compute_units. In particular, tasks_compute_units assumes Task::compute_units() already includes any extra work implied by the task’s strategy (e.g., buffer lifecycle init/write/cleanup for commit tasks using CommitStrategy::*_InBuffer).

If commit/buffer lifecycles are now represented partly outside the Task instruction returned here, there’s a risk of underestimating compute for those TXs. Please double‑check that:

• For every TX assembled via assemble_tasks_tx, all instructions that will actually run are covered by some Task in the slice, and
• Each such Task::compute_units() is a conservative upper bound for its full instruction set (including any buffer lifecycle work, if that’s meant to be in the same TX).

If that isn’t strictly true, consider either: (a) delegating to per‑variant compute_units that also read from BufferLifecycle, or (b) documenting and enforcing that buffer prep/cleanup always live in separate transactions whose budgets are computed elsewhere.

Also applies to: 130-132

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (1)

28-56: Confirm Chunks::from_data_length behavior for empty diffs

BufferLifecycle::new computes:

let data = if let Some(base_account) = base_account {
    dlp::compute_diff(&base_account.data, &account.account.data).to_vec()
} else {
    account.account.data.clone()
};

BufferLifecycle {
    preparation: PreparationTask {
        chunks: Chunks::from_data_length(data.len(), MAX_WRITE_CHUNK_SIZE),
        state_or_diff: data,
        // ...
    },
    // ...
}

If compute_diff happens to return an empty diff (e.g., identical base and committed data), data.len() will be 0. Depending on how Chunks::new is implemented under the hood, Chunks::from_data_length(0, ...) may either:

• Produce a valid “zero-chunk” value, or
• Panic / be considered invalid.

Given this is all service-side and can be hit at runtime, it’d be good to double‑check that the Chunks implementation explicitly supports data_len == 0 and document that assumption here (or guard it with a small check) to avoid surprising panics if a no-op diff ever occurs.

magicblock-committor-service/src/tasks/commit_task.rs (1)

222-245: Verify commit_diff_from_buffer argument type manually

The code passes CommitStateFromBufferArgs to both commit_state_from_buffer (line 209) and commit_diff_from_buffer (line 234). However, the codebase follows a pattern of separate args types for different operations: CommitStateArgs for commit_state vs. CommitDiffArgs for commit_diff.

No CommitDiffFromBufferArgs struct exists in the repository, and the dlp crate (likely from magicblock-delegation-program) is not accessible for verification. Please confirm with the dlp API documentation or source code whether commit_diff_from_buffer should accept a separate args type. If a dedicated CommitDiffFromBufferArgs exists in the dlp crate, update lines 239-242 to use it.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix spelling error.

"eveything" should be "everything".

Apply this diff:

-After our **Task**s are ready we need to prepare eveything for their successful execution. **DeliveryPreparator** - handles ALTs and commit buffers
+After our **Task**s are ready we need to prepare everything for their successful execution. **DeliveryPreparator** - handles ALTs and commit buffers

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	After our **Task**s are ready we need to prepare eveything for their successful execution. **DeliveryPreparator** - handles ALTs and commit buffers
	After our **Task**s are ready we need to prepare everything for their successful execution. **DeliveryPreparator** - handles ALTs and commit buffers

🧰 Tools

🪛 LanguageTool

[grammar] ~26-~26: Ensure spelling is correct
Context: ... Tasks are ready we need to prepare eveything for their successful execution. **Deliv...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 Prompt for AI Agents

In magicblock-committor-service/README.md around line 26, fix the spelling
mistake "eveything" to "everything" in the sentence so it reads: "After our
**Task**s are ready we need to prepare everything for their successful
execution. **DeliveryPreparator** - handles ALTs and commit buffers".

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

write_compute_units saturates to u32::MAX on large inputs—document this as an intentional cap

write_compute_units does a careful overflow-safe multiplication and then falls back to u32::MAX:

const PER_BYTE: u32 = 3;

u32::try_from(bytes_count)
    .ok()
    .and_then(|bytes_count| bytes_count.checked_mul(PER_BYTE))
    .unwrap_or(u32::MAX)

This is a sensible pattern, but the semantics (“if the length doesn’t fit in u32 or the product overflows, treat it as max CU”) aren’t obvious from call sites. A brief comment or docstring note that this acts as a saturating upper bound would make it clear that:

• Extremely large buffers intentionally force a maximal CU estimate, and
• Callers should treat u32::MAX as “don’t even try to pack this into a normal TX”.

No code change needed; just making the saturation behavior explicit will help future readers.

🤖 Prompt for AI Agents

In magicblock-committor-service/src/tasks/buffer_lifecycle.rs around lines 117
to 145, document that write_compute_units intentionally saturates to u32::MAX
when the input bytes_count cannot be converted to u32 or when the multiplication
would overflow; add a brief comment or docstring above write_compute_units
stating that it returns a per-byte estimate (PER_BYTE = 3) and treats
conversion/overflow failures as an intentional cap (u32::MAX) so callers know to
treat that value as “maximal/unsuitable for normal TX packing.”

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

Double‑check COMMIT_STATE_SIZE_THRESHOLD and warn‑level logging for base account fallback

CommitTask::new now:

• Uses a hard‑coded COMMIT_STATE_SIZE_THRESHOLD (256 bytes) to decide when to attempt get_base_account, and
• Logs warn! both when the base account is missing and when the fetch fails, before falling back to StateInArgs.

Two aspects worth validating:

• Is 256 aligned with your instruction size constraints (and MAX_WRITE_CHUNK_SIZE), or should this threshold be derived from those limits instead of being hard‑coded here?
• For workloads where many large accounts legitimately have no base account, the AccountNotFound case could spam logs at warn!. It might be better to downgrade that path to debug!/info! while keeping the actual error (Err(e)) at warn!.

These tweaks wouldn’t change functional behaviour but could materially affect observability and log noise.

---

🌐 Web query:

What guidance do the `dlp` / `magicblock-committor-program` docs give on when to prefer commit‑diff over commit‑state (e.g., size thresholds or instruction limits) on Solana?

💡 Result:

I couldn't find the dlp / magicblock-committor-program docs that mention commit-diff vs commit-state thresholds. Can you share a link (or let me continue searching other repos/pages)?

---

I'll search the codebase to understand the threshold rationale and related constraints.

---

🏁 Script executed:

```shell
#!/bin/bash
# Find COMMIT_STATE_SIZE_THRESHOLD definition and context
rg -n "COMMIT_STATE_SIZE_THRESHOLD" --type rust -B5 -A5

Length of output: 3357

---

🏁 Script executed:

#!/bin/bash
# Search for MAX_WRITE_CHUNK_SIZE or similar instruction size constants
rg -n "MAX_WRITE_CHUNK_SIZE|CHUNK_SIZE|INSTRUCTION.*SIZE|256" --type rust -C3 | head -100

Length of output: 8608

---

🏁 Script executed:

#!/bin/bash
# Search for documentation or comments about commit strategies
rg -n "DiffInArgs|StateInArgs|commit.diff|commit.state" --type rust -C2

Length of output: 17103

---

---

🏁 Script executed:

```shell
#!/bin/bash
# Search for Solana instruction size constants or tests validating threshold
rg -n "1232|instruction.*size|MAX_INSTRUCTION" --type rust -C2 | head -50

Length of output: 4582

---

🏁 Script executed:

#!/bin/bash
# Check if there are tests or comments explaining when AccountNotFound is expected
rg -n "get_base_account|AccountNotFound" --type rust -B3 -A3 | grep -A5 -B5 "test\|comment\|TODO\|FIXME\|expected"

Length of output: 929

---

---

🏁 Script executed:

```shell
#!/bin/bash
# Check MAX_INSTRUCTION_DATA_SIZE and related constants
rg -n "MAX_INSTRUCTION_DATA_SIZE|IX_WRITE_SIZE|COMMIT_STATE_SIZE" --type rust -B2 -A2

Length of output: 4356

---

🏁 Script executed:

#!/bin/bash
# Look for program-level docs or tests that validate the 256-byte threshold
fd -e md -e rs "README|DESIGN|docs" --type f | xargs grep -l "commit.diff\|commit.state\|threshold\|256" 2>/dev/null | head -10

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check if there's a relationship between account size and instruction size in the code
rg -n "instruction_data|CommitDiffArgs|CommitStateArgs" --type rust -C3 | head -80

Length of output: 6612

---

Consider deriving COMMIT_STATE_SIZE_THRESHOLD from program constraints and downgrading AccountNotFound log level

Codebase inspection reveals:

• COMMIT_STATE_SIZE_THRESHOLD (256 bytes) is heuristic-based—the inline doc cites field capacity ("8 u32 fields or 4 u64 fields") rather than alignment with MAX_INSTRUCTION_DATA_SIZE (1028 bytes) or MAX_WRITE_CHUNK_SIZE limits defined in the committor program. This threshold warrants validation against actual program constraints to ensure it's optimal.

• Both Ok(None) (missing base account) and Err(e) (fetch failure) log at warn! level (lines 63 and 68). For workloads where large accounts legitimately have no base, the AccountNotFound case could spam logs. Consider downgrading that path to debug! or info! while keeping actual errors at warn!.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Clarify semantics of reset_commit_id when buffers already exist

reset_commit_id updates both self.commit_id and the nested BufferLifecycle’s preparation.commit_id / cleanup.commit_id. Since BufferLifecycle’s PDAs (e.g., buffer_pda) also depend on commit_id, calling this after buffer initialization will cause new PDAs to be derived, while the old buffer account remains orphaned and its cleanup ix will never be sent.

If reset_commit_id is only ever called before any buffer work happens, that’s fine—but it’s not obvious from the API.

Consider either:

• Preventing or erroring on reset_commit_id when the strategy is already buffer‑based (e.g., return a TaskError instead of silently mutating), or
• Explicitly documenting via rustdoc/tests that reset_commit_id implies “start a fresh commit; caller is responsible for any existing buffers tied to the old ID.”

That will help avoid subtle account leaks if this method starts being used more broadly.

---

🏁 Script executed:

#!/bin/bash
# Inspect call sites to confirm reset_commit_id is only used before buffer initialization.
rg -n "reset_commit_id" --type rust

Length of output: 425

---

🏁 Script executed:

#!/bin/bash
# Inspect context around the main call site in intent_executor
echo "=== intent_executor/mod.rs:369 context ==="
sed -n '355,385p' magicblock-committor-service/src/intent_executor/mod.rs

echo ""
echo "=== task.rs:156 context ==="
sed -n '145,165p' magicblock-committor-service/src/tasks/task.rs

Length of output: 1346

---

---

🏁 Script executed:

#!/bin/bash
# Search for where StateInBuffer or DiffInBuffer strategies are created
echo "=== Looking for buffer strategy creation ==="
rg -n "StateInBuffer|DiffInBuffer" --type rust -B2 -A2 magicblock-committor-service/src/tasks/commit_task.rs

echo ""
echo "=== Check what handle_actions_error does and when it's called ==="
rg -n "handle_actions_error\|handle_actions_error\(" --type rust -B5 -A20 magicblock-committor-service/src/intent_executor/mod.rs | head -100

echo ""
echo "=== Find where buffers are actually created/initialized ==="
rg -n "buffer.*init\|prepare.*buffer\|new.*buffer" --type rust magicblock-committor-service/src/tasks/ -i

Length of output: 2767

---

🏁 Script executed:

#!/bin/bash
# Search for where StateInBuffer or DiffInBuffer strategies are created
echo "=== Looking for buffer strategy creation ==="
rg -n "StateInBuffer|DiffInBuffer" --type rust -B2 -A2 magicblock-committor-service/src/tasks/

echo ""
echo "=== Check the function containing line 369 in intent_executor ==="
sed -n '330,375p' magicblock-committor-service/src/intent_executor/mod.rs

echo ""
echo "=== Search for where buffers/preparation phase happens ==="
rg -n "preparation\|prepare" --type rust -B1 -A1 magicblock-committor-service/src/tasks/commit_task.rs | head -50

Length of output: 7868

---

Validate that reset_commit_id is only called before buffer work starts, or refactor to prevent orphaned accounts

The call site at magicblock-committor-service/src/intent_executor/mod.rs:369 in handle_commit_id_error() confirms the concern: reset_commit_id is invoked on tasks that may already have been migrated to buffer strategies (StateInBuffer or DiffInBuffer).

When a commit_id mismatch is detected in error recovery, the method updates self.commit_id and the lifecycle's commit_id fields, but does not recreate the BufferLifecycle or its derived PDAs. This leaves old buffer accounts orphaned with stale commit_id-derived PDAs, and their cleanup instructions will never be sent.

Either:

• Add a guard in reset_commit_id to error when called on buffer-based strategies (e.g., return a TaskError for StateInBuffer/DiffInBuffer), or
• Document explicitly via rustdoc that calling reset_commit_id invalidates any existing buffers and the caller must handle cleanup.

🤖 Prompt for AI Agents

In magicblock-committor-service/src/tasks/commit_task.rs around lines 113 to
133, reset_commit_id mutates commit_id and updates lifecycle.commit_id for
buffer strategies without recreating buffer PDAs, leaving orphaned buffer
accounts; change reset_commit_id to refuse running for buffer-based strategies
by returning a Result and erroring (e.g., TaskError) when self.strategy is
StateInBuffer or DiffInBuffer so callers must handle cleanup/recreation, update
call sites to handle the error, and add a short rustdoc note describing that
reset_commit_id is only allowed before buffer work begins.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Replace ChatGPT links and clarify try_optimize_tx_size’s lifecycle semantics

The TODO block here references ChatGPT URLs and hints at a future LifecycleTask design and removal of PreparationState. Those external links are brittle and don’t belong in long‑term code; anyone investigating this later won’t have stable access to that context.

Two suggestions:

• Move the design rationale into a short in‑repo doc (or module‑level comment) and remove the external links.
• Briefly document the current invariant: only Task::Commit is ever optimized, and all other variants must return Err(self) to keep behavior identical to the old trait‑based model.

This keeps the optimization logic understandable without relying on external conversations.

🤖 Prompt for AI Agents

In magicblock-committor-service/src/tasks/task.rs around lines 71 to 92, remove
the brittle ChatGPT URLs from the TODO block and replace them with a short,
permanent explanation either as a module-level comment or an in-repo design doc
reference; explicitly state the current invariant that only Task::Commit is
subject to optimization and all other Task variants must return Err(self) to
preserve previous trait-based behavior, and mention the intended future change
(e.g., introducing LifecycleTask or removing PreparationState) as a brief, local
design note with a link to an internal doc path instead of external ChatGPT
links.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

preparation_state is todo!() – this is a panic trap

preparation_state(&self) -> &PreparationState currently calls todo!(), so any runtime path that still queries preparation state through Task will immediately panic.

Given this is a public method, I’d treat this as non‑mergeable unless:

• All call sites have been removed/redirected and this method is genuinely unreachable (in which case it should probably be deleted instead), or
• It is implemented by delegating to the underlying variant, e.g. something along the lines of:

- pub fn preparation_state(&self) -> &PreparationState {
-     todo!()
- }
+ pub fn preparation_state(&self) -> &PreparationState {
+     match self {
+         Task::Commit(commit) => commit.preparation_state(),
+         // All non-commit tasks currently have no explicit preparation.
+         _ => &PreparationState::NotNeeded,
+     }
+ }

(Adjust to whatever CommitTask actually exposes.)

Leaving a todo!() here is a latent runtime panic and should be resolved before the Task enum becomes the central abstraction.

---

I'll verify this review by examining the Task enum structure, checking for call sites to preparation_state(), and understanding whether this method is truly unreachable or needs implementation.

---

🏁 Script executed:

#!/bin/bash
# Find the Task enum definition and its variants
rg -nP -A 50 '^\s*pub\s+enum\s+Task\b' magicblock-committor-service/src/tasks/task.rs | head -100

Length of output: 2111

---

🏁 Script executed:

#!/bin/bash
# Search for all calls to preparation_state() method in the codebase
rg -nP --type=rs '\bpreparation_state\s*\(' --context=3

Length of output: 26

---

🏁 Script executed:

#!/bin/bash
# Find CommitTask structure and its preparation_state method
rg -nP -A 10 '(?s)impl.*CommitTask.*\{|pub\s+fn\s+preparation_state' magicblock-committor-service/src/tasks/ | head -80

Length of output: 3125

---

🏁 Script executed:

#!/bin/bash
# Search for references to preparation_state as a field or accessor
rg -nP 'preparation_state' magicblock-committor-service/src/ --context=2

Length of output: 4157

---

🏁 Script executed:

#!/bin/bash
# Check the full context of the Task struct/enum file
fd 'task\.rs$' magicblock-committor-service/src/tasks/ -exec wc -l {} \;

Length of output: 145

---

🏁 Script executed:

#!/bin/bash
# Get the full context around the preparation_state implementation
sed -n '85,120p' magicblock-committor-service/src/tasks/task.rs

Length of output: 1019

---

🏁 Script executed:

#!/bin/bash
# Look for CommitTask definition and its methods
rg -nP -B 5 -A 30 'struct CommitTask|impl CommitTask' magicblock-committor-service/src/tasks/

Length of output: 7983

---

🏁 Script executed:

#!/bin/bash
# Check if there are any tests or integration code calling preparation_state
rg -nP 'preparation_state' . --type=rs -g '!target/'

Length of output: 26

---

🏁 Script executed:

#!/bin/bash
# Find Task enum variants to understand all possible types
rg -nP '^enum Task' -A 100 magicblock-committor-service/src/tasks/task.rs | grep -E '^\s+(Commit|[A-Z]\w+)\s*\('

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Get full Task enum definition
rg -nP -A 200 'pub enum Task' magicblock-committor-service/src/tasks/task.rs | head -150

Length of output: 5439

---

🏁 Script executed:

#!/bin/bash
# Look at PreparationState enum
rg -nP -B 2 -A 10 'enum PreparationState|struct PreparationState'

Length of output: 857

---

🏁 Script executed:

#!/bin/bash
# Check if CommitTask has a preparation_state method
rg -nP -B 3 -A 5 'impl CommitTask' magicblock-committor-service/src/tasks/ | grep -A 5 'preparation_state'

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Look for the full Task impl block for preparation-related methods
sed -n '1,300p' magicblock-committor-service/src/tasks/task.rs | tail -100

Length of output: 3322

---

🏁 Script executed:

#!/bin/bash
# See what's in the entire task.rs file structure
head -150 magicblock-committor-service/src/tasks/task.rs

Length of output: 5172

---

🏁 Script executed:

#!/bin/bash
# Check for any pub fn that returns PreparationState
rg -nP 'pub fn.*PreparationState' magicblock-committor-service/src/

Length of output: 108

---

🏁 Script executed:

#!/bin/bash
# Look at the git diff to understand what changed
git log --oneline -20

Length of output: 1085

---

🏁 Script executed:

#!/bin/bash
# Check all usages of Task enum
rg -nP '\bTask::' magicblock-committor-service/src/ --context=2

Length of output: 14971

---

🏁 Script executed:

#!/bin/bash
# Check imports to understand what is being used where
head -50 magicblock-committor-service/src/tasks/task.rs

Length of output: 1696

---

🏁 Script executed:

#!/bin/bash
# Full scan for preparation related code
rg -nP 'preparation' magicblock-committor-service/src/tasks/ -C 5

Length of output: 11000

---

🏁 Script executed:

#!/bin/bash
# Check the current state of the entire switch_preparation_state method and prep methods
sed -n '90,130p' magicblock-committor-service/src/tasks/task.rs

Length of output: 1141

---

🏁 Script executed:

#!/bin/bash
# Look for any other Task impl blocks or trait implementations
rg -nP 'impl.*Task' magicblock-committor-service/src/tasks/task.rs -A 2

Length of output: 131

---

🏁 Script executed:

#!/bin/bash
# Understand if this is part of a trait implementation
rg -nP 'trait.*Preparation|pub trait' magicblock-committor-service/src/tasks/

Length of output: 154

---

I found critical evidence that confirms this review comment. The preparation_state() method is actively being called in the codebase at multiple locations in delivery_preparator.rs (lines 94, 165, 430), and all of these are production code paths that pattern-match on the result. This confirms the panic trap is a real, non-mergeable issue.

---

preparation_state() returns todo!() – active panic trap in production paths

This method is not unreachable; it is actively called at delivery_preparator.rs:94, 165, and 430 where the code pattern-matches on its result (e.g., PreparationState::Required(_) and PreparationState::Cleanup(_)). Any invocation will panic immediately.

The method must delegate to the underlying Task variant:

- pub fn preparation_state(&self) -> &PreparationState {
-     todo!()
- }
+ pub fn preparation_state(&self) -> &PreparationState {
+     match self {
+         Task::Commit(commit) => commit.preparation_state(),
+         Task::Finalize(_) | Task::Undelegate(_) | Task::BaseAction(_) => &PreparationState::NotNeeded,
+     }
+ }

Alternatively, if only Commit tasks carry preparation state, delegate through lifecycle():

- pub fn preparation_state(&self) -> &PreparationState {
-     todo!()
- }
+ pub fn preparation_state(&self) -> &PreparationState {
+     match self {
+         Task::Commit(commit) => commit.preparation_state(),
+         _ => &PreparationState::NotNeeded,
+     }
+ }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	/// Nothing to prepare for [`ArgsTaskType`] type
	pub fn preparation_state(&self) -> &PreparationState {
	todo!()
	}
	pub fn switch_preparation_state(
	&mut self,
	new_state: PreparationState,
	) -> TaskResult<()> {
	if !matches!(new_state, PreparationState::NotNeeded) {
	Err(TaskError::PreparationStateTransitionError)
	} else {
	// Do nothing
	Ok(())
	}
	}
	/// Nothing to prepare for [`ArgsTaskType`] type
	pub fn preparation_state(&self) -> &PreparationState {
	match self {
	Task::Commit(commit) => commit.preparation_state(),
	_ => &PreparationState::NotNeeded,
	}
	}
	pub fn switch_preparation_state(
	&mut self,
	new_state: PreparationState,
	) -> TaskResult<()> {
	if !matches!(new_state, PreparationState::NotNeeded) {
	Err(TaskError::PreparationStateTransitionError)
	} else {
	// Do nothing
	Ok(())
	}
	}

🤖 Prompt for AI Agents

magicblock-committor-service/src/tasks/task.rs lines 94-109: the
preparation_state() currently panics via todo!() but is called in production;
implement it to return the real PreparationState from the underlying enum
variant (or via self.lifecycle() if only Commit tasks carry it) by matching on
self and delegating to the variant that holds the PreparationState, returning a
reference (&PreparationState); ensure you handle all variants (returning
PreparationState::NotNeeded for variants without preparation) so callers can
safely pattern-match without panics.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

switch_preparation_state ignores inner state for Commit tasks

This implementation only checks whether new_state is NotNeeded and otherwise returns PreparationStateTransitionError, but it never updates any state inside CommitTask (or other variants).

If callers previously used this hook to drive actual preparation/cleanup state on commit tasks, this will silently change behavior:

• Transitions that used to be allowed (e.g. Pending -> Prepared) will now all fail except NotNeeded.
• The underlying CommitTask (and any BufferLifecycle) never see the new state.

Please either:

• Delegate per variant (e.g., Task::Commit(commit) => commit.switch_preparation_state(new_state) and return Ok(())/error accordingly), or
• Remove this API entirely if the preparation state is being phased out, and update callers to stop relying on it.

Right now it sits in an inconsistent halfway state.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Improve observability for InternalError::TaskError

With the new InternalError::TaskError(#[from] TaskError) variant, any failed switch_preparation_state now bubbles up through prepare_for_delivery as FailedToPrepareBufferAccounts. Unlike buffer send errors, there’s no targeted logging for this path, and the TaskError itself currently only carries a generic message.

If PreparationStateTransitionError is something you ever expect to see in production, consider either:

• Logging TaskError explicitly at the point where you map it into InternalError or where you handle FailedToPrepareBufferAccounts, or
• Enriching TaskError with transition context (from/to state) so downstream logs contain enough detail to debug invalid transitions.

🤖 Prompt for AI Agents

In
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
around lines 523-525, the InternalError::TaskError(#[from] TaskError) variant
swallows transition context so prepare_for_delivery maps
PreparationStateTransitionError into FailedToPrepareBufferAccounts with no
actionable details; fix by enriching TaskError with transition context (include
from_state and to_state and any identifying buffer id) when the
PreparationStateTransitionError is constructed or converted, and/or add an
explicit process_logger.error at the conversion/handling site that logs the
TaskError and the transition details before mapping it into InternalError so
downstream logs contain enough information to debug invalid state transitions.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (8)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)

35-63: Incomplete migration: stale ArgsTaskType references.

This code block references task.task_type and matches on ArgsTaskType::Commit / ArgsTaskType::CommitDiff, but:

1. ArgsTaskType is being removed by this PR in favor of CommitStrategy variants.
2. The Task enum doesn't have a task_type field — you already extracted commit_task on line 25.

The logic should operate on commit_task fields directly (e.g., commit_task.commit_id, commit_task.committed_account.pubkey).

🔎 Proposed fix

-                match &task.task_type {
-                    ArgsTaskType::Commit(task) => {
-                        if let Err(err) = self.persistor.set_commit_strategy(
-                            task.commit_id,
-                            &task.committed_account.pubkey,
-                            commit_strategy,
-                        ) {
-                            error!(
-                                "Failed to persist commit strategy {}: {}",
-                                commit_strategy.as_str(),
-                                err
-                            );
-                        }
-                    }
-                    ArgsTaskType::CommitDiff(task) => {
-                        if let Err(err) = self.persistor.set_commit_strategy(
-                            task.commit_id,
-                            &task.committed_account.pubkey,
-                            commit_strategy,
-                        ) {
-                            error!(
-                                "Failed to persist commit strategy {}: {}",
-                                commit_strategy.as_str(),
-                                err
-                            );
-                        }
-                    }
-                    _ => {}
-                }
+                if let Err(err) = self.persistor.set_commit_strategy(
+                    commit_task.commit_id,
+                    &commit_task.committed_account.pubkey,
+                    commit_strategy,
+                ) {
+                    error!(
+                        "Failed to persist commit strategy {}: {}",
+                        commit_strategy.as_str(),
+                        err
+                    );
+                }

magicblock-committor-service/src/tasks/task_builder.rs (3)

52-81: Remove stale ArgsTask-based code.

This method still uses ArgsTask, ArgsTaskType::Commit, and ArgsTaskType::CommitDiff which are being removed by this PR. The new canonical path uses CommitTaskBuilder::create_commit_task. This dead code should be removed to complete the migration.

---

123-128: Undefined variable: commit_id_fetcher.

Line 124 and 125 reference commit_id_fetcher, but the parameter is named task_info_fetcher. This will cause a compilation error.

🔎 Proposed fix

             tokio::join!(
-                commit_id_fetcher.fetch_next_commit_ids(&committed_pubkeys),
-                commit_id_fetcher
+                task_info_fetcher.fetch_next_commit_ids(&committed_pubkeys),
+                task_info_fetcher
                     .get_base_accounts(diffable_pubkeys.as_slice())
             )

---

142-148: Undefined variable in closure: persister.

Line 145 references persister but this is the parameter name. Since it's &Option<P>, accessing methods directly won't work — you need to handle the Option:

if let Some(ref p) = persister {
    if let Err(err) = p.set_commit_id(...) { ... }
}

magicblock-committor-service/src/tasks/mod.rs (2)

141-164: Test uses await in synchronous function – will not compile.

test_args_task_instruction_serialization is marked #[test] (sync) but uses .await at line 163. Either:

• Change to #[tokio::test] and mark the function async, or
• Use tokio::runtime::Runtime::new().unwrap().block_on(...) for the async call

-#[test]
-fn test_args_task_instruction_serialization() {
+#[tokio::test]
+async fn test_args_task_instruction_serialization() {

---

469-483: Test uses await without async context – will not compile.

test_build_strategy_with_single_small_task at line 472 uses .await but is a sync #[test]. Apply the same fix as above.

magicblock-committor-service/src/tasks/task_strategist.rs (2)

469-483: Multiple tests use await in sync #[test] functions – will not compile.

Tests at lines 469, 485, 505, 586, 611, 637, 657, 671 are marked #[test] but use .await. Either:

• Convert to #[tokio::test] async fn, or
• Remove .await calls by using blocking alternatives

Example fix:

-#[test]
-fn test_build_strategy_with_single_small_task() {
+#[tokio::test]
+async fn test_build_strategy_with_single_small_task() {

Also applies to: 485-503, 505-523, 586-609, 611-635, 637-655, 657-669, 671-709

---

525-584: Tests reference removed types Box<dyn BaseTask> and old signatures.

Tests at lines 525-584 still use Box<dyn BaseTask> and the old create_test_commit_task signature with 3 parameters. These need to be updated to use Task directly.

♻️ Duplicate comments (7)

magicblock-committor-service/README.md (1)

26-26: Fix spelling error: "eveything" → "everything".

magicblock-committor-service/src/tasks/task.rs (4)

42-63: TODO block contains external ChatGPT links – prefer in-repo documentation.

The TODO references external URLs that may become stale. Consider moving the design rationale into an in-repo doc or issue, and update the comment to reference that instead.

---

82-89: Hardcoded compute units could drift from subtask implementations.

Consider delegating to subtask methods to centralize the budget:

 pub fn compute_units(&self) -> u32 {
     match &self {
-        Task::Commit(_) => 70_000,
+        Task::Commit(task) => task.compute_units(),
         Task::BaseAction(task) => task.action.compute_units,
-        Task::Undelegate(_) => 70_000,
-        Task::Finalize(_) => 70_000,
+        Task::Undelegate(task) => task.compute_units(),
+        Task::Finalize(task) => task.compute_units(),
     }
 }

This requires adding compute_units() methods to the subtask types.

---

65-68: preparation_state() still contains todo!() – runtime panic risk.

This method is called in delivery_preparator.rs at lines 102, 172, and 437 where the code pattern-matches on PreparationState::Required and PreparationState::Cleanup. The todo!() will cause immediate panics.

Implement delegation to the underlying variant:

 pub fn preparation_state(&self) -> &PreparationState {
-    todo!()
+    match self {
+        Task::Commit(commit) => commit.preparation_state(),
+        Task::Finalize(_) | Task::Undelegate(_) | Task::BaseAction(_) => {
+            &PreparationState::NotNeeded
+        }
+    }
 }

Note: This requires CommitTask to expose a preparation_state() method. If CommitTask doesn't have one, you'll need to derive the state from self.lifecycle() or add the method.

---

70-80: switch_preparation_state only accepts NotNeeded – incomplete transition logic.

The current implementation rejects all transitions except to NotNeeded, but delivery_preparator.rs calls this with PreparationState::Cleanup(...) and PreparationState::Required(...) at lines 142, 177-178, and 182-184.

This will cause PreparationStateTransitionError for legitimate transitions. Implement proper delegation:

 pub fn switch_preparation_state(
     &mut self,
     new_state: PreparationState,
 ) -> TaskResult<()> {
-    if !matches!(new_state, PreparationState::NotNeeded) {
-        Err(TaskError::PreparationStateTransitionError)
-    } else {
-        // Do nothing
-        Ok(())
-    }
+    match self {
+        Task::Commit(commit) => commit.switch_preparation_state(new_state),
+        _ => {
+            if matches!(new_state, PreparationState::NotNeeded) {
+                Ok(())
+            } else {
+                Err(TaskError::PreparationStateTransitionError)
+            }
+        }
+    }
 }

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (1)

138-145: Document write_compute_units saturation behavior.

The overflow-safe computation with unwrap_or(u32::MAX) is correct, but the semantics aren't obvious. Add a brief doc comment explaining this acts as a saturating upper bound.

magicblock-committor-service/src/tasks/commit_task.rs (1)

131-151: reset_commit_id on buffer strategies may orphan buffer accounts.

When called on StateInBuffer or DiffInBuffer strategies, this method updates the commit_id in the lifecycle but doesn't recreate the buffer PDAs. The old buffer accounts (derived from the previous commit_id) become orphaned.

From past review: either add a guard to error on buffer strategies, or document that callers must handle cleanup of existing buffers.

🧹 Nitpick comments (1)

magicblock-committor-service/src/tasks/commit_task_builder.rs (1)

8-8: Consider removing duplicate code.

The TaskBuilderImpl::create_commit_task method in task_builder.rs (lines 52-81 in the snippets) contains similar synchronous logic. If CommitTaskBuilder::create_commit_task is the new canonical path, consider removing the duplicate in TaskBuilderImpl to avoid confusion and maintenance burden.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb6b8c8 and f43badc.

📒 Files selected for processing (16)

• magicblock-committor-service/README.md
• magicblock-committor-service/src/intent_executor/error.rs
• magicblock-committor-service/src/intent_executor/mod.rs
• magicblock-committor-service/src/tasks/buffer_lifecycle.rs
• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/commit_task_builder.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/task_builder.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs
• magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs
• magicblock-committor-service/src/tasks/utils.rs
• magicblock-committor-service/src/tasks/visitor.rs
• magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
• magicblock-committor-service/src/transaction_preparator/mod.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• magicblock-committor-service/src/tasks/utils.rs

🧰 Additional context used

🧠 Learnings (8)

📚 Learning: 2025-11-21T10:22:07.520Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/commit_task_builder.rs
• magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
• magicblock-committor-service/src/intent_executor/mod.rs
• magicblock-committor-service/src/transaction_preparator/mod.rs
• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/intent_executor/error.rs

📚 Learning: 2025-11-12T09:46:27.553Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task_strategist.rs

📚 Learning: 2025-11-20T17:25:23.444Z

Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task_strategist.rs

📚 Learning: 2025-11-07T13:20:13.793Z

Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/buffer_lifecycle.rs

📚 Learning: 2025-12-03T09:36:01.527Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).

Applied to files:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/commit_task.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:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/commit_task.rs

📚 Learning: 2025-10-26T08:49:31.543Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-12-03T09:33:48.707Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.

Applied to files:

• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task_strategist.rs

🧬 Code graph analysis (10)

magicblock-committor-service/src/tasks/task.rs (3)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (3)

• instruction (69-90)
• instruction (180-186)
• compute_units (189-191)

magicblock-committor-service/src/tasks/commit_task.rs (4)

• instruction (40-56)
• try_optimize_tx_size (246-274)
• lifecycle (108-118)
• reset_commit_id (131-151)

magicblock-committor-service/src/tasks/mod.rs (4)

• instruction (34-34)
• instruction (66-73)
• instruction (82-84)
• instruction (93-114)

magicblock-committor-service/src/tasks/commit_task_builder.rs (1)

magicblock-committor-service/src/tasks/task_builder.rs (1)

• create_commit_task (53-81)

magicblock-committor-service/src/tasks/task_builder.rs (1)

magicblock-committor-service/src/tasks/commit_task_builder.rs (1)

• create_commit_task (11-51)

magicblock-committor-service/src/intent_executor/mod.rs (3)

magicblock-committor-service/src/tasks/task_builder.rs (2)

• create_commit_tasks (27-31)
• create_commit_tasks (87-162)

magicblock-committor-service/src/tasks/task_strategist.rs (1)

• build_strategy (168-219)

magicblock-committor-service/src/tasks/task.rs (1)

• strategy (91-98)

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (2)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)

• visit_task (22-66)

magicblock-committor-service/src/tasks/visitor.rs (1)

• visit_task (4-4)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)

• visit_task (15-26)

magicblock-committor-service/src/tasks/visitor.rs (1)

• visit_task (4-4)

magicblock-committor-service/src/tasks/commit_task.rs (2)

magicblock-committor-service/src/persist/types/commit_strategy.rs (1)

• args (17-23)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (1)

• new (29-56)

magicblock-committor-service/src/tasks/mod.rs (5)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (3)

• instruction (69-90)
• instruction (180-186)
• new (29-56)

magicblock-committor-service/src/tasks/commit_task.rs (3)

• instruction (40-56)
• new (66-106)
• lifecycle (108-118)

magicblock-committor-service/src/tasks/task.rs (3)

• instruction (22-29)
• strategy (91-98)
• lifecycle (100-107)

magicblock-committor-service/src/tasks/args_task.rs (3)

• instruction (51-123)
• new (42-47)
• strategy (173-175)

magicblock-committor-service/src/tasks/buffer_task.rs (3)

• instruction (101-146)
• new (42-50)
• strategy (179-181)

magicblock-committor-service/src/tasks/visitor.rs (2)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)

• visit_task (22-66)

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)

• visit_task (15-26)

magicblock-committor-service/src/tasks/task_strategist.rs (3)

magicblock-committor-service/src/tasks/task_builder.rs (2)

• finalize_tasks (34-37)
• finalize_tasks (165-249)

magicblock-committor-service/src/tasks/commit_task.rs (1)

• new (66-106)

magicblock-committor-service/src/tasks/args_task.rs (1)

• new (42-47)

🪛 LanguageTool

magicblock-committor-service/README.md

[grammar] ~26-~26: Ensure spelling is correct
Context: ... Tasks are ready we need to prepare eveything for their successful execution. **Deliv...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~37-~37: For conciseness, consider replacing this expression with an adverb.
Context: ...es instruction using buffer. BufferTask at the moment supports only commits ### TaskInfoFetc...

(AT_THE_MOMENT)

🔇 Additional comments (24)

magicblock-committor-service/src/tasks/visitor.rs (1)

1-5: LGTM!

Clean consolidation of the visitor pattern from two methods (visit_args_task, visit_buffer_task) to a single visit_task method operating on the unified Task enum.

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)

14-26: LGTM!

The unified visit_task implementation cleanly extracts CommitMeta from Task::Commit variants while correctly handling non-Commit tasks by setting the result to None.

magicblock-committor-service/src/transaction_preparator/mod.rs (1)

11-11: LGTM!

The trait and implementation correctly migrate from Box<dyn BaseTask> to concrete &[Task] slices, aligning with the simplified task type system.

Also applies to: 28-28, 40-40, 115-115

magicblock-committor-service/src/intent_executor/error.rs (1)

174-243: LGTM!

The error mapping logic correctly migrates to use &[Task] and calls task.task_type() to determine the task variant for error classification. The task_type() method is properly implemented on the Task enum and handles all variants: Commit, BaseAction, Undelegate, and Finalize.

magicblock-committor-service/src/tasks/commit_task_builder.rs (1)

10-51: Clean async builder implementation with appropriate fallback strategy.

The threshold check correctly references CommitTask::COMMIT_STATE_SIZE_THRESHOLD, and the error handling aligns with project patterns: logging warnings on fetch failures or missing accounts while falling back safely to StateInArgs. This provides good resilience without panicking on transient errors.

magicblock-committor-service/src/intent_executor/mod.rs (3)

199-204: Consistent method rename from commit_tasks to create_commit_tasks.

The rename is applied consistently at both call sites. This aligns with the task builder API changes.

Also applies to: 224-228

---

668-672: Signature update to accept &[Task] is correct.

The change from &[Box<dyn BaseTask>] to &[Task] aligns with the refactor goals. The function body correctly passes tasks to IntentTransactionErrorMapper.

---

141-173: Remove the unused try_unite_tasks function or clarify its purpose.

The try_unite_tasks function is well-structured with proper error handling and guards against CPI limits, but it is not called anywhere in the codebase. Either remove it if it's dead code from an incomplete refactor, or add a comment explaining why it's being kept for future use.

⛔ Skipped due to learnings

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

magicblock-committor-service/src/tasks/task.rs (1)

12-19: Task enum structure and delegating implementations look correct.

The enum cleanly encapsulates all task variants. The TaskInstruction impl correctly delegates to inner types, and helper methods (strategy, lifecycle, task_type, visit, reset_commit_id) properly route to the appropriate variant.

The LabelValue implementation provides good observability by exposing commit strategy details.

Also applies to: 21-30, 91-129, 131-146

magicblock-committor-service/src/tasks/commit_task.rs (4)

15-37: Clean CommitStrategy design with proper instruction delegation.

The CommitStrategy enum elegantly captures all four commit modes, and the TaskInstruction implementation correctly routes to the appropriate instruction builder for each strategy variant.

Also applies to: 39-57

---

59-106: Async constructor with graceful fallback to StateInArgs.

The CommitTask::new constructor properly:

• Uses the size threshold to decide when to attempt diff-based commits
• Falls back gracefully to StateInArgs when base account fetch fails
• Logs appropriately at warn level for both cases

As noted in past reviews, consider downgrading the AccountNotFound log from warn to debug/info for workloads where missing base accounts are expected.

---

108-129: Helper methods and instruction builders are correct.

The lifecycle(), task_strategy(), and all create_commit_*_ix methods correctly handle each strategy variant and properly construct the PDAs and instruction arguments.

Also applies to: 153-240

---

242-274: try_optimize_tx_size correctly transitions Args to Buffer strategies.

The optimization path properly creates new BufferLifecycle instances when transitioning from *InArgs to *InBuffer variants, and correctly returns Err(self) when already optimized.

As noted in past reviews, document that this only affects CommitStrategy and callers are responsible for PreparationState management.

magicblock-committor-service/src/tasks/mod.rs (2)

17-24: Module structure cleanly exposes the Task-centric API.

The new module layout with buffer_lifecycle, commit_task, commit_task_builder and glob re-exports provides a clean public surface for the refactored task system.

---

32-35: TaskInstruction trait and task type implementations are well-structured.

The trait provides a clean interface for instruction generation. The UndelegateTask, FinalizeTask, and BaseActionTask structs with their TaskInstruction implementations correctly produce the appropriate instructions.

Also applies to: 58-115

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (3)

19-57: BufferLifecycle constructor correctly handles diff vs state data.

The new method properly computes either the diff (when base_account is provided) or uses the full account data, then initializes both preparation and cleanup tasks with consistent commit_id and pubkey.

---

67-90: Instruction generation methods are well-implemented.

The instruction(), realloc_instructions(), and write_instructions() methods correctly:

• Compute account sizes using borsh serialization
• Derive PDAs consistently
• Generate chunked write instructions

The #[allow(clippy::let_and_return)] annotations are appropriate for the return-value-only usage pattern.

Also applies to: 97-136

---

173-214: CleanupTask implementation is correct.

The struct properly captures the minimum data needed for cleanup, and the instruction() method generates the correct close instruction. The max_tx_fit_count_with_budget() constant provides useful transaction packing guidance.

magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)

33-34: Import updates align with the Task-centric refactor.

Correctly imports Task and TaskError instead of the removed BaseTask types.

---

95-144: prepare_task signature updated to accept &mut Task.

The method correctly uses the new type. Note that this relies on task.preparation_state() which currently contains todo!() in task.rs – this will panic at runtime until that's fixed.

---

148-187: prepare_task_handling_errors and cleanup properly updated.

Both methods now operate on Task types. The cleanup logic correctly filters for PreparationState::Cleanup and batches cleanup instructions.

Also applies to: 422-484

---

554-556: InternalError::TaskError variant provides proper error propagation.

The #[from] derive enables automatic conversion from TaskError. As noted in past reviews, consider enriching TaskError with transition context for better debugging.

magicblock-committor-service/src/tasks/task_strategist.rs (2)

20-22: TransactionStrategy and build_strategy correctly use Vec<Task>.

The struct and method signatures are properly updated. The optimization and ALT fallback logic remains intact.

Also applies to: 168-219

---

276-358: try_optimize_tx_size_if_needed correctly operates on &mut [Task].

The optimization loop properly:

• Uses a temporary Task::Finalize for swapping during optimization
• Calls task.try_optimize_tx_size() and handles Ok/Err correctly
• Maintains heap-based priority by instruction size

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Documentation inconsistent with refactor.

Line 37 still references ArgsTask and BufferTask, but this PR removes those types in favor of a unified Task enum with variants (Commit, Finalize, Undelegate, BaseAction) and CommitStrategy variants (StateInArgs, StateInBuffer, DiffInArgs, DiffInBuffer). Update the documentation to reflect the new architecture.

Also, the "(to be renamed...)" notes on line 31 may be stale if this PR completes the terminology transition.

🧰 Tools

🪛 LanguageTool

[style] ~37-~37: For conciseness, consider replacing this expression with an adverb.
Context: ...es instruction using buffer. BufferTask at the moment supports only commits ### TaskInfoFetc...

(AT_THE_MOMENT)

[coderabbitai]

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)

magicblock-committor-service/src/tasks/task_strategist.rs (1)

471-500: Update test helpers to return Task variants for consistency.

The test helper create_test_commit_task (line 429) returns Task, but create_test_base_action_task, create_test_finalize_task, and create_test_undelegate_task still return ArgsTask. Per the PR objectives to replace ArgsTask with the Task enum, these helpers should be updated to return Task variants:

• create_test_base_action_task should return Task::BaseAction(...)
• create_test_finalize_task should return Task::Finalize(...)
• create_test_undelegate_task should return Task::Undelegate(...)

🔎 Proposed refactor

-fn create_test_base_action_task(len: usize) -> ArgsTask {
-    ArgsTask::new(ArgsTaskType::BaseAction(BaseActionTask {
+fn create_test_base_action_task(len: usize) -> Task {
+    Task::BaseAction(BaseActionTask {
         action: BaseAction {
             destination_program: Pubkey::new_unique(),
             escrow_authority: Pubkey::new_unique(),
             account_metas_per_program: vec![],
             data_per_program: ProgramArgs {
                 data: vec![0; len],
                 escrow_index: 0,
             },
             compute_units: 30_000,
         },
-    }))
+    })
 }

-fn create_test_finalize_task() -> ArgsTask {
-    ArgsTask::new(ArgsTaskType::Finalize(FinalizeTask {
+fn create_test_finalize_task() -> Task {
+    Task::Finalize(FinalizeTask {
         delegated_account: Pubkey::new_unique(),
-    }))
+    })
 }

-fn create_test_undelegate_task() -> ArgsTask {
-    ArgsTask::new(ArgsTaskType::Undelegate(UndelegateTask {
+fn create_test_undelegate_task() -> Task {
+    Task::Undelegate(UndelegateTask {
         delegated_account: Pubkey::new_unique(),
         owner_program: system_program_id(),
         rent_reimbursement: Pubkey::new_unique(),
-    }))
+    })
 }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1132b6 and 2dca09f.

📒 Files selected for processing (8)

• magicblock-committor-service/src/tasks/args_task.rs
• magicblock-committor-service/src/tasks/buffer_task.rs
• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/task_builder.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs

💤 Files with no reviewable changes (2)

• magicblock-committor-service/src/tasks/args_task.rs
• magicblock-committor-service/src/tasks/buffer_task.rs

🧰 Additional context used

🧠 Learnings (11)

📓 Common learnings

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

📚 Learning: 2025-12-03T09:33:48.707Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.

Applied to files:

• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/task_builder.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/mod.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:

• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/task_builder.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-12-03T09:36:01.527Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).

Applied to files:

• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/task_builder.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-11-07T13:20:13.793Z

Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.

Applied to files:

• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-11-07T14:20:31.457Z

Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.

Applied to files:

• magicblock-committor-service/src/tasks/task_builder.rs

📚 Learning: 2025-11-21T10:22:07.520Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

Applied to files:

• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task.rs

📚 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:

• magicblock-committor-service/src/tasks/mod.rs

📚 Learning: 2025-11-12T09:46:27.553Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-10-26T08:49:31.543Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-11-20T17:25:23.444Z

Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs

🧬 Code graph analysis (3)

magicblock-committor-service/src/tasks/commit_task.rs (1)

magicblock-committor-service/src/tasks/mod.rs (2)

• instruction (33-33)
• instruction (205-211)

magicblock-committor-service/src/tasks/task_builder.rs (2)

magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (3)

• fetch_next_commit_ids (24-27)
• fetch_next_commit_ids (166-233)
• new (58-65)

magicblock-committor-service/src/tasks/commit_task.rs (1)

• new (63-90)

magicblock-committor-service/src/tasks/mod.rs (2)

magicblock-committor-service/src/tasks/commit_task.rs (2)

• instruction (37-53)
• lifecycle (92-104)

magicblock-committor-service/src/tasks/task.rs (3)

• instruction (22-29)
• strategy (91-98)
• lifecycle (100-107)

[coderabbitai]

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 (2)

test-integration/test-committor-service/tests/test_delivery_preparator.rs (1)

18-30: Update tests to use Task::Commit(...) instead of removed types BufferTask and BufferTaskType.

The tests test_prepare_10kb_buffer (lines 24, 26) and test_already_initialized_error_handled (lines 208, 210, 247, 249) use BufferTaskType::Commit and BufferTask::new_preparation_required, which are being removed. Replace with Task::Commit(...) to match the pattern used in test_prepare_multiple_buffers and test_prepare_cleanup_and_reprepare_mixed_tasks.

test-integration/test-committor-service/tests/test_transaction_preparator.rs (1)

183-204: Compilation error: Old task types not updated.

This test still references BufferTask, ArgsTask, ArgsTaskType, and Box<dyn BaseTask>, which have been removed in this refactor. Update to use the new Task enum variants:

🔎 Proposed fix

-    let buffer_commit_task = BufferTask::new_preparation_required(
-        TaskBuilderImpl::create_commit_task(
-            1,
-            true,
-            committed_account.clone(),
-            None,
-        )
-        .task_type
-        .into(),
-    );
-    let tasks = vec![
-        // commit account
-        Box::new(buffer_commit_task.clone()) as Box<dyn BaseTask>,
-        // finalize account
-        Box::new(ArgsTask::new(ArgsTaskType::Finalize(FinalizeTask {
-            delegated_account: committed_account.pubkey,
-        }))),
-        // BaseAction
-        Box::new(ArgsTask::new(ArgsTaskType::BaseAction(BaseActionTask {
-            action: base_action,
-        }))),
-    ];
+    let tasks = vec![
+        // commit account
+        Task::Commit(CommitTask::new(1, true, committed_account.clone(), None)),
+        // finalize account
+        Task::Finalize(FinalizeTask {
+            delegated_account: committed_account.pubkey,
+        }),
+        // BaseAction
+        Task::BaseAction(BaseActionTask {
+            action: base_action,
+        }),
+    ];

♻️ Duplicate comments (6)

magicblock-committor-service/README.md (2)

26-26: Fix spelling error.

"eveything" should be "everything".

---

34-37: Documentation inconsistent with the refactor.

Line 37 still references ArgsTask and BufferTask, but this PR removes those types in favor of a unified Task enum with CommitStrategy variants (StateInArgs, StateInBuffer, DiffInArgs, DiffInBuffer). The "(to be renamed...)" note on line 31 also appears stale since this PR completes the terminology transition.

magicblock-committor-service/src/tasks/task.rs (1)

42-63: Remove external ChatGPT links from TODO comments.

The TODO block references ChatGPT URLs which are brittle and don't belong in long-term code. Replace with a brief inline design note or link to an internal design doc.

magicblock-committor-service/src/tasks/commit_task.rs (1)

158-180: Clarify reset_commit_id behavior when buffers already exist.

As flagged in a previous review, calling reset_commit_id on a task with an existing buffer-based strategy updates the commit_id and lifecycle fields but does not recreate the BufferLifecycle or its derived PDAs. This could leave old buffer accounts orphaned.

Consider adding a guard or documentation clarifying that callers are responsible for any existing buffers tied to the old ID.

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (1)

135-142: Document saturation behavior in write_compute_units.

As noted in a previous review, this method intentionally saturates to u32::MAX when the input is too large or overflows. A brief docstring would clarify this is an intentional cap for callers.

🔎 Suggested documentation

+    /// Returns compute units required for writing `bytes_count` bytes.
+    /// Uses 3 CU per byte. If the count doesn't fit in u32 or the product
+    /// overflows, returns u32::MAX as a saturating upper bound.
     pub fn write_compute_units(&self, bytes_count: usize) -> u32 {

magicblock-committor-service/src/tasks/mod.rs (1)

122-128: TaskError still lacks transition context.

As noted in a previous review, PreparationStateTransitionError carries no information about which transition failed. Consider adding { from: PreparationState, to: PreparationState } fields to aid debugging.

🧹 Nitpick comments (5)

magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)

105-112: Remove commented-out PreparationState code.

These commented blocks are dead code from the old flow. Since the migration to task.lifecycle() is complete, remove the commented sections to improve readability.

🔎 Proposed cleanup

-        // let PreparationState::Required(preparation_task) =
-        //     task.preparation_state()
-        // else {
-        //     return Ok(());
-        // };
         let Some(lifecycle) = task.lifecycle() else {
             return Ok(());
         };

-        //let cleanup_task = preparation_task.cleanup_task();
-        //task.switch_preparation_state(PreparationState::Cleanup(cleanup_task))?;
         Ok(())

Also applies to: 150-152

---

181-204: Remove extensive commented-out recovery logic.

This large block of commented code obscures the actual flow. The new lifecycle-based approach at lines 197-204 is sufficient.

🔎 Proposed cleanup

-        // Prepare cleanup task
-        // let PreparationState::Required(preparation_task) =
-        //     task.preparation_state().clone()
-        // else {
-        //     return Ok(());
-        // };
-        // task.switch_preparation_state(PreparationState::Cleanup(
-        //     preparation_task.cleanup_task(),
-        // ))?;
-        // self.cleanup(authority, std::slice::from_ref(task), &[])
-        //     .await?;
-        // task.switch_preparation_state(PreparationState::Required(
-        //     preparation_task,
-        // ))?;
-
-        // self.prepare_task(authority, task, persister).await
-
         let Some(_lifecycle) = task.lifecycle() else {
             return Ok(());
         };

---

452-463: Remove commented-out PreparationState matching in cleanup.

The new task.lifecycle().map(|lc| &lc.cleanup) pattern is cleaner. Remove the commented block.

🔎 Proposed cleanup

         let cleanup_tasks: Vec<_> = tasks
             .iter()
             .filter_map(|task| {
                 task.lifecycle().map(|lc| &lc.cleanup)
-                //if let PreparationState::Cleanup(cleanup_task) =
-                //    task.preparation_state()
-                //{
-                //    Some(cleanup_task)
-                //} else {
-                //    None
-                //}
             })
             .collect();

magicblock-committor-service/src/tasks/task.rs (1)

70-80: switch_preparation_state only accepts NotNeeded - document or remove.

This method returns an error for any state other than NotNeeded, which seems like placeholder behavior. If the preparation state model is being phased out in favor of lifecycle(), consider removing these methods entirely.

magicblock-committor-service/src/tasks/mod.rs (1)

19-21: Consider explicit re-exports over wildcards.

Wildcard re-exports (pub use x::*;) can make the public API surface harder to audit and may inadvertently expose internal items. Consider listing explicit re-exports (e.g., pub use buffer_lifecycle::{BufferLifecycle, CreateBufferTask, DestroyTask};) so readers can quickly see what this module provides.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2dca09f and d16b407.

📒 Files selected for processing (17)

• magicblock-committor-service/README.md
• magicblock-committor-service/src/intent_executor/error.rs
• magicblock-committor-service/src/intent_executor/mod.rs
• magicblock-committor-service/src/tasks/buffer_lifecycle.rs
• magicblock-committor-service/src/tasks/commit_task.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/task_builder.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs
• magicblock-committor-service/src/tasks/utils.rs
• magicblock-committor-service/src/tasks/visitor.rs
• magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
• magicblock-committor-service/src/transaction_preparator/mod.rs
• test-integration/test-committor-service/tests/common.rs
• test-integration/test-committor-service/tests/test_delivery_preparator.rs
• test-integration/test-committor-service/tests/test_transaction_preparator.rs

🧰 Additional context used

🧠 Learnings (12)

📓 Common learnings

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

📚 Learning: 2025-11-21T10:22:07.520Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

Applied to files:

• magicblock-committor-service/src/intent_executor/mod.rs
• magicblock-committor-service/src/intent_executor/error.rs
• magicblock-committor-service/src/tasks/utils.rs
• magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
• test-integration/test-committor-service/tests/test_transaction_preparator.rs
• magicblock-committor-service/src/transaction_preparator/mod.rs
• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/commit_task.rs
• test-integration/test-committor-service/tests/test_delivery_preparator.rs

📚 Learning: 2025-10-26T08:49:31.543Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.

Applied to files:

• magicblock-committor-service/src/tasks/utils.rs
• magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
• test-integration/test-committor-service/tests/test_transaction_preparator.rs
• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-12-03T09:36:01.527Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).

Applied to files:

• magicblock-committor-service/src/tasks/task_builder.rs
• test-integration/test-committor-service/tests/test_transaction_preparator.rs
• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/commit_task.rs

📚 Learning: 2025-11-07T14:20:31.457Z

Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.

Applied to files:

• magicblock-committor-service/src/tasks/task_builder.rs

📚 Learning: 2025-12-03T09:33:48.707Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.

Applied to files:

• magicblock-committor-service/src/tasks/task_builder.rs
• test-integration/test-committor-service/tests/common.rs
• test-integration/test-committor-service/tests/test_transaction_preparator.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/mod.rs
• magicblock-committor-service/src/tasks/commit_task.rs
• test-integration/test-committor-service/tests/test_delivery_preparator.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:

• magicblock-committor-service/src/tasks/task_builder.rs
• test-integration/test-committor-service/tests/common.rs
• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/commit_task.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:

• test-integration/test-committor-service/tests/test_transaction_preparator.rs

📚 Learning: 2025-11-07T13:20:13.793Z

Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/buffer_lifecycle.rs
• magicblock-committor-service/src/tasks/commit_task.rs

📚 Learning: 2025-11-12T09:46:27.553Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/mod.rs
• test-integration/test-committor-service/tests/test_delivery_preparator.rs

📚 Learning: 2025-11-20T17:25:23.444Z

Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs
• magicblock-committor-service/src/tasks/task_strategist.rs
• magicblock-committor-service/src/tasks/mod.rs

📚 Learning: 2025-11-24T14:21:00.996Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.

Applied to files:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs

🧬 Code graph analysis (8)

magicblock-committor-service/src/tasks/visitor.rs (2)

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)

• visit_task (15-27)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)

• visit_task (22-75)

magicblock-committor-service/src/intent_executor/mod.rs (1)

magicblock-committor-service/src/tasks/task_builder.rs (2)

• create_commit_tasks (24-28)
• create_commit_tasks (44-123)

magicblock-committor-service/src/tasks/task_builder.rs (3)

magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (3)

• fetch_next_commit_ids (24-27)
• fetch_next_commit_ids (166-233)
• new (58-65)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (1)

• new (26-53)

magicblock-committor-service/src/tasks/commit_task.rs (1)

• new (67-112)

test-integration/test-committor-service/tests/common.rs (2)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (1)

• new (26-53)

magicblock-committor-service/src/tasks/commit_task.rs (1)

• new (67-112)

magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)

magicblock-committor-service/src/tasks/task.rs (1)

• lifecycle (82-89)

magicblock-committor-service/src/tasks/commit_task.rs (1)

• lifecycle (114-126)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (1)

• max_tx_fit_count_with_budget (191-193)

magicblock-committor-service/src/transaction_preparator/error.rs (2)

• from (21-23)
• from (36-41)

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (2)

magicblock-committor-service/src/tasks/visitor.rs (1)

• visit_task (4-4)

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)

• visit_task (22-75)

magicblock-committor-service/src/tasks/task_strategist.rs (2)

magicblock-committor-service/src/tasks/task_builder.rs (2)

• create_commit_tasks (24-28)
• create_commit_tasks (44-123)

magicblock-committor-service/src/tasks/task.rs (1)

• strategy (100-107)

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (6)

magicblock-committor-program/src/instruction_builder/close_buffer.rs (1)

• create_close_ix (15-45)

magicblock-committor-program/src/instruction_builder/realloc_buffer.rs (1)

• create_realloc_buffer_ixs (26-43)

magicblock-committor-program/src/instruction_builder/write_buffer.rs (1)

• create_write_ix (17-51)

test-integration/test-committor-service/tests/common.rs (1)

• new (53-56)

magicblock-committor-program/src/state/chunks.rs (1)

• from_data_length (48-51)

test-integration/test-cloning/tests/07_subscription_limits.rs (1)

• chunk (74-74)

🪛 GitHub Actions: Run CI - Format

magicblock-committor-service/src/tasks/utils.rs

[error] 11-11: Rustfmt: formatting differences detected in imports; run 'cargo +nightly fmt' to format.

---

[error] 11-11: Rustfmt: reorder/import changes required; run 'cargo +nightly fmt' to format.

magicblock-committor-service/src/tasks/task.rs

[error] 3-3: Rustfmt: code formatting differences detected. Import lines order changed by fmt; run 'cargo +nightly fmt' to format.

---

[error] 130-130: Rustfmt: formatting of Task instruction handling differs; run 'cargo +nightly fmt' to fix.

magicblock-committor-service/src/tasks/task_strategist.rs

[error] 4-4: Rustfmt: missing import order; run 'cargo +nightly fmt' to format.

---

[error] 15-15: Rustfmt: import of TaskInstruction moved; run 'cargo +nightly fmt' to format.

magicblock-committor-service/src/tasks/mod.rs

[error] 130-130: Rustfmt: code formatting differences detected. Diff in import block; run 'cargo +nightly fmt' to format.

🪛 LanguageTool

magicblock-committor-service/README.md

[grammar] ~26-~26: Ensure spelling is correct
Context: ... Tasks are ready we need to prepare eveything for their successful execution. **Deliv...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~37-~37: For conciseness, consider replacing this expression with an adverb.
Context: ...es instruction using buffer. BufferTask at the moment supports only commits ### TaskInfoFetc...

(AT_THE_MOMENT)

🔇 Additional comments (27)

magicblock-committor-service/src/intent_executor/error.rs (1)

14-14: LGTM! Type changes align with the Task enum refactor.

The import, function signature, and struct field updates correctly replace Box<dyn BaseTask> with &[Task], maintaining the same error mapping logic while eliminating dynamic dispatch overhead.

Also applies to: 177-177, 259-259

magicblock-committor-service/src/tasks/visitor.rs (1)

1-5: LGTM! Clean trait simplification.

The unified visit_task method replaces the previous per-type visitor methods (visit_args_task, visit_buffer_task), aligning with the Task enum design. Implementations can use pattern matching on Task variants as shown in persistor_visitor.rs and utility_visitor.rs.

test-integration/test-committor-service/tests/common.rs (1)

175-192: LGTM! Test helper correctly uses the new CommitTask constructor.

The create_commit_task function properly uses CommitTask::new(commit_id, allow_undelegation, committed_account, base_account). Passing None for base_account will result in StateInArgs delivery strategy per the constructor logic in commit_task.rs, which is appropriate for test scenarios with small data sizes.

magicblock-committor-service/src/intent_executor/mod.rs (3)

51-51: LGTM! Import updated to use Task enum.

The import change from BaseTask to Task, TaskType aligns with the unified Task abstraction.

---

164-169: LGTM! Method calls updated to use renamed create_commit_tasks.

Both call sites correctly use the renamed method TaskBuilderImpl::create_commit_tasks() which returns Vec<Task> per the new API in task_builder.rs.

Also applies to: 188-193

---

633-637: LGTM! Function signature updated for Task slice.

The execute_message_with_retries method now correctly accepts &[Task] instead of &[Box<dyn BaseTask>], enabling the error mapper to operate on the unified Task type.

magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)

3-3: LGTM! Clean visitor implementation using Task pattern matching.

The visit_task implementation correctly uses if let Task::Commit(ref commit_task) to extract commit metadata, with an else branch setting None for non-Commit variants. This is consistent with the pattern used in persistor_visitor.rs.

Also applies to: 14-27

test-integration/test-committor-service/tests/test_delivery_preparator.rs (1)

86-92: LGTM! Tests correctly updated to use Task::Commit.

These test sections properly construct tasks using Task::Commit(create_commit_task(...)) which aligns with the new unified Task enum API.

Also applies to: 164-167, 303-306, 393-395

magicblock-committor-service/src/transaction_preparator/mod.rs (1)

11-11: LGTM! TransactionPreparator trait updated for Task enum.

The import, documentation, trait signature, and implementation are consistently updated to use &[Task] instead of &[Box<dyn BaseTask>], completing the refactor for the transaction preparation layer.

Also applies to: 28-28, 40-40, 115-115

magicblock-committor-service/src/tasks/utils.rs (1)

32-40: LGTM - Clean migration to concrete Task type.

The function signatures are correctly updated from &[Box<dyn BaseTask>] to &[Task], and the method calls (involved_accounts, instruction, compute_units) now operate directly on the Task enum. The logic remains functionally equivalent.

Also applies to: 52-59, 62-72, 128-130

test-integration/test-committor-service/tests/test_transaction_preparator.rs (1)

35-40: LGTM - Properly migrated to Task enum variants.

These test sections correctly use Task::Commit(CommitTask::new(...)), Task::Finalize(FinalizeTask {...}), and Task::Undelegate(UndelegateTask {...}) patterns.

Also applies to: 86-111, 264-275

magicblock-committor-service/src/tasks/task_builder.rs (2)

44-122: LGTM - Well-structured Task construction.

The create_commit_tasks method cleanly builds Task::Commit and Task::BaseAction variants. The threshold-based filtering for diffable accounts correctly references CommitTask::COMMIT_STATE_SIZE_THRESHOLD, and the parallel fetching of commit IDs and base accounts is well implemented.

---

126-210: LGTM - Finalize task builder correctly produces Task variants.

The finalize_tasks method properly constructs Task::Finalize, Task::Undelegate, and Task::BaseAction variants through clean helper functions.

magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (1)

64-96: LGTM - Lifecycle-based preparation and cleanup flow.

The migration to task.lifecycle() for both preparation and cleanup is clean. The flow correctly handles tasks with and without buffer lifecycles.

Also applies to: 440-503

magicblock-committor-service/src/tasks/task.rs (1)

12-19: LGTM - Clean Task enum design.

The Task enum with its variants and the delegating implementations for TaskInstruction, lifecycle(), strategy(), task_type(), visit(), reset_commit_id(), and LabelValue are well-structured. The delegation pattern keeps the logic in the inner task types where it belongs.

Also applies to: 21-30, 82-89, 100-116, 118-128, 131-146

magicblock-committor-service/src/tasks/commit_task.rs (2)

67-112: Well-designed strategy selection logic.

The new constructor cleanly selects the appropriate DataDeliveryStrategy based on:

1. Account size vs COMMIT_STATE_SIZE_THRESHOLD (256 bytes)
2. Computed diff size vs MAX_TX_SIZE (1232 bytes)

The fallback from diff-based to buffer-based when the diff is too large is correct.

---

271-303: LGTM - Transaction size optimization.

try_optimize_tx_size correctly flips StateInArgs → StateInBuffer and DiffInArgs → DiffInBuffer while creating new BufferLifecycle instances. Returning Err(self) for already-optimized strategies prevents infinite optimization loops.

magicblock-committor-service/src/tasks/buffer_lifecycle.rs (2)

19-54: LGTM - Clean lifecycle initialization.

BufferLifecycle::new correctly computes the diff (if base_account provided) and initializes both CreateBufferTask and DestroyTask with consistent metadata. The Chunks::from_data_length call properly sets up chunking based on MAX_WRITE_CHUNK_SIZE.

---

56-168: LGTM - Comprehensive buffer task implementations.

CreateBufferTask provides all necessary instruction builders (init, realloc, write) with appropriate compute unit estimates. DestroyTask correctly generates close instructions with the batching constant max_tx_fit_count_with_budget() = 8. PDA derivations are consistent between both task types.

Also applies to: 170-212

magicblock-committor-service/src/tasks/task_strategist.rs (3)

69-142: LGTM - Strategy execution mode selection.

The build_execution_strategy logic correctly handles the tradeoff between SingleStage (possibly with ALTs) and TwoStage modes. The MAX_UNITED_TASKS_LEN check prevents CPI limit issues, and the ALT comparison logic prefers TwoStage when it avoids ALT latency.

---

277-358: LGTM - Task optimization loop.

The try_optimize_tx_size_if_needed method correctly uses a binary heap to prioritize optimizing the largest tasks first. The temporary task swap pattern (lines 323-328) is a reasonable approach to avoid borrow checker issues.

---

428-499: LGTM - Comprehensive test coverage.

The test helpers (create_test_commit_task, create_test_base_action_task, create_test_finalize_task, create_test_undelegate_task) correctly construct Task enum variants. The tests cover single/multiple tasks, optimization scenarios, ALT usage, and execution mode selection.

Also applies to: 501-862

magicblock-committor-service/src/tasks/mod.rs (5)

29-32: Clean trait design.

The TaskInstruction trait is minimal and focused, aligning well with the refactor goal of simplifying the task system.

---

55-61: New delivery model aligns with PR objectives.

The DataDelivery enum cleanly captures the four commit strategies (State/Diff × Args/Buffer) described in the PR objectives.

---

97-120: Verify is_signer: false is correct for all handler accounts.

All account metas are set to is_signer: false (line 106). This is likely correct since the DLP program handles signing, but confirm that no accounts in BaseAction require signer status at the instruction level.

---

268-320: Good boundary test for transaction size limits.

The test correctly validates that DestroyTask::max_tx_fit_count_with_budget() returns the exact limit before overflow. The overflow assertion (lines 317-319) confirms the boundary is tight.

---

130-140: Fix formatting to pass CI.

The pipeline reports a formatting error in the import block at line 130. Run cargo +nightly fmt to fix.

#!/bin/bash
# Description: Check the formatting issues in this file
cd magicblock-committor-service && cargo +nightly fmt -- --check src/tasks/mod.rs 2>&1 | head -50

⛔ Skipped due to learnings

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.

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.

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).

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

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.

Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.

Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.

Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

magicblock-committor-service/src/tasks/task.rs (1)

31-40: Acknowledge the performance TODO.

The current implementation recomputes the full instruction per call. The TODO is noted; consider caching involved keys in the underlying task types once the enum design stabilizes.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66c9abc and 192bfde.

📒 Files selected for processing (3)

• magicblock-committor-service/src/tasks/task.rs
• test-integration/test-committor-service/tests/test_delivery_preparator.rs
• test-integration/test-committor-service/tests/test_intent_executor.rs

🧰 Additional context used

🧠 Learnings (13)

📓 Common learnings

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

📚 Learning: 2025-11-21T10:22:07.520Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

Applied to files:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs
• test-integration/test-committor-service/tests/test_intent_executor.rs
• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-12-03T09:33:48.707Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.

Applied to files:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs
• test-integration/test-committor-service/tests/test_intent_executor.rs

📚 Learning: 2025-11-19T09:34:37.917Z

Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.

Applied to files:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs

📚 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/test-committor-service/tests/test_delivery_preparator.rs

📚 Learning: 2025-12-03T09:36:01.527Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).

Applied to files:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs
• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-11-24T14:21:00.996Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.

Applied to files:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs

📚 Learning: 2025-11-12T09:46:27.553Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.

Applied to files:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs
• magicblock-committor-service/src/tasks/task.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:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs

📚 Learning: 2025-11-07T13:20:13.793Z

Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.

Applied to files:

• test-integration/test-committor-service/tests/test_delivery_preparator.rs
• magicblock-committor-service/src/tasks/task.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:

• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-10-26T08:49:31.543Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs

📚 Learning: 2025-11-20T17:25:23.444Z

Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.

Applied to files:

• magicblock-committor-service/src/tasks/task.rs

🧬 Code graph analysis (1)

test-integration/test-committor-service/tests/test_delivery_preparator.rs (2)

test-integration/test-committor-service/tests/common.rs (2)

• create_commit_task (175-192)
• generate_random_bytes (167-172)

magicblock-committor-service/src/tasks/task.rs (1)

• strategy (90-97)

🔇 Additional comments (12)

test-integration/test-committor-service/tests/test_intent_executor.rs (1)

966-972: LGTM!

The method rename from commit_tasks to create_commit_tasks aligns with the broader refactor to the unified Task-based workflow. The test logic remains unchanged.

test-integration/test-committor-service/tests/test_delivery_preparator.rs (5)

7-7: LGTM!

Import updated to reflect the new unified Task type and PreparationState from the refactored tasks module.

---

22-24: LGTM!

Task construction correctly wraps CommitTask in Task::Commit variant, aligning with the new enum-based workflow. The optimized_tasks field naming is consistent with the refactored TransactionStrategy.

---

80-93: LGTM!

Clean iterator-based construction of multiple Task::Commit variants. The buffer sizes (10KB, 100KB, 500KB) provide good coverage for testing the preparation logic at different scales.

---

160-172: LGTM!

Consistent transformation pattern using into_iter() and Task::Commit wrapping for lookup table tests.

---

292-301: LGTM!

Mixed task test correctly constructs multiple Task::Commit variants with cloned tasks, enabling subsequent mutation and re-preparation testing.

magicblock-committor-service/src/tasks/task.rs (6)

11-19: LGTM!

The Task enum provides a clean, closed-set representation of all task variants. The #[allow(clippy::large_enum_variant)] is appropriate here since the enum is designed to handle variant size differences inherent to the different task types.

---

21-30: LGTM!

Clean delegation of TaskInstruction to inner variants maintains the same instruction generation behavior as the previous trait-based model.

---

60-70: switch_preparation_state only accepts NotNeeded — verify this matches the intended design.

This implementation rejects all state transitions except NotNeeded. If CommitTask previously supported preparation state transitions (e.g., Pending -> Prepared), this change would silently break that behavior. Confirm this restriction is intentional for the new Task-based workflow.

---

72-97: LGTM!

The lifecycle() and strategy() methods correctly delegate to the underlying variants, with non-Commit tasks appropriately returning None or DeliveryStrategy::Args.

---

99-118: LGTM!

The task_type(), visit(), and reset_commit_id() methods provide clean mappings. The silent return for non-Commit tasks in reset_commit_id is appropriate since only Commit tasks have commit IDs to reset.

---

121-137: LGTM!

The LabelValue implementation provides detailed metric labels that distinguish commit strategies (state/diff × args/buffer), enabling fine-grained observability of task execution patterns.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Check calls to preparation_state() in test files
rg -n "\.preparation_state\(\)" --type rust test-integration/test-committor-service/tests/ -B 2 -A 2

Repository: magicblock-labs/magicblock-validator

Length of output: 5089

---

🏁 Script executed:

# Check the actual implementation in task.rs
rg -n "pub fn preparation_state" --type rust magicblock-committor-service/src/tasks/task.rs -A 5

Repository: magicblock-labs/magicblock-validator

Length of output: 235

---

preparation_state() panics with todo!() — critical defect.

The method at lines 55-58 contains a todo!() but is actively called by tests in test_transaction_preparator.rs (lines 142, 230) and test_delivery_preparator.rs (lines 41, 108, 222, 259, 316, 408). Tests match on the returned PreparationState enum, so any test invoking this method will panic. Either implement the method to return the appropriate state, or remove it if truly unused.