refactor: replace BaseTask trait objects with BaseTaskImpl enum#973
refactor: replace BaseTask trait objects with BaseTaskImpl enum#973
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThe PR removes ArgsTask/BufferTask and associated visitor modules, replacing trait-object task handling with a concrete enum-based representation 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (4)
magicblock-committor-service/src/transaction_preparator/mod.rs (1)
107-107:⚠️ Potential issue | 🟠 MajorPre-existing
.expect()in production code.While this line is not part of the current changeset, this
.expect("Possibility to assemble checked above")is in production code. The dry-run check above usesdummy_lookup_tableswhile this call uses reallookup_tables, so the assertion that assembly was "checked above" may not hold if the real lookup tables differ in ways that affect compilation. Consider replacing with proper error propagation. As per coding guidelines,{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.Proposed fix
- .expect("Possibility to assemble checked above") - .message; + .map_err(|_| { + crate::transaction_preparator::error::TransactionPreparatorError::from( + crate::tasks::task_strategist::TaskStrategistError::FailedToFitError + ) + })? + .message;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/transaction_preparator/mod.rs` at line 107, The call to .expect("Possibility to assemble checked above") must be replaced with proper error propagation because the dry-run used dummy_lookup_tables while this site uses the real lookup_tables; modify the enclosing function (the one performing assembly/validation) to return a Result and replace the .expect with either the ? operator or map_err to convert the assembly failure into an Err with a clear context message referencing lookup_tables, so callers can handle the failure instead of panicking. Ensure you update the function signature and call sites accordingly to propagate the error up the stack.magicblock-committor-service/src/tasks/mod.rs (1)
44-49:⚠️ Potential issue | 🟡 MinorRemove the
PreparationStateenum—it is unused dead code.Search confirms
PreparationStateis never referenced anywhere in the codebase, only defined. This enum can be safely removed frommagicblock-committor-service/src/tasks/mod.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/tasks/mod.rs` around lines 44 - 49, Remove the dead unused enum PreparationState and its variants (NotNeeded, Required(PreparationTask), Cleanup(CleanupTask)) from the tasks module; delete the entire enum definition in mod.rs and also remove any now-unused imports or re-exports that referenced PreparationState so the codebase has no leftover references or unused warnings. Ensure Compilation: run cargo build/test to confirm no remaining references to PreparationState remain.magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (1)
67-74:⚠️ Potential issue | 🔴 Critical
todo!()inLabelValue for BaseTaskImplwill panic at runtime when metrics are recorded.At line 70,
observe_committor_intent_task_preparation_time(&*task)is called, which invokes.value()on theBaseTaskImpl(magicblock-metrics/src/metrics/mod.rs:729). TheLabelValueimplementation forBaseTaskImpl(magicblock-committor-service/src/tasks/mod.rs:122–127) containstodo!(), causing a panic in production during task preparation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs` around lines 67 - 74, The LabelValue implementation for BaseTaskImpl is currently a todo!(), which will panic when observe_committor_intent_task_preparation_time(&*task) calls .value(); replace the todo!() in the impl LabelValue for BaseTaskImpl with a real implementation that returns a stable string label (e.g., using BaseTaskImpl fields like id, name, or variant) — locate the impl LabelValue block in tasks::BaseTaskImpl and implement value() to return the chosen label (and implement as_bytes() if required by the trait) so metrics recording no longer panics.magicblock-committor-service/src/intent_executor/single_stage_executor.rs (1)
199-228: 🧹 Nitpick | 🔵 TrivialNear-duplicate
handle_unfinalized_account_erroracross executors.This method is almost identical to
TwoStageExecutor::handle_unfinalized_account_error(lines 209–237 intwo_stage_executor.rs) — sameFinalizeTaskconstruction, sameprepare_and_execute_strategycall, same empty-strategy return. Only the error-variant wrappers differ (FailedFinalizePreparationError/FailedToFinalizeErrorhere vsFailedCommitPreparationError/FailedToCommitErrorthere).Consider extracting a shared helper on
IntentExecutorImplthat takes error-mapping closures or an enum, to avoid maintaining the same logic in two places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/intent_executor/single_stage_executor.rs` around lines 199 - 228, Extract the duplicated finalize-and-continue logic from handle_unfinalized_account_error into a shared helper method on IntentExecutorImpl (e.g., a method like finalize_delegated_account_and_continue) that constructs the FinalizeTask (using delegated_account from CommitTask), calls prepare_and_execute_strategy with the single-task TransactionStrategy and maps preparation/execute errors via parameters; have the helper accept either two mapping closures or a small enum to convert the preparation error and the execution error into the caller-specific IntentExecutorError variants (so TwoStageExecutor::handle_unfinalized_account_error and SingleStageExecutor::handle_unfinalized_account_error both call the new helper, passing closures that wrap errors into FailedCommitPreparationError/FailedToCommitError or FailedFinalizePreparationError/FailedToFinalizeError respectively), and return the same Ok(ControlFlow::Continue(empty TransactionStrategy)) on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 122-127: The LabelValue impl for BaseTaskImpl currently panics
because value() contains todo!(); update the value(&self) -> &str implementation
to return a safe label (either a placeholder like "unknown_task" or a real label
by matching on the BaseTaskImpl enum/variants) so it never panics at runtime;
locate the impl LabelValue for BaseTaskImpl and change value() to return a
static &str (or map each BaseTaskImpl variant to a distinct &'static str) so
observe_committor_intent_task_preparation_time (called from
delivery_preparator.rs) can call .value() safely.
In `@test-integration/test-committor-service/tests/test_delivery_preparator.rs`:
- Around line 112-123: The test's filter_map chain on strategy.optimized_tasks
extracts Commit tasks with a Cleanup stage into cleanup_tasks but then indexes
datas[i] assuming lengths match; add a defensive assertion before the loop to
ensure cleanup_tasks.len() == datas.len() (or explicitly handle the mismatch by
failing the test) so indexing cannot drift if some buffer tasks were filtered
out (refer to cleanup_tasks, strategy.optimized_tasks, BaseTaskImpl::Commit,
CommitBufferStage::Cleanup, datas, and prepare_for_delivery).
In
`@test-integration/test-committor-service/tests/test_transaction_preparator.rs`:
- Around line 95-117: The test creates a buffer_commit_task with
create_buffer_commit_task(&account2_data) but the corresponding FinalizeTask
uses committed_account2.pubkey, causing a pubkey mismatch; fix by making the
finalize task reference the same pubkey produced by the buffer commit task
(e.g., derive the finalize delegated_account from buffer_commit_task’s commit
pubkey or change create_buffer_commit_task to accept/return the expected pubkey)
so the FinalizeTask's delegated_account matches the commit task's pubkey.
---
Outside diff comments:
In `@magicblock-committor-service/src/intent_executor/single_stage_executor.rs`:
- Around line 199-228: Extract the duplicated finalize-and-continue logic from
handle_unfinalized_account_error into a shared helper method on
IntentExecutorImpl (e.g., a method like finalize_delegated_account_and_continue)
that constructs the FinalizeTask (using delegated_account from CommitTask),
calls prepare_and_execute_strategy with the single-task TransactionStrategy and
maps preparation/execute errors via parameters; have the helper accept either
two mapping closures or a small enum to convert the preparation error and the
execution error into the caller-specific IntentExecutorError variants (so
TwoStageExecutor::handle_unfinalized_account_error and
SingleStageExecutor::handle_unfinalized_account_error both call the new helper,
passing closures that wrap errors into
FailedCommitPreparationError/FailedToCommitError or
FailedFinalizePreparationError/FailedToFinalizeError respectively), and return
the same Ok(ControlFlow::Continue(empty TransactionStrategy)) on success.
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 44-49: Remove the dead unused enum PreparationState and its
variants (NotNeeded, Required(PreparationTask), Cleanup(CleanupTask)) from the
tasks module; delete the entire enum definition in mod.rs and also remove any
now-unused imports or re-exports that referenced PreparationState so the
codebase has no leftover references or unused warnings. Ensure Compilation: run
cargo build/test to confirm no remaining references to PreparationState remain.
In
`@magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs`:
- Around line 67-74: The LabelValue implementation for BaseTaskImpl is currently
a todo!(), which will panic when
observe_committor_intent_task_preparation_time(&*task) calls .value(); replace
the todo!() in the impl LabelValue for BaseTaskImpl with a real implementation
that returns a stable string label (e.g., using BaseTaskImpl fields like id,
name, or variant) — locate the impl LabelValue block in tasks::BaseTaskImpl and
implement value() to return the chosen label (and implement as_bytes() if
required by the trait) so metrics recording no longer panics.
In `@magicblock-committor-service/src/transaction_preparator/mod.rs`:
- Line 107: The call to .expect("Possibility to assemble checked above") must be
replaced with proper error propagation because the dry-run used
dummy_lookup_tables while this site uses the real lookup_tables; modify the
enclosing function (the one performing assembly/validation) to return a Result
and replace the .expect with either the ? operator or map_err to convert the
assembly failure into an Err with a clear context message referencing
lookup_tables, so callers can handle the failure instead of panicking. Ensure
you update the function signature and call sites accordingly to propagate the
error up the stack.
test-integration/test-committor-service/tests/test_delivery_preparator.rs
Show resolved
Hide resolved
test-integration/test-committor-service/tests/test_transaction_preparator.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-committor-service/src/tasks/mod.rs (1)
264-284:⚠️ Potential issue | 🟠 MajorProduction
.unwrap()onborsh::object_length— requires proper error handling.Line 271 uses
.unwrap()in production code. Even though the safety comment explains why it shouldn't fail in practice, the coding guidelines require proper error handling or explicit invariant justification for.unwrap()/.expect()in production code undermagicblock-*/**.Consider returning a
Resultfrominit_instructionor using.expect()with the invariant explanation inlined, though the preferred approach is propagating the error.Proposed fix — propagate the error
If changing the signature is feasible:
- pub fn init_instruction(&self, authority: &Pubkey) -> Instruction { + pub fn init_instruction(&self, authority: &Pubkey) -> Result<Instruction, std::io::Error> { let chunks_account_size = - borsh::object_length(&self.chunks).unwrap() as u64; + borsh::object_length(&self.chunks)? as u64; let buffer_account_size = self.committed_data.len() as u64; let (instruction, _, _) = create_init_ix(CreateInitIxArgs { ... }); - instruction + Ok(instruction) }As per coding guidelines,
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/tasks/mod.rs` around lines 264 - 284, The call to borsh::object_length(...).unwrap() in init_instruction must not unwrap in production; change init_instruction to return Result<Instruction, E> (or a crate-specific error type) and propagate the borsh error instead of panicking: call borsh::object_length(&self.chunks).map_err(|e| /* map to your error */)? to obtain chunks_account_size, keep buffer_account_size and CreateInitIxArgs usage, and return Ok(instruction) on success; update any callers of init_instruction to handle the Result accordingly (refer to init_instruction, borsh::object_length, and CreateInitIxArgs to locate the change).test-integration/test-committor-service/tests/test_transaction_preparator.rs (1)
149-169: 🧹 Nitpick | 🔵 TrivialDuplicated buffer-verification loop — consider extracting a helper.
The pattern of iterating
optimized_tasks, extractingCommitTaskvia pattern matching, checking forCommitBufferStage::Cleanup, and verifying chunks completeness is repeated verbatim in bothtest_prepare_commit_tx_with_multiple_accounts(lines 149–169) andtest_prepare_commit_tx_with_base_actions(lines 242–263). A shared helper (e.g.,assert_all_buffer_chunks_complete(tasks, fixture)) would reduce duplication and improve maintainability.Also applies to: 242-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-integration/test-committor-service/tests/test_transaction_preparator.rs` around lines 149 - 169, Extract the repeated buffer-verification loop into a shared test helper (e.g., assert_all_buffer_chunks_complete) that takes the task list and fixture; inside the helper iterate over optimized_tasks, match Commit tasks with BaseTaskImpl::Commit, check for CommitBufferStage::Cleanup, compute chunks_pda via cleanup_task.chunks_pda(&fixture.authority.pubkey()), fetch the account using fixture.rpc_client.get_account, deserialize with Chunks::try_from_slice and assert chunks.is_complete(); replace the duplicated blocks in test_prepare_commit_tx_with_multiple_accounts and test_prepare_commit_tx_with_base_actions with a call to this helper to remove duplication and centralize the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-committor-service/src/tasks/commit_task.rs`:
- Around line 275-296: Extract the hardcoded compute unit value 70_000 into a
shared constant (for example DEFAULT_TASK_COMPUTE_UNITS: u32 = 70_000) and
replace literals with that constant; update the compute_units() method in
CommitTask (the function named compute_units) to return
DEFAULT_TASK_COMPUTE_UNITS and also replace the same 70_000 usages in mod.rs for
the Finalize and Undelegate branches so all places reference the single constant
to avoid magic numbers and keep budgets consistent.
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 82-89: Extract the repeated literal 70_000 into a single shared
constant (e.g., const DEFAULT_COMPUTE_UNITS: u32 = 70_000) and use that constant
from all places that currently return 70_000 (the compute_units method on the
enum in tasks::mod for the Finalize and Undelegate arms and
CommitTask::compute_units in commit_task.rs) so they remain in sync; add the
constant in a common module (like tasks mod or a small constants module) with
appropriate visibility and replace the numeric literals with that constant in
both implementations.
---
Outside diff comments:
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 264-284: The call to borsh::object_length(...).unwrap() in
init_instruction must not unwrap in production; change init_instruction to
return Result<Instruction, E> (or a crate-specific error type) and propagate the
borsh error instead of panicking: call
borsh::object_length(&self.chunks).map_err(|e| /* map to your error */)? to
obtain chunks_account_size, keep buffer_account_size and CreateInitIxArgs usage,
and return Ok(instruction) on success; update any callers of init_instruction to
handle the Result accordingly (refer to init_instruction, borsh::object_length,
and CreateInitIxArgs to locate the change).
In
`@test-integration/test-committor-service/tests/test_transaction_preparator.rs`:
- Around line 149-169: Extract the repeated buffer-verification loop into a
shared test helper (e.g., assert_all_buffer_chunks_complete) that takes the task
list and fixture; inside the helper iterate over optimized_tasks, match Commit
tasks with BaseTaskImpl::Commit, check for CommitBufferStage::Cleanup, compute
chunks_pda via cleanup_task.chunks_pda(&fixture.authority.pubkey()), fetch the
account using fixture.rpc_client.get_account, deserialize with
Chunks::try_from_slice and assert chunks.is_complete(); replace the duplicated
blocks in test_prepare_commit_tx_with_multiple_accounts and
test_prepare_commit_tx_with_base_actions with a call to this helper to remove
duplication and centralize the logic.
| fn compute_units(&self) -> u32 { | ||
| 70_000 | ||
| } | ||
|
|
||
| fn accounts_size_budget(&self) -> u32 { | ||
| match &self.delivery_details { | ||
| CommitDelivery::StateInArgs => { | ||
| commit_size_budget(AccountSizeClass::Dynamic( | ||
| self.committed_account.account.data.len() as u32, | ||
| )) | ||
| } | ||
| CommitDelivery::StateInBuffer { .. } | ||
| | CommitDelivery::DiffInBuffer { .. } => { | ||
| commit_size_budget(AccountSizeClass::Huge) | ||
| } | ||
| CommitDelivery::DiffInArgs { .. } => { | ||
| commit_diff_size_budget(AccountSizeClass::Dynamic( | ||
| self.committed_account.account.data.len() as u32, | ||
| )) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the hardcoded compute unit budget.
compute_units() returns a hardcoded 70_000 (line 276). This same value appears in mod.rs (lines 86–87) for Finalize and Undelegate variants. Consider defining a shared constant (e.g., const DEFAULT_TASK_COMPUTE_UNITS: u32 = 70_000;) to avoid magic numbers and ensure consistency if the budget changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/src/tasks/commit_task.rs` around lines 275 -
296, Extract the hardcoded compute unit value 70_000 into a shared constant (for
example DEFAULT_TASK_COMPUTE_UNITS: u32 = 70_000) and replace literals with that
constant; update the compute_units() method in CommitTask (the function named
compute_units) to return DEFAULT_TASK_COMPUTE_UNITS and also replace the same
70_000 usages in mod.rs for the Finalize and Undelegate branches so all places
reference the single constant to avoid magic numbers and keep budgets
consistent.
| fn compute_units(&self) -> u32 { | ||
| match self { | ||
| Self::Commit(value) => value.compute_units(), | ||
| Self::BaseAction(value) => value.action.compute_units, | ||
| Self::Finalize(_) => 70_000, | ||
| Self::Undelegate(_) => 70_000, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Magic number 70_000 repeated for Finalize and Undelegate compute units.
Lines 86–87 use 70_000 directly, and CommitTask::compute_units() in commit_task.rs also returns 70_000. Consider extracting a shared constant to keep these in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/src/tasks/mod.rs` around lines 82 - 89, Extract
the repeated literal 70_000 into a single shared constant (e.g., const
DEFAULT_COMPUTE_UNITS: u32 = 70_000) and use that constant from all places that
currently return 70_000 (the compute_units method on the enum in tasks::mod for
the Finalize and Undelegate arms and CommitTask::compute_units in
commit_task.rs) so they remain in sync; add the constant in a common module
(like tasks mod or a small constants module) with appropriate visibility and
replace the numeric literals with that constant in both implementations.
# Conflicts: # magicblock-committor-service/src/tasks/args_task.rs # magicblock-committor-service/src/tasks/buffer_task.rs # magicblock-committor-service/src/tasks/mod.rs # magicblock-committor-service/src/tasks/task_builder.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
95-98: 🧹 Nitpick | 🔵 TrivialCloning both task vectors for the single-stage attempt has a non-trivial cost.
commit_tasksandfinalize_tasksare cloned on Line 98 to attempt single-stage assembly, with originals preserved for the two-stage fallback. SinceCommitTaskcontainsAccount(withVec<u8>data), this clone can be expensive for many large accounts. This is likely pre-existing behavior, but worth noting as a future optimization opportunity (e.g., a dry-run size check before cloning).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/tasks/task_strategist.rs` around lines 95 - 98, The current eager cloning of commit_tasks and finalize_tasks into single_stage_tasks is expensive because CommitTask contains Account with Vec<u8>; instead, implement a dry-run size check before cloning: compute the combined payload size by iterating commit_tasks and finalize_tasks and summing their Account data lengths (using CommitTask and Account accessors), compare against a conservative threshold, and only create single_stage_tasks (clone and concat) when the size is under that threshold; otherwise skip the clone and fall back to the existing two-stage path.magicblock-committor-service/src/tasks/mod.rs (1)
166-172: 🧹 Nitpick | 🔵 TrivialRemove the unused
CommitDiffTaskstruct.The struct is not referenced anywhere in the codebase (only its definition exists). Diff-based commits are now handled via
CommitDelivery::DiffInArgs { base_account }insideCommitTask, making this struct obsolete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/tasks/mod.rs` around lines 166 - 172, The CommitDiffTask struct is unused and obsolete; remove its definition (the pub struct CommitDiffTask { ... }) from the file so diff-based commits are handled via CommitDelivery::DiffInArgs within CommitTask; ensure there are no remaining references to CommitDiffTask elsewhere and run a build to confirm the compiler reports no uses of CommitDiffTask.magicblock-committor-service/src/tasks/task_builder.rs (1)
196-216: 🧹 Nitpick | 🔵 TrivialVerify if commit_id=0 is accepted downstream or if contract guarantees should be explicit.
The
unwrap_or_elsefallback to0is intentional per the comment about retry logic. However, this defensive approach masks the actual guarantee:fetch_next_commit_idsshould return all requested pubkeys or error. The code does honor this contract (strict validation infetch_accountsensures Vec counts match), but relying on a0placeholder assumes downstream code either rejects it or handles it gracefully.Consider either:
- Adding an explicit assertion that all pubkeys are present in the result HashMap before proceeding, or
- Documenting whether
commit_id=0is valid in the on-chain instruction validator.Note: This uses
unwrap_or_else(safe), notunwrap/expect(unsafe).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/tasks/task_builder.rs` around lines 196 - 216, Add an explicit pre-check that every account.pubkey has a commit id in commit_ids before building tasks instead of silently falling back to 0: verify commit_ids contains all requested pubkeys (e.g. compare counts or iterate and check commit_ids.contains_key(&account.pubkey)), and if any are missing log the pubkey and return/propagate an error (or panic with a clear message) so downstream code never receives commit_id = 0; then remove or replace the current unwrap_or_else fallback in the mapping that produces Self::create_commit_task to reflect this guaranteed invariant (references: commit_ids, account.pubkey, Self::create_commit_task, fetch_accounts/fetch_next_commit_ids).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 266-288: The instruction method in BaseActionTaskV1 duplicates the
logic from account_metas_static and call_handler_args_static; replace the manual
account meta and CallHandlerArgs construction in BaseActionTaskV1::instruction
with calls to the existing helpers: use self.account_metas() to produce the
AccountMeta vector and self.call_handler_args() to produce the CallHandlerArgs,
then pass those results into dlp::instruction_builder::call_handler (same as
BaseActionTaskV2::instruction). Ensure types match (no extra cloning) and remove
the now-redundant manual mapping over action.account_metas_per_program and
manual CallHandlerArgs construction.
---
Outside diff comments:
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 166-172: The CommitDiffTask struct is unused and obsolete; remove
its definition (the pub struct CommitDiffTask { ... }) from the file so
diff-based commits are handled via CommitDelivery::DiffInArgs within CommitTask;
ensure there are no remaining references to CommitDiffTask elsewhere and run a
build to confirm the compiler reports no uses of CommitDiffTask.
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Around line 196-216: Add an explicit pre-check that every account.pubkey has a
commit id in commit_ids before building tasks instead of silently falling back
to 0: verify commit_ids contains all requested pubkeys (e.g. compare counts or
iterate and check commit_ids.contains_key(&account.pubkey)), and if any are
missing log the pubkey and return/propagate an error (or panic with a clear
message) so downstream code never receives commit_id = 0; then remove or replace
the current unwrap_or_else fallback in the mapping that produces
Self::create_commit_task to reflect this guaranteed invariant (references:
commit_ids, account.pubkey, Self::create_commit_task,
fetch_accounts/fetch_next_commit_ids).
In `@magicblock-committor-service/src/tasks/task_strategist.rs`:
- Around line 95-98: The current eager cloning of commit_tasks and
finalize_tasks into single_stage_tasks is expensive because CommitTask contains
Account with Vec<u8>; instead, implement a dry-run size check before cloning:
compute the combined payload size by iterating commit_tasks and finalize_tasks
and summing their Account data lengths (using CommitTask and Account accessors),
compare against a conservative threshold, and only create single_stage_tasks
(clone and concat) when the size is under that threshold; otherwise skip the
clone and fall back to the existing two-stage path.
---
Duplicate comments:
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 87-94: Extract the repeated magic number into a shared constant
(e.g., const DEFAULT_TASK_COMPUTE_UNITS: u32 = 70_000) and replace the literal
70_000 in the Task::compute_units() match arms for Finalize and Undelegate with
that constant; ensure the constant is defined in the same module as Task or a
common module so both Task::compute_units() and CommitTask::compute_units() can
reference it to stay in sync, and update CommitTask::compute_units() to return
the constant if it currently returns the literal.
| impl BaseActionTaskV1 { | ||
| pub fn instruction(&self, validator: &Pubkey) -> Instruction { | ||
| let action = &self.action; | ||
| let account_metas = action | ||
| .account_metas_per_program | ||
| .iter() | ||
| .map(|short_meta| AccountMeta { | ||
| pubkey: short_meta.pubkey, | ||
| is_writable: short_meta.is_writable, | ||
| is_signer: false, | ||
| }) | ||
| .collect(); | ||
| dlp::instruction_builder::call_handler( | ||
| *validator, | ||
| action.destination_program, | ||
| action.escrow_authority, | ||
| account_metas, | ||
| CallHandlerArgs { | ||
| data: action.data_per_program.data.clone(), | ||
| escrow_index: action.data_per_program.escrow_index, | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
BaseActionTaskV1::instruction duplicates logic already in account_metas() and call_handler_args().
Lines 269-277 manually build account metas and lines 283-286 manually build CallHandlerArgs, duplicating the logic from account_metas_static (Line 298) and call_handler_args_static (Line 310). Compare with BaseActionTaskV2::instruction (Line 337) which correctly calls self.account_metas() and self.call_handler_args().
Proposed fix: reuse existing helpers
pub fn instruction(&self, validator: &Pubkey) -> Instruction {
let action = &self.action;
- let account_metas = action
- .account_metas_per_program
- .iter()
- .map(|short_meta| AccountMeta {
- pubkey: short_meta.pubkey,
- is_writable: short_meta.is_writable,
- is_signer: false,
- })
- .collect();
dlp::instruction_builder::call_handler(
*validator,
action.destination_program,
action.escrow_authority,
- account_metas,
- CallHandlerArgs {
- data: action.data_per_program.data.clone(),
- escrow_index: action.data_per_program.escrow_index,
- },
+ self.account_metas(),
+ self.call_handler_args(),
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/src/tasks/mod.rs` around lines 266 - 288, The
instruction method in BaseActionTaskV1 duplicates the logic from
account_metas_static and call_handler_args_static; replace the manual account
meta and CallHandlerArgs construction in BaseActionTaskV1::instruction with
calls to the existing helpers: use self.account_metas() to produce the
AccountMeta vector and self.call_handler_args() to produce the CallHandlerArgs,
then pass those results into dlp::instruction_builder::call_handler (same as
BaseActionTaskV2::instruction). Ensure types match (no extra cloning) and remove
the now-redundant manual mapping over action.account_metas_per_program and
manual CallHandlerArgs construction.
There was a problem hiding this comment.
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/mod.rs (1)
159-165: 🧹 Nitpick | 🔵 Trivial
CommitDiffTaskis unused dead code — remove it.This struct is defined but has no references anywhere in the codebase. It lacks methods, trait implementations, and no code instantiates it. Remove it if it was superseded by the
CommitTask/CommitDeliveryconsolidation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/tasks/mod.rs` around lines 159 - 165, Remove the dead struct CommitDiffTask: delete the #[derive(Clone, Debug)] pub struct CommitDiffTask { ... } definition, ensure no other code references CommitDiffTask (search for symbol CommitDiffTask) and update/remove any imports or docs that mentioned it, and confirm compilation/tests pass given CommitTask/CommitDelivery are the intended replacements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 159-165: Remove the dead struct CommitDiffTask: delete the
#[derive(Clone, Debug)] pub struct CommitDiffTask { ... } definition, ensure no
other code references CommitDiffTask (search for symbol CommitDiffTask) and
update/remove any imports or docs that mentioned it, and confirm
compilation/tests pass given CommitTask/CommitDelivery are the intended
replacements.
---
Duplicate comments:
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 259-281: BaseActionTaskV1::instruction duplicates the construction
of account metas and CallHandlerArgs instead of reusing existing helpers;
replace the manual rebuild (the account_metas mapping and CallHandlerArgs
construction) with calls to self.account_metas() and self.call_handler_args(),
then pass those results into dlp::instruction_builder::call_handler (mirroring
BaseActionTaskV2::instruction) so the function uses the shared helper logic
rather than duplicated code.
- Around line 80-87: Replace the duplicated literal 70_000 with a single shared
constant so compute unit values stay in sync: define a constant (e.g.
COMPUTE_UNITS_FINALIZE_UNDELEGATE or COMPUTE_UNITS_DEFAULT) near the
Task/CommitTask definitions and use it in Task::compute_units() for the Finalize
and Undelegate arms and also ensure CommitTask::compute_units() returns the same
constant; update references to Finalize, Undelegate and
CommitTask::compute_units() to use that constant.
1 participant
Summary
Closing technical debt and simplifying tasks and intents moving forward.
This is refactoring PR that focused on just replacing
dyn BaseTaskin order to keep it small. Future improvements will be made in next iterations. Some BaseTask methods were removed from the trait as unnecessary with concrete type.Box<dyn BaseTask>with concreteBaseTaskImplenum across committor serviceArgsTask/BufferTaskinto unifiedCommitTaskwithCommitDeliveryenum for delivery strategy (StateInArgs, StateInBuffer, DiffInArgs, DiffInBuffer)PersistorVisitor,TaskVisitorUtils) in favor of direct enum matchingCommitTaskinto its own module withBaseTasktrait implementation and separated instruction buildersCommitStage->CommitBufferStage,CommitDeliveryDetails->CommitDeliveryreset_commit_idfromBaseTasktrait (CommitTask-specific), changetry_optimize_tx_sizeto&mut self->boolCompatibility
Testing
serialization_safety_testrewritten to useCommitTaskdirectlytest-committor-serviceupdated to use new typesChecklist
Summary by CodeRabbit
Refactor
New Features
Tests
Chores