refactor(committor): add IntentClient, move common executor logic in utils#1078
Merged
refactor(committor): add IntentClient, move common executor logic in utils#1078
Conversation
# 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/intent_executor/utils.rs`:
- Around line 148-171: The current code preserves ALT usage for both commit and
finalize stages based only on the original strategy.lookup_tables_keys and
TaskStrategist::collect_lookup_table_keys, which can force ALTs even when each
split stage could fit without them; instead, rebuild a per-stage execution
strategy using TaskStrategist::build_execution_strategy (or an equivalent "fit
without ALTs first" path) for commit_stage_tasks and finalize_stage_tasks to
determine whether ALTs are actually needed for each stage, and then construct
the commit_strategy and finalize_strategy TransactionStrategy objects from those
per-stage strategies rather than reusing the original
strategy.lookup_tables_keys or only re-collecting keys.
- Around line 90-96: The reset() call on TaskInfoFetcher
(CacheTaskInfoFetcher::reset with ResetType::Specific) does not invalidate
in-flight nonce guards, so fetch_next_commit_nonces(...) can still treat stale
guards as cache hits; update this recovery path to either (A) call a
cache-bypassing current-nonce fetch for the committed_pubkeys (e.g., use the
existing "bypass cache" variant of current-nonce lookup) and pass those nonces
into commit build, or (B) add a step on TaskInfoFetcher to invalidate
live/in-flight guards (e.g., an invalidate_live_guards(committed_pubkeys) or a
stronger reset_including_guards API) before calling
fetch_next_commit_nonces(...), then re-run fetch_next_commit_nonces to ensure
fresh on-chain nonces are used; adjust TaskInfoFetcher (and
CacheTaskInfoFetcher) accordingly and reference ResetType::Specific, reset, and
fetch_next_commit_nonces in the change.
In `@magicblock-committor-service/src/tasks/task_strategist.rs`:
- Around line 32-42: The regression is missing tests ensuring ALT cleanup logic
in remove_actions() relies on dummy_revaluate_alts() returning only lookup-table
keys that became unused after action stripping, and that handle_actions_result()
disposes the returned strategy immediately; add unit tests exercising
TaskStrategist::remove_actions() (and/or dummy_revaluate_alts()) for (1) a "no
BaseAction tasks" scenario and (2) an ALT-backed scenario where actions are
removed and only the newly-unused keys from
TaskStrategist::collect_lookup_table_keys(self.lookup_tables_keys,
&self.optimized_tasks) are released—assert that unused_alts exactly equals the
set difference between the pre- and post-partition lookup table keys, and assert
handle_actions_result() receives/disposes the returned strategy as expected
(mock or inspect the returned value) so removing actions only releases
newly-unused keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a88d6b2-e52f-4c82-93b4-e49f3f846bd4
📒 Files selected for processing (2)
magicblock-committor-service/src/intent_executor/utils.rsmagicblock-committor-service/src/tasks/task_strategist.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (1)
176-187:⚠️ Potential issue | 🟠 MajorDon't strip actions in the terminal callback path.
execute_callbacks()still leavestransaction_strategyavailable throughconsume_strategy(). On anErr, routing throughhandle_actions_result()hands the pre-striplookup_tables_keystoexecution_report.dispose(...), and later cleanup of the consumed strategy releases the retained lookup-table refs again. Keepremove_actions()for retry patching only; the terminal callback path should just extract callbacks from the current strategy.Proposed fix
pub fn execute_callbacks( &mut self, result: Result<(), impl Into<ActionError>>, ) { - let junk_strategy = - self.handle_actions_result(result.map_err(|err| err.into())); - self.execution_report.dispose(junk_strategy); + let callbacks = self.transaction_strategy.extract_action_callbacks(); + if !callbacks.is_empty() { + let callbacks_results = self + .callback_scheduler + .schedule(callbacks, result.map_err(|err| err.into())); + self.execution_report.add_callback_report(callbacks_results); + } }magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
159-165:⚠️ Potential issue | 🟠 MajorSeparate retry cleanup from terminal callback execution.
commit()andfinalize()now callexecute_callbacks()and then immediately dispose the whole stage. On anErr,execute_callbacks()currently goes throughhandle_actions_result(), so the pre-striplookup_tables_keysare disposed first and the retained stage keys are disposed again right after.remove_actions()is fine for retry patching, but these terminal callback paths should only extract callbacks and leave the strategy intact for the final dispose.Proposed fix
pub fn execute_callbacks( &mut self, result: Result<(), impl Into<ActionError>>, ) { let result = result.map(|_| ()).map_err(|err| err.into()); - let junk_strategy = handle_actions_result( - &self.authority.pubkey(), - &self.callback_scheduler, - self.execution_report, - &mut self.state.commit_strategy, - result.clone(), - ); - self.execution_report.dispose(junk_strategy); + let callbacks = self.state.commit_strategy.extract_action_callbacks(); + if !callbacks.is_empty() { + let report = + self.callback_scheduler.schedule(callbacks, result.clone()); + self.execution_report.add_callback_report(report); + } if result.is_err() { - let junk_strategy = handle_actions_result( - &self.authority.pubkey(), - &self.callback_scheduler, - self.execution_report, - &mut self.state.finalize_strategy, - result, - ); - self.execution_report.dispose(junk_strategy); + let callbacks = self.state.finalize_strategy.extract_action_callbacks(); + if !callbacks.is_empty() { + let report = self.callback_scheduler.schedule(callbacks, result); + self.execution_report.add_callback_report(report); + } } } @@ pub fn execute_callbacks( &mut self, result: Result<(), impl Into<ActionError>>, ) { - let junk_strategy = handle_actions_result( - &self.authority.pubkey(), - &self.callback_scheduler, - self.execution_report, - &mut self.state.finalize_strategy, - result.map_err(|err| err.into()), - ); - self.execution_report.dispose(junk_strategy); + let result = result.map_err(|err| err.into()); + let callbacks = self.state.finalize_strategy.extract_action_callbacks(); + if !callbacks.is_empty() { + let report = self.callback_scheduler.schedule(callbacks, result); + self.execution_report.add_callback_report(report); + } }Also applies to: 301-324, 401-405, 419-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/intent_executor/two_stage_executor.rs` around lines 159 - 165, commit() and finalize() currently call execute_callbacks() which routes through handle_actions_result()/remove_actions(), causing pre-strip lookup_tables_keys to be removed before the final dispose and then disposed again; change the flow so terminal callback execution only extracts callbacks and does not mutate or remove stage keys, and only after callbacks run do you call execution_report.dispose(mem::take(&mut self.state.commit_strategy)) / ...finalize_strategy. Concretely: add a new method (or a boolean flag on execute_callbacks) that returns the list of terminal callbacks without invoking remove_actions/handle_actions_result mutation logic, have commit() and finalize() call that non-mutating extractor then call execution_report.dispose(mem::take(&mut self.state.commit_strategy)) (and the finalize_strategy dispose on Err) as the final step; leave remove_actions and handle_actions_result behavior unchanged for retry paths that need patching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@magicblock-committor-service/src/intent_executor/two_stage_executor.rs`:
- Around line 159-165: commit() and finalize() currently call
execute_callbacks() which routes through
handle_actions_result()/remove_actions(), causing pre-strip lookup_tables_keys
to be removed before the final dispose and then disposed again; change the flow
so terminal callback execution only extracts callbacks and does not mutate or
remove stage keys, and only after callbacks run do you call
execution_report.dispose(mem::take(&mut self.state.commit_strategy)) /
...finalize_strategy. Concretely: add a new method (or a boolean flag on
execute_callbacks) that returns the list of terminal callbacks without invoking
remove_actions/handle_actions_result mutation logic, have commit() and
finalize() call that non-mutating extractor then call
execution_report.dispose(mem::take(&mut self.state.commit_strategy)) (and the
finalize_strategy dispose on Err) as the final step; leave remove_actions and
handle_actions_result behavior unchanged for retry paths that need patching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9bc9868f-0f5f-4176-a6df-04bd009f5dc4
📒 Files selected for processing (3)
magicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/tasks/task_strategist.rs
Contributor
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 (2)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
33-64:⚠️ Potential issue | 🟠 MajorKeep the stage strategies reachable from
TwoStageExecutor.After this refactor the strategies live behind private state, and
TwoStageExecutordoes not expose equivalent accessors. That breaks the direct cleanup/error-handling access this standalone executor API is supposed to preserve.Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-committor-service/src/intent_executor/two_stage_executor.rs` around lines 33 - 64, The refactor hid commit_strategy/finalize_strategy inside the state structs (Initialized, Committed, Finalized) making TwoStageExecutor unusable for standalone cleanup/error handling; restore public accessors equivalent to the SingleStageExecutor API by exposing the strategies and report fields on TwoStageExecutor: add public fields or methods named transaction_strategy, junk, and patched_errors (or public getters returning &mut TransactionStrategy and &mut the report structs) that delegate to the internal commit_strategy and finalize_strategy inside Initialized/Committed and to the execution_report for junk/patched_errors; reference TwoStageExecutor, Initialized, Committed, Finalized, commit_strategy, finalize_strategy, transaction_strategy, junk, and patched_errors when updating the API so callers outside IntentExecutor can access them.magicblock-committor-service/src/intent_executor/single_stage_executor.rs (1)
179-182:⚠️ Potential issue | 🟠 MajorDon't short-circuit non-commit recovery here.
This guard returns before
ActionsErrorandUndelegationErrorreach their recovery branches. For single-stage strategies that legitimately have nocommitted_pubkeys, those recoverable cases become hard failures.🤖 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 179 - 182, The early return on committed_pubkeys.is_empty() short-circuits the recovery handling for ActionsError and UndelegationError; remove or relocate this guard so recoverable errors are processed first. Specifically, in single_stage_executor.rs (around the block using committed_pubkeys and returning ControlFlow::Break(())), either move the committed_pubkeys.is_empty() check to after the error-recovery branches or make the guard conditional (only return when no recoverable ActionsError/UndelegationError was encountered), ensuring ActionsError and UndelegationError recovery code runs before breaking out.
🤖 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/intent_executor/single_stage_executor.rs`:
- Around line 32-40: The executor must keep its strategy observable: make the
transaction_strategy field public on SingleStageExecutor (change
transaction_strategy: TransactionStrategy to pub transaction_strategy:
TransactionStrategy) so callers can inspect it without consuming the executor;
also ensure any related execution-report fields intended to be observable (e.g.,
junk, patched_errors if present) are declared pub as well and leave
consume_strategy() intact for the consume path. This preserves external
cleanup/error-handling access while keeping existing APIs like
consume_strategy() working.
In `@magicblock-committor-service/src/intent_executor/two_stage_executor.rs`:
- Around line 119-127: prepare_and_execute_strategy(...) is currently awaited
with the `?` operator which returns early on error and skips the later
`dispose(mem::take(...))` cleanup, potentially leaving TransactionStrategy in a
partial state (affects commit() and finalize()). Change each call (e.g.,
prepare_and_execute_strategy in commit()/finalize()) to capture the Result first
(let result = prepare_and_execute_strategy(...).await), map or convert the error
after cleanup, then always call dispose(mem::take(&mut
self.state.commit_strategy)) (and dispose of finalize_strategy where applicable)
before returning or propagating the error; apply the same pattern to the other
occurrences noted (around lines 158-163, 367-375, 401-402) to ensure disposal
runs regardless of success or failure.
In `@magicblock-committor-service/src/intent_executor/utils.rs`:
- Around line 237-262: The timeout branch in execute_with_timeout is causing
execute_callbacks to run twice; remove the early
executor.execute_callbacks(Err(ActionError::TimeoutError)) calls inside
execute_with_timeout (both the timeout match arm and the "already timed out"
branch) and instead propagate a suitable Err(ActionError::TimeoutError) (or
return immediately) so the caller (the code in mod.rs that invokes
execute_with_timeout and then calls execute_callbacks) is the single place that
triggers callbacks; update execute_with_timeout to only call
executor.execute(persister).await or return the timeout error without invoking
execute_callbacks directly.
---
Outside diff comments:
In `@magicblock-committor-service/src/intent_executor/single_stage_executor.rs`:
- Around line 179-182: The early return on committed_pubkeys.is_empty()
short-circuits the recovery handling for ActionsError and UndelegationError;
remove or relocate this guard so recoverable errors are processed first.
Specifically, in single_stage_executor.rs (around the block using
committed_pubkeys and returning ControlFlow::Break(())), either move the
committed_pubkeys.is_empty() check to after the error-recovery branches or make
the guard conditional (only return when no recoverable
ActionsError/UndelegationError was encountered), ensuring ActionsError and
UndelegationError recovery code runs before breaking out.
In `@magicblock-committor-service/src/intent_executor/two_stage_executor.rs`:
- Around line 33-64: The refactor hid commit_strategy/finalize_strategy inside
the state structs (Initialized, Committed, Finalized) making TwoStageExecutor
unusable for standalone cleanup/error handling; restore public accessors
equivalent to the SingleStageExecutor API by exposing the strategies and report
fields on TwoStageExecutor: add public fields or methods named
transaction_strategy, junk, and patched_errors (or public getters returning &mut
TransactionStrategy and &mut the report structs) that delegate to the internal
commit_strategy and finalize_strategy inside Initialized/Committed and to the
execution_report for junk/patched_errors; reference TwoStageExecutor,
Initialized, Committed, Finalized, commit_strategy, finalize_strategy,
transaction_strategy, junk, and patched_errors when updating the API so callers
outside IntentExecutor can access them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ceed9a7-7d62-467b-9342-8039fb89e0b5
📒 Files selected for processing (3)
magicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/utils.rs
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
Show resolved
Hide resolved
# Conflicts: # Cargo.lock # magicblock-committor-service/src/tasks/mod.rs # programs/magicblock/src/magicblock_processor.rs # test-integration/Cargo.lock # test-integration/programs/flexi-counter/src/instruction.rs # test-integration/programs/flexi-counter/src/processor.rs # test-integration/programs/schedulecommit/src/lib.rs # test-integration/test-ledger-restore/tests/07_commit_delegated_account.rs # test-integration/test-schedule-intent/tests/test_schedule_intents.rs
# Conflicts: # test-integration/test-committor-service/tests/test_intent_executor.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/intent_executor/mod.rs`:
- Around line 296-302: The public staged helpers currently force callers to pass
an IntentExecutionReport (e.g., single_stage_execution_flow and the two-stage
equivalent), which leaks that out-parameter into the exported API; restore a
report-less public wrapper and move the new signature behind a differently named
or narrower-visibility function (for example keep pub fn
single_stage_execution_flow(...) that constructs an IntentExecutionReport and
calls a new private or pub(crate) single_stage_execution_flow_with_report(...)
that accepts &mut IntentExecutionReport), or alternatively make the existing
report-taking functions non-pub and expose a public wrapper that does not
require the caller to thread the report; update both single_stage_execution_flow
and the two-stage helper accordingly.
In `@test-integration/test-committor-service/tests/test_intent_executor.rs`:
- Around line 173-182: Extract the duplicated direct-execution harness into a
small TestEnv helper (e.g., run_direct_execution_harness or
build_and_execute_intent) that builds IntentExecutionClient via
IntentExecutionClient::new(self.rpc_client.clone()), creates the
TransactionPreparator via self.create_transaction_preparator(), calls
prepare_and_execute_strategy with the provided transaction_strategy and
self.authority (or accept authority as argument), and returns the
execution_result; replace the repeated setup in tests with calls to this helper
to avoid duplication and drifting wiring.
- Around line 15-35: The file is missing the try_join_all import used later;
update the imports at the top where futures::future items are imported (the
block that currently imports join_all) to also import try_join_all so calls to
try_join_all at the sites in the tests (around the usages near the calls in
test_intent_executor.rs) resolve correctly; ensure the import includes
try_join_all from futures::future alongside join_all.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c98b237e-092b-407e-8090-b957f96b30a4
📒 Files selected for processing (3)
magicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/test-committor-service/tests/test_intent_executor.rs
# Conflicts: # magicblock-committor-service/src/intent_executor/mod.rs # magicblock-committor-service/src/intent_executor/single_stage_executor.rs # magicblock-committor-service/src/intent_executor/two_stage_executor.rs
# Conflicts: # magicblock-committor-service/src/intent_executor/mod.rs # magicblock-committor-service/src/intent_executor/single_stage_executor.rs # magicblock-committor-service/src/intent_executor/two_stage_executor.rs # magicblock-committor-service/src/tasks/task_strategist.rs # test-integration/test-committor-service/tests/test_intent_executor.rs
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
thlorenz
approved these changes
Apr 3, 2026
test-integration/test-committor-service/tests/test_intent_executor.rs
Outdated
Show resolved
Hide resolved
magicblock-committor-service/src/intent_executor/two_stage_executor.rs
Outdated
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactor of the intent executor internals: extracts shared logic, centralizes timeout handling, and cleans up the two-stage state machine. Scope is purely structural — no behavioral changes.
Changes
New:
IntentExecutionClientRPC execution and retry logic extracted from
mod.rsinto its own file (intent_execution_client.rs).mod.rsno longer owns raw RPC concerns.New:
utils.rsShared executor utilities extracted from
mod.rsandsingle_stage_executor.rs:prepare_and_execute_strategy— prepare + execute a single strategyhandle_commit_id_error/handle_cpi_limit_error/handle_undelegation_error— error recovery helpersexecute_with_timeout— centralizes the timeout + callback-firing pattern that was previously duplicated inline for every stageStageExecutortrait +SingleStage/CommitStage/FinalizeStagewrappers — uniform interface over the three execution phasesmod.rssingle_stage_execution_flowandtwo_stage_execution_flownow each callexecute_with_timeoutonce (or twice for two-stage) instead of duplicating theif has_callbacks { if let Some(time_left) { match timeout {...} } else {...} } else {...}block per stage.IntentExecutionReportis now local perexecute()call rather than held as mutable state on the executor between calls.TransactionStrategy::remove_actionsNew method on
TransactionStrategythat strips action tasks and returns them as a separate strategy for cleanup, used by both single-stage and two-stage error handlers.TwoStageExecutorCallback handling on commit failure now fires both commit and finalize callbacks (since commit failure means finalize won't run).
patch_commit_strategyandpatch_finalize_strategyextracted as public methods.Compatibility
Testing
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests