refactor: Simplify Task types #665
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR is a major architectural refactoring that replaces the trait-object-based task system with a concrete Task enum design. ArgsTask and BufferTask modules are removed entirely, replaced by a unified Task enum with variants for Commit, Finalize, Undelegate, and BaseAction. New modules for buffer lifecycle and commit task management are introduced. All public and internal API surfaces transition from Possibly related PRs
Suggested reviewers
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
snawaz
force-pushed
the
snawaz/commit-task
branch
2 times, most recently
from
230abd8 to
bdf21ae
Compare
snawaz
force-pushed
the
snawaz/commit-task
branch
from
bdf21ae to
568c647
Compare
snawaz
force-pushed
the
snawaz/diff-stragegy
branch
from
a58c61d to
8ecc317
Compare
snawaz
force-pushed
the
snawaz/commit-task
branch
from
568c647 to
28302c9
Compare
snawaz
force-pushed
the
snawaz/diff-stragegy
branch
from
8ecc317 to
3beda6e
Compare
snawaz
force-pushed
the
snawaz/commit-task
branch
from
28302c9 to
cb6b8c8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 callingIntentPersistermethods directly on&Option<P>In
create_commit_tasksyou 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 implementedIntentPersisterforOption<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_strategyand 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‑containedThe small helpers
finalize_task,undelegate_task, andprocess_commitnicely encapsulate theTaskconstruction. To make the behavior easier to audit in the future, consider:
- Adding a short doc comment on
process_commitclarifying that base actions are appended after finalize tasks (ordering matters for execution semantics).- Keeping the helper visibility strictly local (
fnis 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
Taskvariants 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” pathsIn 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
PersistorVisitorunconditionally withpersistor: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
PersistorVisitornow has to internally handleNone.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 temporaryFinalizetask in optimization is purely a swap placeholderInside
try_optimize_tx_size_if_neededyou 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 onlookup_tables_keysfor the “buffer-only” scenariosThe 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_buffersthatlookup_tables_keysremains 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 fullPreparationState/PreparationTaskon retry pathIn
prepare_task_handling_errors,task.preparation_state().clone()followed by destructuring intoRequired(preparation_task)means we clone the entirePreparationTask(including chunks and payload) just to flip the stateRequired → Cleanup → Requiredwhen handlingAccountAlreadyInitializedError. For large buffers this is potentially expensive on a relatively hot error path.Consider exposing a helper on
Task/PreparationStatethat toggles betweenRequiredandCleanupwithout cloning the inner payload, or adding an accessor likefn preparation_task(&self) -> Option<&PreparationTask>so you can deriveCleanupTaskfrom a borrow instead of a clonedPreparationTask. That keeps behaviour identical while reducing allocations and copies on retries.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 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.rsmagicblock-committor-service/src/tasks/task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/transaction_preparator/mod.rsmagicblock-committor-service/src/tasks/commit_task.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-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.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-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 thatTask::compute_unitsstill upper-bounds real TX compute usage
unique_involved_pubkeys,tasks_instructions, andassemble_tasks_txare now fully driven byTask’sinvolved_accounts/instruction/compute_units. In particular,tasks_compute_unitsassumesTask::compute_units()already includes any extra work implied by the task’s strategy (e.g., buffer lifecycle init/write/cleanup for commit tasks usingCommitStrategy::*_InBuffer).If commit/buffer lifecycles are now represented partly outside the
Taskinstruction 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 someTaskin 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_unitsthat also read fromBufferLifecycle, 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: ConfirmChunks::from_data_lengthbehavior for empty diffs
BufferLifecycle::newcomputes: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_diffhappens to return an empty diff (e.g., identical base and committed data),data.len()will be 0. Depending on howChunks::newis 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
Chunksimplementation explicitly supportsdata_len == 0and 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: Verifycommit_diff_from_bufferargument type manuallyThe code passes
CommitStateFromBufferArgsto bothcommit_state_from_buffer(line 209) andcommit_diff_from_buffer(line 234). However, the codebase follows a pattern of separate args types for different operations:CommitStateArgsforcommit_statevs.CommitDiffArgsforcommit_diff.No
CommitDiffFromBufferArgsstruct exists in the repository, and thedlpcrate (likely frommagicblock-delegation-program) is not accessible for verification. Please confirm with the dlp API documentation or source code whethercommit_diff_from_buffershould accept a separate args type. If a dedicatedCommitDiffFromBufferArgsexists in the dlp crate, update lines 239-242 to use it.
|
|
||
| ## DeliveryPreparator | ||
| After our **BaseTask**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 eveything for their successful execution. **DeliveryPreparator** - handles ALTs and commit buffers |
There was a problem hiding this comment.
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.
| 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".
| /// Returns realloc instruction required for Buffer preparation | ||
| #[allow(clippy::let_and_return)] | ||
| pub fn write_instructions(&self, authority: &Pubkey) -> Vec<Instruction> { | ||
| let chunks_iter = | ||
| ChangesetChunks::new(&self.chunks, self.chunks.chunk_size()) | ||
| .iter(&self.state_or_diff); | ||
| let write_instructions = chunks_iter | ||
| .map(|chunk| { | ||
| create_write_ix(CreateWriteIxArgs { | ||
| authority: *authority, | ||
| pubkey: self.pubkey, | ||
| offset: chunk.offset, | ||
| data_chunk: chunk.data_chunk, | ||
| commit_id: self.commit_id, | ||
| }) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| write_instructions | ||
| } | ||
|
|
||
| pub fn write_compute_units(&self, bytes_count: usize) -> u32 { | ||
| 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) | ||
| } |
There was a problem hiding this comment.
🧹 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::MAXas “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.”
| pub async fn new<C: TaskInfoFetcher>( | ||
| commit_id: u64, | ||
| allow_undelegation: bool, | ||
| committed_account: CommittedAccount, | ||
| task_info_fetcher: &Arc<C>, | ||
| ) -> Self { | ||
| let base_account = if committed_account.account.data.len() | ||
| > CommitTask::COMMIT_STATE_SIZE_THRESHOLD | ||
| { | ||
| match task_info_fetcher | ||
| .get_base_account(&committed_account.pubkey) | ||
| .await | ||
| { | ||
| Ok(Some(account)) => Some(account), | ||
| Ok(None) => { | ||
| log::warn!("AccountNotFound for commit_diff, pubkey: {}, commit_id: {}, Falling back to commit_state.", | ||
| committed_account.pubkey, commit_id); | ||
| None | ||
| } | ||
| Err(e) => { | ||
| log::warn!("Failed to fetch base account for commit diff, pubkey: {}, commit_id: {}, error: {}. Falling back to commit_state.", | ||
| committed_account.pubkey, commit_id, e); | ||
| None | ||
| } | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| Self { | ||
| commit_id, | ||
| allow_undelegation, | ||
| committed_account, | ||
| strategy: match base_account { | ||
| Some(base_account) => { | ||
| CommitStrategy::DiffInArgs { base_account } | ||
| } | ||
| None => CommitStrategy::StateInArgs, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 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 attemptget_base_account, and - Logs
warn!both when the base account is missing and when the fetch fails, before falling back toStateInArgs.
Two aspects worth validating:
- Is
256aligned with your instruction size constraints (andMAX_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
AccountNotFoundcase could spam logs atwarn!. It might be better to downgrade that path todebug!/info!while keeping the actual error (Err(e)) atwarn!.
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 -100Length 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 -C2Length 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 -10Length 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 -80Length 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 withMAX_INSTRUCTION_DATA_SIZE(1028 bytes) orMAX_WRITE_CHUNK_SIZElimits defined in the committor program. This threshold warrants validation against actual program constraints to ensure it's optimal. -
Both
Ok(None)(missing base account) andErr(e)(fetch failure) log atwarn!level (lines 63 and 68). For workloads where large accounts legitimately have no base, theAccountNotFoundcase could spam logs. Consider downgrading that path todebug!orinfo!while keeping actual errors atwarn!.
| pub fn reset_commit_id(&mut self, commit_id: u64) { | ||
| if self.commit_id == commit_id { | ||
| return; | ||
| } | ||
|
|
||
| self.commit_id = commit_id; | ||
| let lifecycle = match &mut self.strategy { | ||
| CommitStrategy::StateInArgs => None, | ||
| CommitStrategy::StateInBuffer { lifecycle } => Some(lifecycle), | ||
| CommitStrategy::DiffInArgs { base_account: _ } => None, | ||
| CommitStrategy::DiffInBuffer { | ||
| base_account: _, | ||
| lifecycle, | ||
| } => Some(lifecycle), | ||
| }; | ||
|
|
||
| if let Some(lifecycle) = lifecycle { | ||
| lifecycle.preparation.commit_id = commit_id; | ||
| lifecycle.cleanup.commit_id = commit_id; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 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_idwhen the strategy is already buffer‑based (e.g., return aTaskErrorinstead of silently mutating), or - Explicitly documenting via rustdoc/tests that
reset_commit_idimplies “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 rustLength 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.rsLength 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/ -iLength 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 -50Length 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_idto error when called on buffer-based strategies (e.g., return aTaskErrorfor StateInBuffer/DiffInBuffer), or - Document explicitly via rustdoc that calling
reset_commit_idinvalidates 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.
| // FIXME: DELETE | ||
| pub fn compute_diff(&self) -> Option<dlp::rkyv::AlignedVec> { | ||
| panic!() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Remove or hide CommitTask::compute_diff panicking stub
CommitTask::compute_diff is pub, marked // FIXME: DELETE, and unconditionally panic!(). Combined with the unused use core::panic; import, this looks like leftover scaffolding.
Even if nothing calls it today, leaving a public method that always panics is a footgun for future refactors.
I’d suggest:
- Removing the method (and the
use core::panic;import) in this PR once you’ve confirmed there are no callers, or - If you really need it temporarily, making it
pub(crate)or#[cfg(test)]and documenting that it is not for production use.
🏁 Script executed:
#!/bin/bash
# Verify that CommitTask::compute_diff has no call sites before deleting.
rg -n "compute_diff\s*\(" --type rustLength of output: 348
🏁 Script executed:
#!/bin/bash
# Inspect context around line 181 in commit_task.rs where compute_diff is called
sed -n '170,190p' magicblock-committor-service/src/tasks/commit_task.rsLength of output: 579
🏁 Script executed:
#!/bin/bash
# Get broader context to understand the function containing line 181
sed -n '150,200p' magicblock-committor-service/src/tasks/commit_task.rsLength of output: 1467
🏁 Script executed:
#!/bin/bash
# Check if compute_diff stub is called from multiple places or just line 181
rg -n -B5 -A5 "diff: compute_diff\(" magicblock-committor-service/src/tasks/commit_task.rsLength of output: 450
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or other call paths that might invoke compute_diff
rg -n "\.compute_diff\(" magicblock-committor-service/src/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the use core::panic import to see if it's related to compute_diff
rg -n "use core::panic" magicblock-committor-service/src/tasks/commit_task.rsLength of output: 18
🏁 Script executed:
#!/bin/bash
# Look for all uses of 'panic!' in the file to understand panic usage patterns
rg -n "panic!" magicblock-committor-service/src/tasks/commit_task.rsLength of output: 20
🏁 Script executed:
#!/bin/bash
# Check if compute_diff can be reached from any code path or if the code is dead
rg -n -B10 "diff: compute_diff\(" magicblock-committor-service/src/tasks/commit_task.rs | head -30Length of output: 344
🏁 Script executed:
#!/bin/bash
# Get the line 181 context with more lines before it to see the function structure
sed -n '140,190p' magicblock-committor-service/src/tasks/commit_task.rsLength of output: 1692
🏁 Script executed:
#!/bin/bash
# Check usage of create_commit_ix or any function that might call compute_diff indirectly
grep -n "create_commit_ix\|compute_diff" magicblock-committor-service/src/tasks/commit_task.rsLength of output: 201
🏁 Script executed:
#!/bin/bash
# Verify the file structure: find where line 181 falls in terms of functions
rg -n "fn \|pub fn\|impl\|struct\|enum" magicblock-committor-service/src/tasks/commit_task.rs | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get exact lines around 181 with more context
awk 'NR >= 175 && NR <= 190 {print NR": "$0}' magicblock-committor-service/src/tasks/commit_task.rsLength of output: 597
🏁 Script executed:
#!/bin/bash
# Get the complete context of the function that contains line 181
sed -n '140,200p' magicblock-committor-service/src/tasks/commit_task.rs | cat -nLength of output: 2356
🏁 Script executed:
#!/bin/bash
# Look at the file around lines 175-190 to see if compute_diff call is reachable
sed -n '170,195p' magicblock-committor-service/src/tasks/commit_task.rs | nl -v 170Length of output: 40
🏁 Script executed:
#!/bin/bash
# Find which function contains line 181 and check if code path is reachable
awk 'NR==154 || NR==181 || NR<=200 && /^[[:space:]]*(pub\s+)?fn\s+/ {print NR": "$0}' magicblock-committor-service/src/tasks/commit_task.rs | tail -20Length of output: 524
🏁 Script executed:
#!/bin/bash
# Simple approach: show lines 150-200 to get full context
head -200 magicblock-committor-service/src/tasks/commit_task.rs | tail -50Length of output: 1457
🏁 Script executed:
#!/bin/bash
# Get the actual content between lines 150-200
sed '1,149d;200q' magicblock-committor-service/src/tasks/commit_task.rs | cat -n -v | sed 's/^/150 +/'Length of output: 2079
🏁 Script executed:
#!/bin/bash
# Simpler: just cat the range
cat magicblock-committor-service/src/tasks/commit_task.rs | sed -n '150,200p'Length of output: 1467
🏁 Script executed:
#!/bin/bash
# Check if the compute_diff call at line 181 is in dead code or actually reachable
rg -n -B20 "diff: compute_diff" magicblock-committor-service/src/tasks/commit_task.rsLength of output: 771
🏁 Script executed:
#!/bin/bash
# Find all match/if branches that might contain line 181 to check if code is reachable
ast-grep --pattern 'match $_' magicblock-committor-service/src/tasks/commit_task.rsLength of output: 8747
🏁 Script executed:
#!/bin/bash
# Check what compute_diff is imported from (line 6)
sed -n '1,15p' magicblock-committor-service/src/tasks/commit_task.rs | grep -A2 -B2 "compute_diff"Length of output: 174
🏁 Script executed:
#!/bin/bash
# Look at the imports section of the file
head -20 magicblock-committor-service/src/tasks/commit_task.rsLength of output: 527
🏁 Script executed:
#!/bin/bash
# Verify that .compute_diff() method (with self) is never called anywhere
rg -n "\.compute_diff\(" magicblock-committor-service/src/ --type rustLength of output: 0
🏁 Script executed:
#!/bin/bash
# Double check: are there any call sites that invoke the method on self or &self
rg -n "self\.compute_diff\|self_ref\.compute_diff" magicblock-committor-service/src/ --type rustLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's any dynamic or indirect invocation through method references
rg -n "compute_diff" magicblock-committor-service/src/tasks/commit_task.rs | head -20Length of output: 125
🏁 Script executed:
#!/bin/bash
# Verify if CommitTask::compute_diff could be called from outside this file
rg -n "CommitTask::compute_diff" magicblock-committor-service/src/ --type rustLength of output: 0
Remove CommitTask::compute_diff and unused use core::panic; import
Verification confirms CommitTask::compute_diff is never called anywhere in the codebase. The function call at line 181 invokes the imported dlp::compute_diff (a two-argument function), not the method stub.
Having a public method that unconditionally panics is a footgun for future refactors. Remove:
- Lines 153–156: the method definition (and
// FIXME: DELETEcomment) - Line 1:
use core::panic;(unused;panic!()is a built-in macro)
🤖 Prompt for AI Agents
In magicblock-committor-service/src/tasks/commit_task.rs around lines 153–156
and line 1, remove the unused panic import and the dead public method: delete
the top-level `use core::panic;` import (line 1) and remove the `// FIXME:
DELETE` comment and the entire `pub fn compute_diff(&self) ->
Option<dlp::rkyv::AlignedVec> { panic!() }` method definition (lines 153–156),
since the codebase uses the free function `dlp::compute_diff` and the stub
panics unnecessarily.
| pub fn try_optimize_tx_size(self) -> Result<Task, Task> { | ||
| // TODO (snawaz): do two things: | ||
| // 1. this does not properly handle preparation state as both ArgsTask | ||
| // and CommitTask have this. Only CommitTask needs to have this. | ||
| // 3. Remove PreparationState. | ||
| // 4. Instead have enum LifecycleTask { PreparationTask, CleanupTask } or struct (ee | ||
| // [2]. | ||
| // 5. NotNeeded is not needed (pun not intended). Instead use Option<LifecycleTask> | ||
| // | ||
| // ref: | ||
| // 1: https://chatgpt.com/s/t_691e1c39f47081919efcc73a2f599cf9 | ||
| // 2: https://chatgpt.com/s/t_691e1d7e82a08191963b43c6c8ad7a96 | ||
| match self { | ||
| Task::Commit(value) => value | ||
| .try_optimize_tx_size() | ||
| .map(Task::Commit) | ||
| .map_err(Task::Commit), | ||
| Task::BaseAction(_) | Task::Finalize(_) | Task::Undelegate(_) => { | ||
| Err(self) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 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::Commitis ever optimized, and all other variants must returnErr(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.
| /// 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(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 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 -100Length 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=3Length 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 -80Length 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=2Length 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.rsLength 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 -150Length 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 -100Length 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.rsLength 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 -20Length of output: 1085
🏁 Script executed:
#!/bin/bash
# Check all usages of Task enum
rg -nP '\bTask::' magicblock-committor-service/src/ --context=2Length of output: 14971
🏁 Script executed:
#!/bin/bash
# Check imports to understand what is being used where
head -50 magicblock-committor-service/src/tasks/task.rsLength of output: 1696
🏁 Script executed:
#!/bin/bash
# Full scan for preparation related code
rg -nP 'preparation' magicblock-committor-service/src/tasks/ -C 5Length 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.rsLength 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 2Length 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.
| /// 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.
| pub fn switch_preparation_state( | ||
| &mut self, | ||
| new_state: PreparationState, | ||
| ) -> TaskResult<()> { | ||
| if !matches!(new_state, PreparationState::NotNeeded) { | ||
| Err(TaskError::PreparationStateTransitionError) | ||
| } else { | ||
| // Do nothing | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
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 exceptNotNeeded. - The underlying
CommitTask(and anyBufferLifecycle) never see the new state.
Please either:
- Delegate per variant (e.g.,
Task::Commit(commit) => commit.switch_preparation_state(new_state)and returnOk(())/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.
| pub fn compute_units(&self) -> u32 { | ||
| match &self { | ||
| Task::Commit(_) => 70_000, | ||
| Task::BaseAction(task) => task.action.compute_units, | ||
| Task::Undelegate(_) => 70_000, | ||
| Task::Finalize(_) => 70_000, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
compute_units duplicates per-variant constants; prefer delegating to subtasks
For Commit, Finalize, and Undelegate you hard‑code 70_000, while BaseAction reads from action.compute_units. This works, but it risks drifting from the underlying implementations (e.g., if CommitTask’s cost model changes, or if buffer‑based strategies are refined).
To avoid that divergence, consider delegating to the subtasks instead:
- match &self {
- Task::Commit(_) => 70_000,
- Task::BaseAction(task) => task.action.compute_units,
- Task::Undelegate(_) => 70_000,
- Task::Finalize(_) => 70_000,
- }
+ match &self {
+ Task::Commit(task) => task.compute_units(),
+ Task::BaseAction(task) => task.action.compute_units,
+ Task::Undelegate(task) => task.compute_units(),
+ Task::Finalize(task) => task.compute_units(),
+ }(Assuming the subtasks already expose appropriate methods; otherwise it may be worth adding them there and keeping the constants in one place.)
This keeps TransactionUtils::tasks_compute_units aligned with the true per‑variant budget.
I'll verify the review comment by examining the task structure and whether subtasks expose the necessary methods.
🏁 Script executed:
#!/bin/bash
# Find and examine the Task enum definition
rg -n "enum Task" --type rust -A 20 magicblock-committor-service/src/tasks/task.rsLength of output: 739
🏁 Script executed:
#!/bin/bash
# Check for compute_units methods in CommitTask
rg -n "impl.*CommitTask\|compute_units" --type rust -B 2 -A 5 magicblock-committor-service/src/tasks/ | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for UndeleteTask and FinalizeTask definitions and their methods
rg -n "struct (UndeleteTask|FinalizeTask|CommitTask)" --type rust -A 10 magicblock-committor-service/src/tasks/Length of output: 2880
🏁 Script executed:
#!/bin/bash
# Search for compute_units method implementations across task files
fd --type rust --path "*/tasks/*" -x grep -l "compute_units" {} \;Length of output: 233
🏁 Script executed:
#!/bin/bash
# Examine the files that contain compute_units methods
rg -n "fn compute_units" --type rust magicblock-committor-service/src/tasks/ -B 2 -A 3Length of output: 990
🏁 Script executed:
#!/bin/bash
# Check Task enum variant structure to see what types are wrapped
rg -n "Task::" --type rust magicblock-committor-service/src/tasks/task.rs | head -20Length of output: 1115
🏁 Script executed:
#!/bin/bash
# Look at the complete current Task enum definition
cat magicblock-committor-service/src/tasks/task.rs | grep -A 30 "^pub enum Task"Length of output: 1073
🏁 Script executed:
#!/bin/bash
# Search for CommitTask, UndeleteTask, FinalizeTask structs
rg -n "pub struct (CommitTask|UndeleteTask|FinalizeTask)" --type rust magicblock-committor-service/src/tasks/ -A 15Length of output: 4166
🏁 Script executed:
#!/bin/bash
# Check if BaseAction has compute_units field
rg -n "compute_units" --type rust magicblock-committor-service/src/ -B 3 -A 1 | grep -A 4 -B 4 "BaseAction\|action.compute"Length of output: 728
🏁 Script executed:
#!/bin/bash
# Get complete picture of all task files
fd --type rust --path "*/tasks/*" magicblock-committor-service/src/tasks/Length of output: 233
🏁 Script executed:
#!/bin/bash
# Look at buffer_task.rs to understand compute_units pattern
rg -n "compute_units" --type rust magicblock-committor-service/src/tasks/buffer_task.rs -B 2 -A 2Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check commit_task.rs for compute_units
rg -n "compute_units\|struct CommitTask" --type rust magicblock-committor-service/src/tasks/commit_task.rs -B 2 -A 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check undelegate_task.rs
rg -n "compute_units\|struct UndeleteTask" --type rust magicblock-committor-service/src/tasks/undelegate_task.rs -B 2 -A 3Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check finalize_task.rs
rg -n "compute_units\|struct FinalizeTask" --type rust magicblock-committor-service/src/tasks/finalize_task.rs -B 2 -A 3Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check if there's an action field or similar in the task structs
rg -n "pub action\|pub struct" --type rust magicblock-committor-service/src/tasks/base_action_task.rs -A 5Length of output: 98
🏁 Script executed:
#!/bin/bash
# Look for all task type files
ls -la magicblock-committor-service/src/tasks/*.rs | head -20Length of output: 928
🏁 Script executed:
#!/bin/bash
# Check the complete Task enum definition in detail
sed -n '/^pub enum Task/,/^}/p' magicblock-committor-service/src/tasks/task.rsLength of output: 158
🏁 Script executed:
#!/bin/bash
# Verify if tasks wrap the actual task structs or just unit types
rg -n "Task::Commit\|Task::Finalize\|Task::Undelegate\|Task::BaseAction" --type rust magicblock-committor-service/src/tasks/task.rs -B 1 -A 1Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check what Undelegate wraps (it might be different from UndeleteTask)
rg -n "Undelegate" --type rust magicblock-committor-service/src/tasks/ -A 2 -B 2Length of output: 9494
🏁 Script executed:
#!/bin/bash
# Look at the full task.rs file around the compute_units method
sed -n '100,130p' magicblock-committor-service/src/tasks/task.rsLength of output: 938
🏁 Script executed:
#!/bin/bash
# Check all files in tasks directory to understand structure
ls magicblock-committor-service/src/tasks/Length of output: 141
🏁 Script executed:
#!/bin/bash
# Examine the actual enum definition
cat magicblock-committor-service/src/tasks/task.rs | head -150 | tail -80Length of output: 2744
🏁 Script executed:
#!/bin/bash
# Get exact structure around line 111-118
sed -n '105,125p' magicblock-committor-service/src/tasks/task.rsLength of output: 636
🏁 Script executed:
#!/bin/bash
# Find the enum Task definition early in the file
head -100 magicblock-committor-service/src/tasks/task.rs | grep -A 20 "enum Task"Length of output: 676
🏁 Script executed:
#!/bin/bash
# Check what the Task enum looks like with proper context
awk '/enum Task/,/^}/' magicblock-committor-service/src/tasks/task.rs | head -30Length of output: 158
🏁 Script executed:
#!/bin/bash
# Look at imports to understand task types
head -40 magicblock-committor-service/src/tasks/task.rsLength of output: 1330
🏁 Script executed:
#!/bin/bash
# Check if each task type has compute_units() by searching dedicated files
for file in commit_task undelegate_task finalize_task base_action_task; do
echo "=== $file.rs ===";
rg -n "fn compute_units\|struct" magicblock-committor-service/src/tasks/${file}.rs | head -10;
doneLength of output: 395
🏁 Script executed:
#!/bin/bash
# Get complete enum definition using sed
sed -n '1,200p' magicblock-committor-service/src/tasks/task.rs | grep -A 15 "^pub enum Task"Length of output: 510
🏁 Script executed:
#!/bin/bash
# Simpler approach - dump the whole file and look for enum
cat magicblock-committor-service/src/tasks/task.rs | head -200Length of output: 5368
🏁 Script executed:
#!/bin/bash
# List all rust files in tasks directory with line counts
wc -l magicblock-committor-service/src/tasks/*.rsLength of output: 544
🏁 Script executed:
#!/bin/bash
# Print the entire task.rs file to see the full context
cat magicblock-committor-service/src/tasks/task.rsLength of output: 5368
🏁 Script executed:
#!/bin/bash
# Look for any references to see if the compute_units values are correct
rg "70_000\|compute_units" --type rust magicblock-committor-service/src/tasks/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if CommitTask, UndeleteTask, FinalizeTask are mentioned anywhere as having compute_units
rg "CommitTask::compute_units\|UndeleteTask::compute_units\|FinalizeTask::compute_units" --type rust magicblock-committor-service/Length of output: 0
🏁 Script executed:
#!/bin/bash
# See if these task types exist and what they contain
rg "pub struct CommitTask|pub struct UndeleteTask|pub struct FinalizeTask" --type rust -A 8Length of output: 2141
🏁 Script executed:
#!/bin/bash
# Check the impl blocks for each task type
rg "impl (CommitTask|UndeleteTask|FinalizeTask)" --type rust -A 15Length of output: 3102
🏁 Script executed:
#!/bin/bash
# Examine what the Task enum wraps
grep -n "^pub enum Task\|^ [A-Z]" magicblock-committor-service/src/tasks/task.rs | head -15Length of output: 244
🏁 Script executed:
#!/bin/bash
# Try using ast-grep to find the enum definition
ast-grep --pattern 'enum Task {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Try to find the exact structure of Task enum variants
rg -n "^\s+(Commit|Finalize|Undelegate|BaseAction)" --type rust magicblock-committor-service/src/tasks/task.rs -B 2 -A 2Length of output: 207
🏁 Script executed:
#!/bin/bash
# Get the full context of task.rs file structure
head -150 magicblock-committor-service/src/tasks/task.rs | cat -nLength of output: 6222
🏁 Script executed:
#!/bin/bash
# Check if Undelegate wraps an UndeleteTask or something else
rg "Undelegate\(" --type rust magicblock-committor-service/src/ -B 3 -A 3Length of output: 10764
🏁 Script executed:
#!/bin/bash
# Look at all task files to understand the module structure
ls -la magicblock-committor-service/src/tasks/ && echo "---" && head -50 magicblock-committor-service/src/tasks/mod.rsLength of output: 1754
🏁 Script executed:
#!/bin/bash
# Check what each Task variant contains by looking at usages
rg "Task::(Commit|Finalize|Undelegate|BaseAction)\(" --type rust magicblock-committor-service/src/ -B 1 -A 1Length of output: 9668
🏁 Script executed:
#!/bin/bash
# Final attempt: get the raw Task enum from the actual PR diff or file
cat magicblock-committor-service/src/tasks/task.rs | sed -n '1,150p'Length of output: 5172
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if CommitTask has an impl block that could house compute_units
rg -n "^impl CommitTask" magicblock-committor-service/src/tasks/commit_task.rs -A 100 | head -80
Length of output: 3177
🏁 Script executed:
#!/bin/bash
# Check if FinalizeTask or UndelegateTask have any impl blocks
rg -n "^impl (FinalizeTask|UndelegateTask)" magicblock-committor-service/src/tasks/ -A 20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the complete mod.rs to see if FinalizeTask or UndelegateTask have impl blocks there
cat magicblock-committor-service/src/tasks/mod.rs | grep -A 50 "pub struct FinalizeTask"Length of output: 1363
🏁 Script executed:
#!/bin/bash
# Search for any impl of FinalizeTask anywhere in the codebase
rg "impl.*FinalizeTask" magicblock-committor-service/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any impl of UndelegateTask anywhere in the codebase
rg "impl.*UndelegateTask" magicblock-committor-service/Length of output: 0
Centralize compute budget constants by adding compute_units() methods to subtasks
The current approach works but hardcodes 70_000 in three match arms. To avoid drift between Task and subtask implementations, consider delegating to methods on each task type:
- Add
compute_units()toCommitTask's impl block (commit_task.rs) - Create impl blocks for
FinalizeTaskandUndelegateTask(currently data-only structs in mod.rs) and addcompute_units()to each
Then update Task::compute_units() to call these methods instead of hardcoding values. This keeps the per-variant budget centralized in each task's own implementation.
🤖 Prompt for AI Agents
magicblock-committor-service/src/tasks/task.rs lines 111-118: the
Task::compute_units() method hardcodes 70_000 for Commit, Undelegate and
Finalize variants; add a compute_units(&self) -> u32 method to CommitTask
(commit_task.rs) and create impl blocks for FinalizeTask and UndelegateTask
(where those structs are defined, e.g., mod.rs) returning their respective
budgets, then change Task::compute_units() to call the per-variant methods
(e.g., task.compute_units()) instead of using the literal 70_000 so each task
owns its compute budget.
| #[error("TaskError: {0}")] | ||
| TaskError(#[from] TaskError), | ||
| } |
There was a problem hiding this comment.
🧹 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
TaskErrorexplicitly at the point where you map it intoInternalErroror where you handleFailedToPrepareBufferAccounts, or - Enriching
TaskErrorwith 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.
3beda6e to
a01308a
Compare
snawaz
force-pushed
the
snawaz/commit-task
branch
from
cb6b8c8 to
1fafa4f
Compare
snawaz
force-pushed
the
snawaz/diff-stragegy
branch
from
a01308a to
d31f0a8
Compare
snawaz
force-pushed
the
snawaz/commit-task
branch
2 times, most recently
from
5f99250 to
b9e5dab
Compare
snawaz
force-pushed
the
snawaz/diff-stragegy
branch
from
d31f0a8 to
bac80f0
Compare
snawaz
force-pushed
the
snawaz/commit-task
branch
from
b9e5dab to
e0f71f3
Compare
snawaz
force-pushed
the
snawaz/diff-stragegy
branch
from
bac80f0 to
4d93f4e
Compare
snawaz
force-pushed
the
snawaz/diff-stragegy
branch
from
4d93f4e to
e17f99d
Compare
snawaz
force-pushed
the
snawaz/commit-task
branch
from
e0f71f3 to
d34fac7
Compare
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):CommitTasknow 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, thestrategyowns theBufferLifecyclewhen applicable.With this redesign, we no longer need the layers of indirection such as
BaseTask,Box<dyn BaseTask>,ArgsTask,ArgsTaskType,BufferTask,BufferTaskType, etcIn short, we don’t need
dyn BaseTaskbecause our tasks are a closed set. All task types are known upfront, so an enum works best — simpler, clearer, and cheaper.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.