Draft
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
# Conflicts: # magicblock-committor-service/src/intent_execution_manager.rs # magicblock-committor-service/src/intent_executor/intent_executor_factory.rs # 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/commit_task.rs # magicblock-committor-service/src/tasks/mod.rs # magicblock-committor-service/src/tasks/task_strategist.rs # magicblock-core/src/traits.rs # programs/magicblock/src/schedule_transactions/mod.rs # programs/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rs # test-integration/test-committor-service/tests/common.rs # test-integration/test-committor-service/tests/test_intent_executor.rs
# Conflicts: # magicblock-magic-program-api/src/instruction.rs # programs/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rs
# Conflicts: # Cargo.lock # test-integration/Cargo.lock # test-integration/Cargo.toml
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
Contributor
📝 WalkthroughWalkthroughThis pull request introduces action callback scheduling capabilities to the MagicBlock system. It adds a new Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Rust Clippy can be used to improve the quality of Rust code reviews.Clippy is the official Rust linter. It provides lints to catch common mistakes and improve your Rust code. To configure Clippy, add a See Clippy Documentation for more details. |
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-api/src/magic_validator.rs (1)
414-423: 🧹 Nitpick | 🔵 TrivialMake
actions_timeoutoperator-configurable.Line 420 hardcodes
DEFAULT_ACTIONS_TIMEOUTat the API layer, so slower/faster deployments cannot tune callback failure detection without a rebuild. This timeout changes execution semantics, so it should come fromValidatorParamsinstead of being baked in here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-api/src/magic_validator.rs` around lines 414 - 423, The ChainConfig construction is hardcoding DEFAULT_ACTIONS_TIMEOUT; make actions_timeout operator-configurable by adding an actions_timeout field to ValidatorParams and using that value when constructing ChainConfig (replace DEFAULT_ACTIONS_TIMEOUT with params.actions_timeout). Update the ValidatorParams struct (and any builders/constructors) to include a suitable Duration (or same type as ChainConfig.actions_timeout) with a default for backward compatibility, and propagate the new field to all call sites that build ValidatorParams so ChainConfig receives the operator-specified timeout.
🤖 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_execution_manager/intent_execution_engine.rs`:
- Around line 49-60: Tests construct IntentExecutionResult struct literals but
were not updated to include the new callbacks_report field; update every test
literal of IntentExecutionResult in this file to include a callbacks_report:
Vec<Result<Signature, Arc<CallbackScheduleError>>> value (use an empty vec or
appropriate mocked entries) so the constructors match the struct definition,
e.g., locate occurrences of IntentExecutionResult { ... } in the file and add
callbacks_report: vec![] or the required test data alongside existing fields
like inner, patched_errors, and callbacks_report usages in
BroadcastedIntentExecutionResult::new to ensure compilation.
In `@magicblock-committor-service/src/intent_executor/mod.rs`:
- Around line 263-304: The timeout handler in the intent execution branch
currently calls single_stage_executor.execute() again immediately after timeout
(after execute_callbacks), which can cause duplicate RPC submissions; modify the
timeout path inside the block around SingleStageExecutor::execute, so instead of
unconditionally re-calling execute() you first consult in-flight
tracking/transaction status (e.g., use the CacheTaskInfoFetcher per-account lock
/ retiring cache or an RPC query) to detect whether the timed-out attempt was
already submitted, and only if it is not in-flight retry execute(); if it is
in-flight, await confirmation or poll finalize() instead of re-submitting;
update the same pattern for the finalize() timeout path as well, and keep
references to SingleStageExecutor, execute(), execute_callbacks(), finalize(),
actions_timeout, started_at and time_left() when making these changes.
In `@magicblock-committor-service/src/intent_executor/single_stage_executor.rs`:
- Around line 26-30: The change made transaction_strategy private on
SingleStageExecutor and hid junk/patched_errors behind the restricted inner
(&mut IntentExecutorImpl), which prevents standalone callers from accessing
cleanup/error state; restore the prior public visibility by making
transaction_strategy, junk, and patched_errors public (or pub(in
crate::intent_executor) as originally intended) on SingleStageExecutor (and
mirror the same visibility adjustments in two_stage_executor.rs), and ensure
consume_strategy() still returns ownership when needed but does not prevent
callers from directly reading/cleaning up junk and patched_errors via the
executor fields or the exposed IntentExecutorImpl accessors.
In `@magicblock-committor-service/src/intent_executor/two_stage_executor.rs`:
- Around line 27-42: The Initialized and Committed state structs currently hide
commit_strategy and finalize_strategy, preventing TwoStageExecutor consumers
from accessing staged strategies for cleanup/error handling; make these fields
public (or add public accessor methods) on the Initialized and Committed structs
so callers can read the commit_strategy and finalize_strategy (mirror the
visibility used for transaction_strategy/junk/patched_errors in
single_stage_executor.rs), ensuring the TwoStageExecutor API exposes the same
staged context for standalone callers.
In `@magicblock-committor-service/src/stubs/changeset_committor_stub.rs`:
- Line 236: The stub currently sets callbacks_report: vec![] unconditionally
which hides any callback outputs; update the stub in ChangesetCommittorStub (the
code path constructing the execution result with the callbacks_report field) to
either synthesize a realistic callbacks_report from the scheduled bundle's
callback definitions (inspect the scheduled bundle/IntentBundle passed into
schedule_intent_bundles_waiting or the stubbed execution function and populate
callbacks_report accordingly) or return an immediate error when the bundle
contains callbacks so tests can detect callback-related failures; make the
change around the construction that includes the callbacks_report field (do not
leave it hardcoded to an empty vec).
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Line 624: The test/serialization vector currently only uses the branch with
callback: None, so add at least one serialization case that sets callback:
Some(BaseActionCallback { ... }) to exercise the new wire format and detect
size-budget regressions; locate the struct/instance where the field callback is
assigned (the occurrence with callback: None) and replace or add a parallel case
that fills in a realistic BaseActionCallback (matching the BaseActionCallback
type used in this module) to ensure both wire formats are covered; repeat the
same change for the other occurrence noted (the second callback: None) so both
serialization vectors include a Some callback variant.
In `@magicblock-committor-service/src/tasks/task_strategist.rs`:
- Around line 42-68: Add a unit test that constructs a mixed optimized_tasks
vector containing BaseTaskImpl variants (including multiple BaseAction items
with some callbacks Some(...) and some None) and then asserts that
has_actions_callbacks() returns true/false appropriately and that
extract_action_callbacks() returns callbacks in the expected order and
consumes/does not consume tasks as intended; locate and exercise the functions
extract_action_callbacks, has_actions_callbacks and the BaseActionTask helpers
(BaseActionTask::extract_callback and BaseActionTask::has_callback) when
building the test so it fails if ordering, filtering, or consumption semantics
change.
In `@magicblock-magic-program-api/src/response.rs`:
- Around line 9-11: Update the doc comment above the pub field named error in
response.rs to fix the typo "TimoutError" to "TimeoutError" so the comment reads
something like "TimeoutError/ActionError" for the Reason for callback execution
with ok = false; modify the triple-slash comment lines immediately preceding the
pub error: String declaration to correct the spelling.
- Around line 3-12: Change the MagicResponse struct's error field from String to
Option<String> so absence of an error is explicit (update pub struct
MagicResponse { pub ok: bool, pub data: Vec<u8>, pub error: Option<String>, });
update any constructors, builders, serializers/deserializers, and call sites
that read or set MagicResponse::error to handle Some(message)/None (and consider
adding serde attributes like skip_serializing_if = "Option::is_none" where
appropriate), and adjust tests and pattern matches that assumed a plain String
accordingly.
In `@magicblock-services/src/actions_callback_service.rs`:
- Around line 126-160: The schedule function currently returns Ok(signature)
immediately after local signing (in the map over transactions_result) while
actual submission happens asynchronously, which can report success even if
send_transaction later fails; fix this by performing the RPC submission before
returning results: after building transactions via build_transactions, iterate
the successfully built transactions, call rpc_client.send_transaction(&tx).await
(using the current async runtime—make schedule async or otherwise await the
sends) and map send errors into CallbackScheduleError so that signatures are
only returned on confirmed send; reference build_transactions, schedule,
rpc_client.send_transaction, and tx.get_signature to locate and update the code
path that currently pushes to valid_transactions and spawns the task.
- Around line 52-80: The build_transactions path drops callback.compute_units so
high-CU callbacks never get the compute-budget instruction; inside
build_transactions (in the closure that maps each BaseActionCallback) prepend a
ComputeBudgetInstruction::set_compute_unit_limit(callback.compute_units) to the
instruction list before creating the Message (i.e. build a small Vec of
instructions where the compute-budget instruction is first and then the existing
ix from Self::build_instruction), then create the Message::new_with_blockhash
from that instruction slice and continue signing as before.
In
`@programs/magicblock/src/schedule_transactions/process_add_action_callback.rs`:
- Line 24: The callback persistence path currently skips charging the delegated
payer because the call to charge_delegated_payer was commented out, making
callback scheduling free; restore and invoke charge_delegated_payer with
CALLBACK_FEE_LAMPORTS before you persist or enqueue the callback (in the
function handling add_action_callback / schedule flow), ensure you handle and
propagate any error from charge_delegated_payer so persistence is aborted on
payment failure, and remove the commented-out line so the fee is deducted prior
to saving the callback.
In
`@programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs`:
- Around line 240-247: The loop that iterates commit.callbacks_report and emits
an ic_msg for every entry (the for (idx, report) in
commit.callbacks_report.iter().enumerate() { ic_msg!(invoke_context,
"ScheduledCommitSent callback[{}]: {}", idx, report); }) can produce excessive
logs; change it to only log a bounded number of entries (e.g., first N, like 10)
and then emit a single summary ic_msg with the total callback count and how many
were omitted; keep the same variables (idx, report, invoke_context,
commit.callbacks_report) and ensure the logic preserves ordering for the logged
prefix and still composes a final summary message indicating omitted entries.
In `@programs/magicblock/src/utils/account_actions.rs`:
- Around line 33-36: Replace the silent clamp using saturating_add in the
recipient lamports update with a checked_add and propagate an error on overflow:
call recipient.borrow().lamports().checked_add(amount) and if it returns None,
return Err(InstructionError::ArithmeticOverflow); otherwise pass the resulting
sum to recipient.borrow_mut().set_lamports(...). Update the code around the
recipient lamports arithmetic (the expressions using
recipient.borrow().lamports(), amount, and recipient.borrow_mut().set_lamports)
to follow this pattern so the function returns Err(...) on overflow instead of
silently clamping.
In `@test-integration/programs/flexi-counter/src/processor/callback.rs`:
- Around line 95-119: The callback currently trusts caller-supplied
MagicResponse and ignores the prepended authority; change the logic so the first
account (currently named _validator_authority) is validated before refunding:
require that validator_authority.is_signer (or otherwise verify its pubkey
equals the expected callback authority/PDA for this flow) and only proceed to
read MagicResponse and move lamports to payer when that authority check passes;
update references in callback.rs (validator_authority, MagicResponse,
counter_pda, payer) to perform this authority check and return an error (e.g.,
ProgramError::MissingRequiredSignature or InvalidAccountData) if validation
fails.
In `@test-integration/programs/flexi-counter/src/processor/transfer_intent.rs`:
- Around line 116-125: The current MagicIntentBundleBuilder invocation only
calls commit(...) but the callback spends from counter_pda and therefore
requires the account to be undelegated/owned by crate::id() when the callback
runs; change the intent to use the commit-and-undelegate variant (e.g.,
CommitAndUndelegate/commit_and_undelegate) so counter_pda is undelegated back to
crate::id() before add_post_commit_action_with_callback(call_handler, callback)
is invoked, ensuring the refund callback has an owned source to spend from; keep
the same counter_pda.clone() reference and place the undelegate step before
build_and_invoke().
---
Outside diff comments:
In `@magicblock-api/src/magic_validator.rs`:
- Around line 414-423: The ChainConfig construction is hardcoding
DEFAULT_ACTIONS_TIMEOUT; make actions_timeout operator-configurable by adding an
actions_timeout field to ValidatorParams and using that value when constructing
ChainConfig (replace DEFAULT_ACTIONS_TIMEOUT with params.actions_timeout).
Update the ValidatorParams struct (and any builders/constructors) to include a
suitable Duration (or same type as ChainConfig.actions_timeout) with a default
for backward compatibility, and propagate the new field to all call sites that
build ValidatorParams so ChainConfig receives the operator-specified timeout.
🪄 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: 45a8a93b-2b20-4e01-93f5-10300580d21c
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (51)
Cargo.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/Cargo.tomlmagicblock-api/src/magic_sys_adapter.rsmagicblock-api/src/magic_validator.rsmagicblock-committor-service/src/actions_callback_executor.rsmagicblock-committor-service/src/committor_processor.rsmagicblock-committor-service/src/config.rsmagicblock-committor-service/src/intent_execution_manager.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/intent_executor_factory.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/lib.rsmagicblock-committor-service/src/service.rsmagicblock-committor-service/src/stubs/changeset_committor_stub.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-core/Cargo.tomlmagicblock-core/src/intent.rsmagicblock-core/src/traits.rsmagicblock-ledger/src/lib.rsmagicblock-magic-program-api/src/args.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-magic-program-api/src/lib.rsmagicblock-magic-program-api/src/response.rsmagicblock-services/Cargo.tomlmagicblock-services/src/actions_callback_service.rsmagicblock-services/src/lib.rsprograms/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/magic_sys.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/schedule_transactions/mod.rsprograms/magicblock/src/schedule_transactions/process_add_action_callback.rsprograms/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rsprograms/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rsprograms/magicblock/src/test_utils/mod.rsprograms/magicblock/src/utils/account_actions.rstest-integration/Cargo.tomltest-integration/programs/flexi-counter/src/instruction.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/programs/flexi-counter/src/processor/callback.rstest-integration/programs/flexi-counter/src/processor/schedule_intent.rstest-integration/programs/flexi-counter/src/processor/transfer_intent.rstest-integration/test-committor-service/tests/common.rstest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/test_transaction_preparator.rstest-integration/test-schedule-intent/Cargo.tomltest-integration/test-schedule-intent/tests/test_schedule_intents.rs
💤 Files with no reviewable changes (2)
- programs/magicblock/src/magic_sys.rs
- programs/magicblock/src/test_utils/mod.rs
Comment on lines
+49
to
+60
| pub callbacks_report: Vec<Result<Signature, Arc<CallbackScheduleError>>>, | ||
| } | ||
|
|
||
| impl BroadcastedIntentExecutionResult { | ||
| fn new(id: u64, execution_result: IntentExecutionResult) -> Self { | ||
| let inner = execution_result.inner.map_err(Arc::new); | ||
| let patched_errors = execution_result.patched_errors.into(); | ||
| let callbacks_report = execution_result | ||
| .callbacks_report | ||
| .into_iter() | ||
| .map(|el| el.map_err(Arc::new)) | ||
| .collect(); |
Contributor
There was a problem hiding this comment.
Add missing callbacks_report field in test IntentExecutionResult literals to avoid compile failures.
After introducing callback reporting here, local unit tests in this file still construct IntentExecutionResult without the new field. Those literals need to be updated.
💡 Suggested patch
@@
IntentExecutionResult {
inner: Err(ExecutorError::FailedToCommitError {
@@
patched_errors: vec![
TransactionStrategyExecutionError::ActionsError(
TransactionError::AccountNotFound,
None,
),
],
+ callbacks_report: vec![],
}
} else {
IntentExecutionResult {
inner: Ok(ExecutionOutput::TwoStage {
commit_signature: Signature::default(),
finalize_signature: Signature::default(),
}),
patched_errors: vec![],
+ callbacks_report: vec![],
}
};Run this to verify there are no remaining IntentExecutionResult struct literals missing callbacks_report:
#!/bin/bash
python - <<'PY'
from pathlib import Path
import re
def find_blocks(text):
pat = re.compile(r'IntentExecutionResult\s*\{')
i = 0
while True:
m = pat.search(text, i)
if not m:
break
start = m.start()
brace = text.find('{', m.start())
depth = 0
j = brace
while j < len(text):
c = text[j]
if c == '{':
depth += 1
elif c == '}':
depth -= 1
if depth == 0:
yield start, j + 1, text[start:j+1]
i = j + 1
break
j += 1
else:
break
issues = []
for p in Path('.').rglob('*.rs'):
t = p.read_text(encoding='utf-8', errors='ignore')
for s, e, block in find_blocks(t):
if 'callbacks_report' not in block:
line = t.count('\n', 0, s) + 1
issues.append((str(p), line))
if not issues:
print("OK: all IntentExecutionResult literals include callbacks_report")
else:
print("MISSING callbacks_report in IntentExecutionResult literals:")
for p, line in issues:
print(f"{p}:{line}")
PYAlso applies to: 66-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs`
around lines 49 - 60, Tests construct IntentExecutionResult struct literals but
were not updated to include the new callbacks_report field; update every test
literal of IntentExecutionResult in this file to include a callbacks_report:
Vec<Result<Signature, Arc<CallbackScheduleError>>> value (use an empty vec or
appropriate mocked entries) so the constructors match the struct definition,
e.g., locate occurrences of IntentExecutionResult { ... } in the file and add
callbacks_report: vec![] or the required test data alongside existing fields
like inner, patched_errors, and callbacks_report usages in
BroadcastedIntentExecutionResult::new to ensure compilation.
Comment on lines
+263
to
+304
| let started_at = self.started_at; | ||
| let actions_timeout = self.actions_timeout; | ||
| let time_left = || -> Option<Duration> { | ||
| actions_timeout.checked_sub(started_at.elapsed()) | ||
| }; | ||
|
|
||
| let has_callbacks = transaction_strategy.has_actions_callbacks(); | ||
| let committed_pubkeys = base_intent.get_all_committed_pubkeys(); | ||
| let res = single_stage_executor | ||
| .execute(&committed_pubkeys, persister) | ||
| .await; | ||
|
|
||
| let mut single_stage_executor = | ||
| SingleStageExecutor::new(self, transaction_strategy); | ||
| let res = if has_callbacks { | ||
| if let Some(time_left) = time_left() { | ||
| let execute_fut = single_stage_executor | ||
| .execute(&committed_pubkeys, persister); | ||
| match timeout(time_left, execute_fut).await { | ||
| Ok(res) => res, | ||
| Err(_) => { | ||
| info!( | ||
| "Intent execution timed out, cleaning up actions" | ||
| ); | ||
| single_stage_executor | ||
| .execute_callbacks(Err(ActionError::TimeoutError)); | ||
| single_stage_executor | ||
| .execute(&committed_pubkeys, persister) | ||
| .await | ||
| } | ||
| } | ||
| } else { | ||
| // Already timed out | ||
| // Handle timeout and continue execution | ||
| single_stage_executor | ||
| .execute_callbacks(Err(ActionError::TimeoutError)); | ||
| single_stage_executor | ||
| .execute(&committed_pubkeys, persister) | ||
| .await | ||
| } | ||
| } else { | ||
| single_stage_executor | ||
| .execute(&committed_pubkeys, persister) | ||
| .await | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- single_stage_execution_flow timeout branch ---'
nl -ba magicblock-committor-service/src/intent_executor/mod.rs | sed -n '263,304p'
printf '\n%s\n' '--- two_stage_execution_flow timeout branches ---'
nl -ba magicblock-committor-service/src/intent_executor/mod.rs | sed -n '355,419p'Repository: magicblock-labs/magicblock-validator
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# View the single_stage_execution_flow timeout branch (lines 263-304)
printf '%s\n' '--- single_stage_execution_flow timeout branch (lines 263-304) ---'
sed -n '263,304p' magicblock-committor-service/src/intent_executor/mod.rs | cat -n
printf '\n%s\n' '--- two_stage_execution_flow timeout branches (lines 355-419) ---'
sed -n '355,419p' magicblock-committor-service/src/intent_executor/mod.rs | cat -nRepository: magicblock-labs/magicblock-validator
Length of output: 5594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for transaction status verification or in-flight checks before retry
printf '%s\n' '--- Searching for in-flight transaction checks ---'
rg -A 5 -B 5 'in.?flight|status|check.*submit' magicblock-committor-service/src/intent_executor/ -t rust
# Look for idempotency patterns or nonce/signature deduplication
printf '\n%s\n' '--- Searching for deduplication or idempotency mechanisms ---'
rg -A 3 -B 3 'deduplicate|idempotent|nonce|signature.*dedup' magicblock-committor-service/src/ -t rustRepository: magicblock-labs/magicblock-validator
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the commit and finalize implementations to understand if they're safe to retry
printf '%s\n' '--- Searching for commit and finalize implementations ---'
rg -l 'fn (commit|finalize)' magicblock-committor-service/src/ -t rust
# Also check for any transaction signature or deduplication at RPC level
printf '\n%s\n' '--- Check if PhotonRPC or similar has deduplication ---'
rg -A 5 'submit.*transaction|send.*instruction' magicblock-committor-service/src/intent_executor/ -t rust | head -50Repository: magicblock-labs/magicblock-validator
Length of output: 602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check SingleStageExecutor::execute to see if it refetches nonces
printf '%s\n' '--- SingleStageExecutor::execute signature and flow ---'
rg -A 15 'impl SingleStageExecutor' magicblock-committor-service/src/intent_executor/single_stage_executor.rs | head -60
printf '\n%s\n' '--- TwoStageExecutor commit/finalize signature ---'
rg -A 10 'fn commit\(' magicblock-committor-service/src/intent_executor/two_stage_executor.rs | head -40Repository: magicblock-labs/magicblock-validator
Length of output: 135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if nonces are refetched on each execute call or acquired once
printf '%s\n' '--- Check task building flow (fetch nonces timing) ---'
rg -B 5 -A 15 'pub async fn execute' magicblock-committor-service/src/intent_executor/single_stage_executor.rs | head -80
# Also check TwoStageExecutor
printf '\n%s\n' '--- TwoStageExecutor commit ---'
rg -B 5 -A 15 'pub async fn commit' magicblock-committor-service/src/intent_executor/two_stage_executor.rs | head -80Repository: magicblock-labs/magicblock-validator
Length of output: 1452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for task preparation to see when nonces are fetched relative to execute()
printf '%s\n' '--- When are tasks prepared and nonces fetched ---'
rg -B 10 -A 10 'prepare_and_execute_strategy\|fn prepare_transaction_strategy' magicblock-committor-service/src/intent_executor/mod.rs | head -100Repository: magicblock-labs/magicblock-validator
Length of output: 130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the prepare_and_execute_strategy implementation
printf '%s\n' '--- prepare_and_execute_strategy definition ---'
rg -B 5 -A 40 'async fn prepare_and_execute_strategy' magicblock-committor-service/src/intent_executor/mod.rs | head -120
# Also check where this function fetches nonces
printf '\n%s\n' '--- Nonce fetching in execution flow ---'
rg -B 2 -A 8 'fetch.*nonce\|prepare_transaction_strategy' magicblock-committor-service/src/intent_executor/mod.rs | head -100Repository: magicblock-labs/magicblock-validator
Length of output: 1537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get more context on the timeout execution flow (lines 263-304, 355-419)
# Focus on understanding the order of operations
printf '%s\n' '--- Full single_stage execution context (lines 240-310) ---'
sed -n '240,310p' magicblock-committor-service/src/intent_executor/mod.rs
printf '\n%s\n' '--- Full two_stage commit context (lines 330-420) ---'
sed -n '330,420p' magicblock-committor-service/src/intent_executor/mod.rsRepository: magicblock-labs/magicblock-validator
Length of output: 6794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check transaction_preparator.prepare_for_strategy to see if it refetches nonces
printf '%s\n' '--- TransactionPreparator prepare_for_strategy ---'
rg -B 5 -A 40 'fn prepare_for_strategy' magicblock-committor-service/src/transaction_preparator/ -t rust
# Also check if there's any in-flight or status checking
printf '\n%s\n' '--- Any in-flight TX status checks before send ---'
rg 'get_signature_status\|check.*status\|is_confirmed' magicblock-committor-service/src/intent_executor/ -t rust | head -20Repository: magicblock-labs/magicblock-validator
Length of output: 8854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for transaction_preparator module structure
fd -type f 'preparator' magicblock-committor-service/src/ -name '*.rs' | head -10
# Check what gets prepared
printf '%s\n' '--- Files in transaction_preparator ---'
fd -type f . magicblock-committor-service/src/transaction_preparator/ -name '*.rs'Repository: magicblock-labs/magicblock-validator
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for delivery_preparator prepare_for_delivery to see if it refetches nonces
printf '%s\n' '--- DeliveryPreparator prepare_for_delivery ---'
rg -B 3 -A 30 'fn prepare_for_delivery' magicblock-committor-service/src/transaction_preparator/ --type rust | head -100Repository: magicblock-labs/magicblock-validator
Length of output: 4165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if tasks are rebuilt or nonces updated in execute() loop
printf '%s\n' '--- SingleStageExecutor execute loop structure ---'
sed -n '1,150p' magicblock-committor-service/src/intent_executor/single_stage_executor.rs
# Also check TwoStageExecutor
printf '\n%s\n' '--- TwoStageExecutor commit loop structure ---'
sed -n '1,150p' magicblock-committor-service/src/intent_executor/two_stage_executor.rsRepository: magicblock-labs/magicblock-validator
Length of output: 9842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if TransactionStrategy or tasks are modified between retries
printf '%s\n' '--- Check task mutation/rebuild in execute loop ---'
rg -B 5 -A 15 'RECURSION_CEILING\|current_attempt' magicblock-committor-service/src/intent_executor/single_stage_executor.rs --type rustRepository: magicblock-labs/magicblock-validator
Length of output: 131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if TransactionStrategy is modified between retry attempts
# Look for any mutation of tasks in the execute loop
printf '%s\n' '--- Check if transaction_strategy tasks are mutated ---'
sed -n '50,140p' magicblock-committor-service/src/intent_executor/single_stage_executor.rs
# Also verify when tasks are built with nonces
printf '\n%s\n' '--- When are task nonces determined (before execute) ---'
rg -B 10 'SingleStageExecutor::new\|TwoStageExecutor::new' magicblock-committor-service/src/intent_executor/mod.rs | head -40Repository: magicblock-labs/magicblock-validator
Length of output: 3284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if prepare_for_strategy actually rebuilds tasks or just prepares buffers
printf '%s\n' '--- Does prepare_for_strategy modify tasks? ---'
sed -n '1,100p' magicblock-committor-service/src/transaction_preparator/mod.rs | tail -60
# Verify if optimized_tasks are immutable during execution
printf '\n%s\n' '--- TransactionStrategy structure ---'
rg -B 3 -A 12 'pub struct TransactionStrategy' magicblock-committor-service/src/tasks/ --type rust | head -50Repository: magicblock-labs/magicblock-validator
Length of output: 3576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the exact timeline: when are tasks built vs when is timeout applied
printf '%s\n' '--- Timeline: when SingleStageExecutor/tasks created vs timeout ---'
sed -n '250,280p' magicblock-committor-service/src/intent_executor/mod.rs
# Check if BaseTaskImpl contains nonce
printf '\n%s\n' '--- BaseTaskImpl structure and nonce field ---'
rg -B 3 -A 8 'pub enum BaseTaskImpl\|CommitTask {' magicblock-committor-service/src/tasks/ --type rust | head -60Repository: magicblock-labs/magicblock-validator
Length of output: 1529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify: prepare_for_delivery does NOT rebuild tasks
# It only prepares buffers
printf '%s\n' '--- What does prepare_task_handling_errors do to task? ---'
rg -B 5 -A 25 'fn prepare_task_handling_errors' magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs --type rust | head -80Repository: magicblock-labs/magicblock-validator
Length of output: 1243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check CommitTask structure to confirm nonce is embedded
printf '%s\n' '--- CommitTask structure ---'
rg -B 5 -A 15 'pub struct CommitTask' magicblock-committor-service/src/tasks/ --type rust | head -50
# Also check FinalizeTask
printf '\n%s\n' '--- FinalizeTask structure ---'
rg -B 5 -A 15 'pub struct FinalizeTask' magicblock-committor-service/src/tasks/ --type rust | head -50Repository: magicblock-labs/magicblock-validator
Length of output: 3481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check when nonces are actually fetched in the task building/preparation
# Look for fetch_next_commit_nonces calls
printf '%s\n' '--- When fetch_next_commit_nonces is called ---'
rg -B 10 'fetch_next_commit_nonces\|fetch_current_commit_nonces' magicblock-committor-service/src/intent_executor/mod.rs --type rust | head -60
# Also check the transaction strategy preparation
printf '\n%s\n' '--- TransactionStrategy preparation and task building ---'
rg -B 8 -A 8 'prepare_transaction_strategy' magicblock-committor-service/src/intent_executor/mod.rs --type rust | head -80Repository: magicblock-labs/magicblock-validator
Length of output: 127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where TransactionStrategy is created/built in the execution flow
printf '%s\n' '--- Where is TransactionStrategy created in execute_intent ---'
rg -B 15 'single_stage_execution_flow\|two_stage_execution_flow' magicblock-committor-service/src/intent_executor/mod.rs --type rust | grep -A 15 'pub async fn execute_intent'
# Search for strategy preparation
printf '\n%s\n' '--- Strategy preparation before execution ---'
rg -B 20 'StrategyExecutionMode' magicblock-committor-service/src/intent_executor/mod.rs --type rust | head -80Repository: magicblock-labs/magicblock-validator
Length of output: 142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the full execute_intent to see when strategy is created
printf '%s\n' '--- Full execute_intent function ---'
sed -n '150,250p' magicblock-committor-service/src/intent_executor/mod.rsRepository: magicblock-labs/magicblock-validator
Length of output: 3407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the timeline: tasks built once, then strategy passed to executor
printf '%s\n' '--- TaskBuilderImpl::commit_tasks to see when nonce is fetched ---'
rg -B 10 -A 30 'pub async fn commit_tasks' magicblock-committor-service/src/tasks/task_builder.rs --type rust | head -100Repository: magicblock-labs/magicblock-validator
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any transaction status checking before the retry
printf '%s\n' '--- Check for any TX status validation before timeout retry ---'
sed -n '275,310p' magicblock-committor-service/src/intent_executor/mod.rs
# Verify the exact timeout retry code again
printf '\n%s\n' '--- Finalize timeout retry ---'
sed -n '400,415p' magicblock-committor-service/src/intent_executor/mod.rsRepository: magicblock-labs/magicblock-validator
Length of output: 2391
Do not immediately retry a timed-out stage without checking if the transaction was already submitted.
When tokio::time::timeout fires on the execute() call, it only stops waiting locally—it does not guarantee the commit/finalize transaction was never sent to RPC or won't still land. The current code retries immediately with the same tasks and nonces:
- Lines 275–294 (single-stage timeout): fires callbacks, then calls
execute()again - Lines 391–407 (finalize timeout): fires callbacks, then calls
finalize()again
Even though nonces provide on-chain deduplication (only one transaction applies), this pattern causes:
- Double submission to RPC — wasted bandwidth and unnecessary load
- Misclassification — slow confirmation or mempool backlog gets reported as an action timeout
The nonce system in the codebase (CacheTaskInfoFetcher with per-account locks and retiring cache) shows awareness of in-flight request tracking. Before retrying, consult this or query the transaction status to determine if the first attempt reached the network. If already in-flight, await confirmation; if genuinely unsent, retry with confidence.
Applies to: Lines 263–304, 355–419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/src/intent_executor/mod.rs` around lines 263 -
304, The timeout handler in the intent execution branch currently calls
single_stage_executor.execute() again immediately after timeout (after
execute_callbacks), which can cause duplicate RPC submissions; modify the
timeout path inside the block around SingleStageExecutor::execute, so instead of
unconditionally re-calling execute() you first consult in-flight
tracking/transaction status (e.g., use the CacheTaskInfoFetcher per-account lock
/ retiring cache or an RPC query) to detect whether the timed-out attempt was
already submitted, and only if it is not in-flight retry execute(); if it is
in-flight, await confirmation or poll finalize() instead of re-submitting;
update the same pattern for the finalize() timeout path as well, and keep
references to SingleStageExecutor, execute(), execute_callbacks(), finalize(),
actions_timeout, started_at and time_left() when making these changes.
Comment on lines
+26
to
+30
| pub struct SingleStageExecutor<'a, T, F, A> { | ||
| current_attempt: u8, | ||
| // TODO(edwin): remove this and replace with IntentClient | ||
| pub(in crate::intent_executor) inner: &'a mut IntentExecutorImpl<T, F, A>, | ||
| transaction_strategy: TransactionStrategy, |
Contributor
There was a problem hiding this comment.
Keep the standalone executor reports accessible.
transaction_strategy is now private and consume_strategy() only gives it back at the end, while junk / patched_errors stay hidden behind a restricted inner. That regresses the direct cleanup/error-handling surface this executor is intended to expose.
Based on learnings: in single_stage_executor.rs and two_stage_executor.rs, transaction_strategy, junk, and patched_errors are intentionally public so standalone callers can handle cleanup and errors.
Also applies to: 137-148
🤖 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 26 - 30, The change made transaction_strategy private on
SingleStageExecutor and hid junk/patched_errors behind the restricted inner
(&mut IntentExecutorImpl), which prevents standalone callers from accessing
cleanup/error state; restore the prior public visibility by making
transaction_strategy, junk, and patched_errors public (or pub(in
crate::intent_executor) as originally intended) on SingleStageExecutor (and
mirror the same visibility adjustments in two_stage_executor.rs), and ensure
consume_strategy() still returns ownership when needed but does not prevent
callers from directly reading/cleaning up junk and patched_errors via the
executor fields or the exposed IntentExecutorImpl accessors.
Comment on lines
27
to
+42
| pub struct Initialized { | ||
| /// Commit stage strategy | ||
| pub commit_strategy: TransactionStrategy, | ||
| commit_strategy: TransactionStrategy, | ||
| /// Finalize stage strategy | ||
| pub finalize_strategy: TransactionStrategy, | ||
| finalize_strategy: TransactionStrategy, | ||
|
|
||
| current_attempt: u8, | ||
| } | ||
|
|
||
| pub struct Committed { | ||
| /// Signature of commit stage | ||
| pub commit_signature: Signature, | ||
| commit_signature: Signature, | ||
| /// Finalize stage strategy | ||
| pub finalize_strategy: TransactionStrategy, | ||
| finalize_strategy: TransactionStrategy, | ||
|
|
||
| current_attempt: u8, |
Contributor
There was a problem hiding this comment.
Expose the staged strategies for direct TwoStageExecutor users.
The new Initialized / Committed state wrappers keep commit_strategy and finalize_strategy private, so standalone callers lose the staged cleanup/error context that this executor is supposed to expose. Please keep those strategies reachable via public fields or accessors.
Based on learnings: in single_stage_executor.rs and two_stage_executor.rs, transaction_strategy, junk, and patched_errors are intentionally public so standalone callers can handle cleanup and errors.
Also applies to: 52-56
🤖 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 27 - 42, The Initialized and Committed state structs currently hide
commit_strategy and finalize_strategy, preventing TwoStageExecutor consumers
from accessing staged strategies for cleanup/error handling; make these fields
public (or add public accessor methods) on the Initialized and Committed structs
so callers can read the commit_strategy and finalize_strategy (mirror the
visibility used for transaction_strategy/junk/patched_errors in
single_stage_executor.rs), ensuring the TwoStageExecutor API exposes the same
staged context for standalone callers.
| finalize_signature: Signature::new_unique(), | ||
| }), | ||
| patched_errors: Arc::new(vec![]), | ||
| callbacks_report: vec![], |
Contributor
There was a problem hiding this comment.
Don't hardcode callbacks_report to empty here.
This makes every stubbed execution look like it produced no callback output, so callback-aware callers and tests can never surface callback failures or signatures through schedule_intent_bundles_waiting. Either synthesize the report from the scheduled bundle or fail fast when callbacks are present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/src/stubs/changeset_committor_stub.rs` at line
236, The stub currently sets callbacks_report: vec![] unconditionally which
hides any callback outputs; update the stub in ChangesetCommittorStub (the code
path constructing the execution result with the callbacks_report field) to
either synthesize a realistic callbacks_report from the scheduled bundle's
callback definitions (inspect the scheduled bundle/IntentBundle passed into
schedule_intent_bundles_waiting or the stubbed execution function and populate
callbacks_report accordingly) or return an immediate error when the bundle
contains callbacks so tests can detect callback-related failures; make the
change around the construction that includes the callbacks_report field (do not
leave it hardcoded to an empty vec).
| }, | ||
| MagicContext, | ||
| }; | ||
| const CALLBACK_FEE_LAMPORTS: u64 = 5_000; |
Contributor
There was a problem hiding this comment.
Charge for callback attachment before persisting it.
Line 63 is commented out even though CALLBACK_FEE_LAMPORTS and charge_delegated_payer are already wired in. That makes callback scheduling effectively free while the validator still pays to submit the follow-up callback transaction.
Proposed fix
let context_acc = get_instruction_account_with_idx(
transaction_context,
MAGIC_CONTEXT_IDX,
)?;
- // charge_delegated_payer(payer_acc, context_acc, CALLBACK_FEE_LAMPORTS)?;
+ charge_delegated_payer(payer_acc, context_acc, CALLBACK_FEE_LAMPORTS)?;
let context_data = &mut context_acc.borrow_mut();Also applies to: 59-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/magicblock/src/schedule_transactions/process_add_action_callback.rs`
at line 24, The callback persistence path currently skips charging the delegated
payer because the call to charge_delegated_payer was commented out, making
callback scheduling free; restore and invoke charge_delegated_payer with
CALLBACK_FEE_LAMPORTS before you persist or enqueue the callback (in the
function handling add_action_callback / schedule flow), ensure you handle and
propagate any error from charge_delegated_payer so persistence is aborted on
payment failure, and remove the commented-out line so the fee is deducted prior
to saving the callback.
Comment on lines
+240
to
+247
| for (idx, report) in commit.callbacks_report.iter().enumerate() { | ||
| ic_msg!( | ||
| invoke_context, | ||
| "ScheduledCommitSent callback[{}]: {}", | ||
| idx, | ||
| report | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Bound callback-report logging to protect instruction reliability.
Line 240 logs every callback entry. Capping emitted entries avoids log amplification when callback lists are large.
♻️ Proposed change
+ const MAX_CALLBACK_REPORTS_TO_LOG: usize = 32;
- for (idx, report) in commit.callbacks_report.iter().enumerate() {
+ for (idx, report) in commit
+ .callbacks_report
+ .iter()
+ .take(MAX_CALLBACK_REPORTS_TO_LOG)
+ .enumerate()
+ {
ic_msg!(
invoke_context,
"ScheduledCommitSent callback[{}]: {}",
idx,
report
);
}
+ if commit.callbacks_report.len() > MAX_CALLBACK_REPORTS_TO_LOG {
+ ic_msg!(
+ invoke_context,
+ "ScheduledCommitSent callbacks omitted: {}",
+ commit.callbacks_report.len() - MAX_CALLBACK_REPORTS_TO_LOG
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (idx, report) in commit.callbacks_report.iter().enumerate() { | |
| ic_msg!( | |
| invoke_context, | |
| "ScheduledCommitSent callback[{}]: {}", | |
| idx, | |
| report | |
| ); | |
| } | |
| const MAX_CALLBACK_REPORTS_TO_LOG: usize = 32; | |
| for (idx, report) in commit | |
| .callbacks_report | |
| .iter() | |
| .take(MAX_CALLBACK_REPORTS_TO_LOG) | |
| .enumerate() | |
| { | |
| ic_msg!( | |
| invoke_context, | |
| "ScheduledCommitSent callback[{}]: {}", | |
| idx, | |
| report | |
| ); | |
| } | |
| if commit.callbacks_report.len() > MAX_CALLBACK_REPORTS_TO_LOG { | |
| ic_msg!( | |
| invoke_context, | |
| "ScheduledCommitSent callbacks omitted: {}", | |
| commit.callbacks_report.len() - MAX_CALLBACK_REPORTS_TO_LOG | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs`
around lines 240 - 247, The loop that iterates commit.callbacks_report and emits
an ic_msg for every entry (the for (idx, report) in
commit.callbacks_report.iter().enumerate() { ic_msg!(invoke_context,
"ScheduledCommitSent callback[{}]: {}", idx, report); }) can produce excessive
logs; change it to only log a bounded number of entries (e.g., first N, like 10)
and then emit a single summary ic_msg with the total callback count and how many
were omitted; keep the same variables (idx, report, invoke_context,
commit.callbacks_report) and ensure the logic preserves ordering for the logged
prefix and still composes a final summary message indicating omitted entries.
Comment on lines
+33
to
+36
| let recipient_lamports = recipient.borrow().lamports(); | ||
| recipient | ||
| .borrow_mut() | ||
| .set_lamports(recipient_lamports.saturating_add(amount)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's explore the repository structure
ls -la | head -20Repository: magicblock-labs/magicblock-validator
Length of output: 1308
🏁 Script executed:
# Find the specific file
find . -name "account_actions.rs" -type fRepository: magicblock-labs/magicblock-validator
Length of output: 130
🏁 Script executed:
# Read the file to understand the context
cat -n "programs/magicblock/src/utils/account_actions.rs" | head -60Repository: magicblock-labs/magicblock-validator
Length of output: 1882
🏁 Script executed:
#!/bin/bash
# Search for lamport operations with saturating_add in the programs directory
rg "saturating_add|checked_add" programs/ --type rust -B 2 -A 2Repository: magicblock-labs/magicblock-validator
Length of output: 2961
🏁 Script executed:
# Search for other lamport transfer patterns in the codebase
rg "set_lamports" programs/ --type rust -B 3 -A 1Repository: magicblock-labs/magicblock-validator
Length of output: 6415
🏁 Script executed:
# Check if there are other account_actions patterns with error handling
rg "InstructionError" programs/magicblock/src/utils/ --type rustRepository: magicblock-labs/magicblock-validator
Length of output: 2161
Replace saturating_add with checked_add and return InstructionError::ArithmeticOverflow.
Line 36 silently clamps on overflow, which breaks lamport transfer invariants. The codebase consistently uses checked_add with proper error propagation for all other lamport arithmetic operations. The function already returns Result<(), InstructionError>, making error handling the expected pattern.
Proposed fix
let recipient_lamports = recipient.borrow().lamports();
- recipient
- .borrow_mut()
- .set_lamports(recipient_lamports.saturating_add(amount));
+ let new_recipient_lamports = recipient_lamports
+ .checked_add(amount)
+ .ok_or(InstructionError::ArithmeticOverflow)?;
+ recipient.borrow_mut().set_lamports(new_recipient_lamports);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let recipient_lamports = recipient.borrow().lamports(); | |
| recipient | |
| .borrow_mut() | |
| .set_lamports(recipient_lamports.saturating_add(amount)); | |
| let recipient_lamports = recipient.borrow().lamports(); | |
| let new_recipient_lamports = recipient_lamports | |
| .checked_add(amount) | |
| .ok_or(InstructionError::ArithmeticOverflow)?; | |
| recipient.borrow_mut().set_lamports(new_recipient_lamports); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/magicblock/src/utils/account_actions.rs` around lines 33 - 36,
Replace the silent clamp using saturating_add in the recipient lamports update
with a checked_add and propagate an error on overflow: call
recipient.borrow().lamports().checked_add(amount) and if it returns None, return
Err(InstructionError::ArithmeticOverflow); otherwise pass the resulting sum to
recipient.borrow_mut().set_lamports(...). Update the code around the recipient
lamports arithmetic (the expressions using recipient.borrow().lamports(),
amount, and recipient.borrow_mut().set_lamports) to follow this pattern so the
function returns Err(...) on overflow instead of silently clamping.
Comment on lines
+95
to
+119
| let [_validator_authority, counter_pda, payer, _system_program] = accounts | ||
| else { | ||
| return Err(ProgramError::NotEnoughAccountKeys); | ||
| }; | ||
|
|
||
| let response: MagicResponse = bincode::deserialize(data) | ||
| .map_err(|_| ProgramError::InvalidInstructionData)?; | ||
|
|
||
| msg!( | ||
| "TransferCallback: ok={}, error={}", | ||
| response.ok, | ||
| response.error | ||
| ); | ||
|
|
||
| if !response.ok { | ||
| let amount = u64::from_le_bytes( | ||
| response | ||
| .data | ||
| .try_into() | ||
| .map_err(|_| ProgramError::InvalidInstructionData)?, | ||
| ); | ||
| msg!("TransferCallback: refunding {} lamports to payer", amount); | ||
| **counter_pda.try_borrow_mut_lamports()? -= amount; | ||
| **payer.try_borrow_mut_lamports()? += amount; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't let arbitrary callers trigger the refund path.
Lines 95-118 ignore the prepended authority and refund based only on caller-supplied MagicResponse. That makes this callback instruction directly invokable, so the integration flow can appear to work without proving that MagicBlock actually emitted the callback. Validate the callback authority/capability before moving lamports.
Based on learnings, the file test-integration/programs/flexi-counter/src/processor/callback.rs contains a test smart contract used for integration testing, not production code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-integration/programs/flexi-counter/src/processor/callback.rs` around
lines 95 - 119, The callback currently trusts caller-supplied MagicResponse and
ignores the prepended authority; change the logic so the first account
(currently named _validator_authority) is validated before refunding: require
that validator_authority.is_signer (or otherwise verify its pubkey equals the
expected callback authority/PDA for this flow) and only proceed to read
MagicResponse and move lamports to payer when that authority check passes;
update references in callback.rs (validator_authority, MagicResponse,
counter_pda, payer) to perform this authority check and return an error (e.g.,
ProgramError::MissingRequiredSignature or InvalidAccountData) if validation
fails.
Comment on lines
+116
to
+125
| // CommitAndUndelegate so that counter_pda is owned by the flexi-counter | ||
| // program again by the time the callback fires, enabling the lamport refund. | ||
| MagicIntentBundleBuilder::new( | ||
| payer.clone(), | ||
| magic_context.clone(), | ||
| magic_program.clone(), | ||
| ) | ||
| .commit(&[counter_pda.clone()]) | ||
| .add_post_commit_action_with_callback(call_handler, callback) | ||
| .build_and_invoke() |
Contributor
There was a problem hiding this comment.
A plain commit can't support this refund callback.
The callback spends from counter_pda, but this intent never undelegates that account back to crate::id(). On the failure path the callback therefore has no owned refund source, so the refund flow described here cannot finalize.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-integration/programs/flexi-counter/src/processor/transfer_intent.rs`
around lines 116 - 125, The current MagicIntentBundleBuilder invocation only
calls commit(...) but the callback spends from counter_pda and therefore
requires the account to be undelegated/owned by crate::id() when the callback
runs; change the intent to use the commit-and-undelegate variant (e.g.,
CommitAndUndelegate/commit_and_undelegate) so counter_pda is undelegated back to
crate::id() before add_post_commit_action_with_callback(call_handler, callback)
is invoked, ensuring the refund callback has an owned source to spend from; keep
the same counter_pda.clone() reference and place the undelegate step before
build_and_invoke().
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
1 participant
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
Introduces an ability to add callback for BaseAction. In case Action fails to be executed due to ActionError/Timeout reasons on BaseChain we will execute user supplied callback function so state could be finalized accordingly.
PR defines:
magicblock-servicescrate where future services could be presentActionCallbackServicewithinmagicblock-servicesthat shots supplied callbacks on ERmagic-programSentCommit contains callbacks signaturesMagicBlockInstruction::AddActionCallbackthat inserts callback in last scheduled Intent. This doesn't break backwards-compatability ofScheduleIntentBundle.For more see #996.
Compatibility
Testing
Checklist
Summary by CodeRabbit
New Features
Changes