Some optimizations for intent execution #711
Conversation
chore: remove unnecessary network requests
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughReplaces single-transaction unification with a strategy-driven execution flow by adding 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 |
![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 (1)
magicblock-committor-service/src/intent_execution_manager/intent_scheduler.rs (1)
728-775: Missing imports forCommitAndUndelegateandUndelegateType.The function uses
CommitAndUndelegateandUndelegateType::Standaloneat lines 763-766, but these types are not imported within the function'suseblock (lines 733-739) or at the module level.Add the missing imports:
#[cfg(test)] pub(crate) fn create_test_intent( id: u64, pubkeys: &[Pubkey], is_undelegate: bool, ) -> ScheduledBaseIntentWrapper { use magicblock_program::magic_scheduled_base_intent::{ - CommitType, CommittedAccount, MagicBaseIntent, + CommitAndUndelegate, CommitType, CommittedAccount, MagicBaseIntent, + UndelegateType, };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
magicblock-accounts/src/scheduled_commits_processor.rs(1 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(8 hunks)magicblock-committor-service/src/intent_execution_manager/intent_scheduler.rs(15 hunks)magicblock-committor-service/src/intent_executor/mod.rs(5 hunks)magicblock-committor-service/src/tasks/task_strategist.rs(3 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(3 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/intent_executor/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-committor-service/src/tasks/task_strategist.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.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/task_strategist.rsmagicblock-committor-service/src/intent_execution_manager/intent_scheduler.rsmagicblock-committor-service/src/intent_executor/mod.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
magicblock-committor-service/src/tasks/task_strategist.rs
📚 Learning: 2025-11-12T09:46:27.553Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Applied to files:
magicblock-committor-service/src/tasks/task_strategist.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-committor-service/src/intent_execution_manager/intent_scheduler.rs
🧬 Code graph analysis (2)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
test-integration/test-tools/src/conversions.rs (1)
err(8-8)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
magicblock-metrics/src/metrics/mod.rs (2)
observe_committor_intent_task_preparation_time(556-564)observe_committor_intent_alt_preparation_time(566-568)magicblock-committor-service/src/tasks/mod.rs (1)
write_instructions(187-204)
🔇 Additional comments (12)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
283-284: LGTM! Improved error logging for better observability.Adding the
intent_idto the error log makes it easier to trace and debug commit failures by correlating them with specific intents. The format is correct and consistent with other logging patterns in the file.magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
68-73: Timer handling change looks correct.The
_timerpattern relies onHistogramTimer::drop()to callstop_and_record()automatically when the timer goes out of scope. This is idiomatic for Prometheus histogram timers and ensures timing is recorded even if early returns occur.
400-402: Good optimization to short-circuit ALT preparation.Early return when no lookup table keys are needed avoids unnecessary
table_maniaoperations. This aligns well with the PR's goal of preferring execution modes without ALTs.test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
246-256: Test expectation correctly updated for new execution strategy.The change from expecting
FromBufferWithLookupTable+FromBufferto onlyFromBufferreflects the PR's optimization: preferring two-stage execution without ALTs over single-stage with ALTs, since ALT activation latency is significant.This aligns with the
build_execution_strategylogic that checksif !single_stage_strategy.uses_alts()before falling back to two-stage.magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
434-438: Test helper signature update applied consistently.All
create_test_intentcalls correctly passfalsefor the newis_undelegateparameter, maintaining existing test behavior. This is a mechanical update following the test helper signature change.magicblock-committor-service/src/intent_execution_manager/intent_scheduler.rs (1)
312-321: Test updates are consistent and maintain existing behavior.All test invocations correctly pass
falseforis_undelegate, preserving the original test semantics while enabling the new capability for undelegate scenario testing elsewhere.magicblock-committor-service/src/intent_executor/mod.rs (3)
187-202: Parallel task building is a good optimization.Using
jointo build commit and finalize tasks concurrently is efficient. The error handling with(commit_tasks?, finalize_tasks?)correctly propagates any errors from either future.
204-233: Clean execution strategy pattern matching.The branching on
StrategyExecutionModeis well-structured:
SingleStagepath reuses the existingsingle_stage_execution_flowTwoStagepath properly destructures commit and finalize stagesThe trace logging at lines 212 and 224 provides good observability for debugging execution mode selection.
163-184: Standalone actions path correctly uses simpler strategy builder.For intents without committed accounts (BaseActions), building only commit tasks and using
build_strategydirectly is correct since there's no finalize stage needed.magicblock-committor-service/src/tasks/task_strategist.rs (3)
44-68: Well-designed execution mode abstraction.The
uses_alts()methods on bothTransactionStrategyandStrategyExecutionModeprovide clean introspection for ALT usage. The enum clearly captures the two execution paths with appropriate data for each.
124-142: ALT avoidance logic correctly implements the optimization.The decision flow is sound:
- If single-stage doesn't need ALTs → use it (fastest)
- If single-stage needs ALTs but two-stage doesn't → use two-stage (avoids ALT latency)
- If both need ALTs → use single-stage (fewer transactions)
The inline comment at lines 129-131 clearly documents the rationale about ALT activation latency.
394-416: MockInfoFetcher implementation is appropriate for unit tests.The mock provides predictable responses for task building without requiring actual chain state. The implementation correctly satisfies the
TaskInfoFetchertrait requirements.
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
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 (1)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
430-436: create_test_intent signature updates look consistentAll updated call sites now pass
falsefor the newis_undelegateflag, which keeps these engine tests exercising the original “commit-only” behavior while matching the new helper signature. This aligns with the helper’s semantics (undelegate only when explicitly requested) and avoids unintentionally changing what these tests cover. You may later want a dedicated engine test exercisingis_undelegate = true, but nothing is blocking here.Also applies to: 452-457, 475-482, 503-509, 537-544, 571-580, 605-612, 666-676
♻️ Duplicate comments (1)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (1)
78-82: Same timer verification needed as lines 68-74.This segment has the same timer pattern change as lines 68-74. Please ensure the timer guard behavior is confirmed for both metrics observations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(8 hunks)magicblock-committor-service/src/intent_executor/mod.rs(5 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(3 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/mod.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/intent_executor/mod.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.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/intent_executor/mod.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/mod.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/intent_executor/mod.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (4)
commit_tasks(26-30)commit_tasks(46-104)finalize_tasks(33-36)finalize_tasks(107-199)magicblock-committor-service/src/tasks/task_strategist.rs (1)
build_execution_strategy(77-143)
🔇 Additional comments (4)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
240-256: LGTM! Valid optimization removing unnecessary RPC call.The simplification directly uses
preparation_task.chunksinstead of fetching and deserializing the on-chain chunks PDA. Sincechunksrepresents the immutable source data to be written (not on-chain state tracking), this eliminates an unnecessary RPC call while preserving correctness.
400-403: LGTM! Good optimization for the no-ALT case.The early return when
lookup_table_keysis empty avoids unnecessary HashSet construction and TableMania operations, improving performance when address lookup tables are not needed.
68-74: No action required. The timer guard behavior is correct as implemented.
HistogramTimerfrom theprometheuscrate is a RAII guard that records metrics automatically when dropped. The_timerbinding keeps the guard in scope through the async block execution, and it will record timing data when the block exits. This follows the established pattern elsewhere in the codebase (e.g.,magicblock-aperture/src/requests/http/send_transaction.rs). The underscore prefix correctly signals that the binding is held intentionally for its side effects rather than being unused.magicblock-committor-service/src/intent_executor/mod.rs (1)
140-185: Execution-mode selection via StrategyExecutionMode is wired correctlyThe new
execute_innerflow cleanly separates intents with and without committed pubkeys:
- For intents without committed pubkeys (e.g.,
MagicBaseIntent::BaseActions), you build tasks viaTaskBuilderImpl::commit_tasksand immediately feed them intoTaskStrategist::build_strategyfor a single‑stage path. That matches the behavior ofcommit_tasksforBaseActions(pure Args/BaseAction tasks) and keeps these intents out of the two‑stage machinery.- For intents with committed pubkeys, you delegate the choice between single‑stage and two‑stage execution to
TaskStrategist::build_execution_strategy, then branch onStrategyExecutionModeand call eithersingle_stage_execution_flowortwo_stage_execution_flowusing the previously extractedcommitted_pubkeys.This preserves the existing CPI‑limit fallback (still handled inside
single_stage_execution_flow) and centralizes strategy selection per the new API, without changing howtwo_stage_execution_flowis driven. The layering looks sound. One thing to double‑check is thatget_committed_pubkeys()never returnsNonefor aCommit/CommitAndUndelegateintent with “empty but semantically meaningful” commits; otherwise those would now be routed through the BaseActions branch and skip status persistence.Also applies to: 204-233
| // Build tasks for commit & finalize stages | ||
| let (commit_tasks, finalize_tasks) = { | ||
| let commit_tasks_fut = TaskBuilderImpl::commit_tasks( | ||
| &self.task_info_fetcher, | ||
| &base_intent, | ||
| persister, | ||
| ); | ||
| let finalize_tasks_fut = TaskBuilderImpl::finalize_tasks( | ||
| &self.task_info_fetcher, | ||
| &base_intent, | ||
| ); | ||
| let (commit_tasks, finalize_tasks) = | ||
| join(commit_tasks_fut, finalize_tasks_fut).await; | ||
|
|
||
| (commit_tasks?, finalize_tasks?) | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parallel commit/finalize task building is fine, but consider try_join to avoid extra work on errors
Building commit and finalize task lists in parallel with:
let (commit_tasks, finalize_tasks) =
join(commit_tasks_fut, finalize_tasks_fut).await;
let (commit_tasks, finalize_tasks) = (commit_tasks?, finalize_tasks?);correctly overlaps the two builder futures and matches the intent of the new strategy‑selection flow. However:
joinwill always await both futures to completion, even if one returns an error; you then early‑return on the first?, but any side effects of the other builder (extra RPC fetches, rent reimbursement lookups, etc.) have already happened.- If you’d prefer to short‑circuit work and side effects when one builder fails, using
futures_util::future::try_joinortokio::try_join!over the same two futures would preserve behavior while avoiding unnecessary work on failing paths.
Also worth confirming that TaskInfoFetcher and the underlying builders are safe to call concurrently on the shared Arc (they look read‑oriented from the snippets, so this is likely fine).
* master: Some optimizations for intent execution (#711)
#684
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.