feat: Add ScheduleCommitFinalize along with CommitFinalizeTask#847
feat: Add ScheduleCommitFinalize along with CommitFinalizeTask#847
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughAdds end-to-end "CommitFinalize" support and a new on-chain instruction 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
173-173: Replace.expect()on commit_id lookup with a typed error.Panics are disallowed in production Rust here; return a
TaskBuilderErrorinstead. As per coding guidelines, avoidunwrap()/expect()in production paths.🔧 Proposed fix
- let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!"); + let commit_id = *commit_ids + .get(&account.pubkey) + .ok_or(TaskBuilderError::MissingCommitId(account.pubkey))?;pub enum TaskBuilderError { #[error("CommitIdFetchError: {0}")] CommitTasksBuildError(#[source] TaskInfoFetcherError), #[error("FinalizedTasksBuildError: {0}")] FinalizedTasksBuildError(#[source] TaskInfoFetcherError), + #[error("Missing commit id for pubkey {0}")] + MissingCommitId(Pubkey), } @@ pub fn signature(&self) -> Option<Signature> { match self { Self::CommitTasksBuildError(err) => err.signature(), Self::FinalizedTasksBuildError(err) => err.signature(), + Self::MissingCommitId(_) => None, } } }
🤖 Fix all issues with AI agents
In `@magicblock-committor-service/src/intent_executor/mod.rs`:
- Around line 190-229: Remove the entire commented-out branch that starts with
the base_intent.is_commit_finalize() check and the code that builds commit_tasks
via TaskBuilderImpl::commit_tasks and calls
TaskStrategist::build_execution_strategy; then ensure the unified execution path
(where you now always proceed through the non-commented logic) never selects a
two-stage execution with an empty finalize stage—specifically verify the logic
that interprets TaskStrategist::StrategyExecutionMode does not produce
StrategyExecutionMode::TwoStage (and thus does not call
two_stage_execution_flow) when finalize_tasks is empty for
CommitFinalize/CommitFinalizeAndUndelegate flows, and instead falls back to the
single-stage behavior used by single_stage_execution_flow.
- Around line 163-167: The TODO about MagicBaseIntent::BaseActions being handled
in the execution path must be tracked by creating a follow-up issue and linking
it from the code; open a ticket describing the expected behavior, migration plan
(either implementing BaseActions handling or renaming the function to a generic
name), and any test cases, then replace the inline TODO in mod.rs (the comment
immediately above the call to TaskBuilderImpl::commit_tasks) with a short
one-line comment that includes the created issue/ID and a one-sentence summary
so future readers can find the ticket (and update any references to
MagicBaseIntent::BaseActions or TaskBuilderImpl::commit_tasks in the issue
text).
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 123-129: The CommitFinalizeTask struct currently only derives
Clone; add Debug to its derives so it matches CommitDiffTask and improves
logging/observability—i.e., update the derive on the CommitFinalizeTask
declaration to #[derive(Clone, Debug)] (also consider adding Debug to CommitTask
if you want full parity), ensuring any contained types (Account,
CommittedAccount) implement Debug or derive it as needed.
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Around line 111-116: The code builds Commit/CommitDiff tasks for
MagicBaseIntent::CommitFinalize and ::CommitFinalizeAndUndelegate but should
construct CommitFinalizeTask when base_intent.is_commit_finalize(); update the
match arms handling MagicBaseIntent::CommitFinalize(t) and
MagicBaseIntent::CommitFinalizeAndUndelegate(t) to instantiate
CommitFinalizeTask (using t.get_committed_accounts() for the former and
t.commit_action.get_committed_accounts() for the latter), set the finalize_tasks
payload appropriately (not leaving it empty for CommitFinalize), and ensure the
boolean/undelegate flag is propagated to the CommitFinalizeTask constructor so
finalization uses the new instruction.
In `@magicblock-magic-program-api/src/instruction.rs`:
- Around line 109-124: Add a new instruction enum variant
ScheduleCommitFinalizeAndUndelegate to mirror ScheduleCommitFinalize and restore
symmetry with existing CommitFinalizeAndUndelegate/CommitFinalize pairs: add the
new variant (with similar documentation as ScheduleCommitFinalize) to the
instruction enum in instruction.rs, ensure its name matches the existing
MagicBaseIntentArgs/MagicBaseIntent variant CommitFinalizeAndUndelegate and the
existing ScheduleCommit/ScheduleCommitAndUndelegate naming pattern, and then
update any pattern matches or handler code that enumerates schedule variants so
the new variant is recognized where ScheduleCommitAndUndelegate and
ScheduleCommitFinalize are handled.
In
`@programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs`:
- Around line 249-256: The code builds a legacy Commit/CommitAndUndelegate
intent (MagicBaseIntent::Commit / MagicBaseIntent::CommitAndUndelegate using
CommitType::Standalone) which will bypass the new finalize workflow; change the
branch that currently uses Commit and CommitAndUndelegate to instead construct
MagicBaseIntent::CommitFinalize and MagicBaseIntent::CommitFinalizeAndUndelegate
(with the appropriate CommitFinalize::Standalone payload or equivalent) when the
ScheduleCommitFinalize path is intended — use the same opts.request_undelegation
conditional to pick between CommitFinalizeAndUndelegate and CommitFinalize so
the new CommitFinalize task flow is invoked.
- Around line 40-42: The code clones invoke_context.transaction_context into
transaction_context and then mutates accounts/state (e.g., undelegation marking,
MagicContext updates), which can either be a no-op if TransactionContext::clone
is deep or expensive if shallow; instead verify TransactionContext::clone
semantics and remove the clone: borrow the transaction_context directly from
invoke_context (use an immutable or mutable borrow as required) and limit the
borrow scope around calls like get_current_instruction_context() and follow-up
mutations so changes apply to the real context and avoid unnecessary copies;
update usages that assume ownership to use &transaction_context or &mut
transaction_context and ensure functions like get_current_instruction_context()
are called on the borrowed instance.
In `@test-integration/Cargo.toml`:
- Around line 64-66: Replace the local path dependency for
magicblock-delegation-program in this Cargo.toml with a workspace reference so
the crate is inherited from the workspace root; specifically change the
dependency entry for magicblock-delegation-program to use { workspace = true }
while preserving the features array (e.g., keep "no-entrypoint") so other
test-integration crates follow the same workspace-based pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/types.rsmagicblock-magic-program-api/src/args.rsmagicblock-magic-program-api/src/instruction.rsprograms/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/schedule_transactions/mod.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rstest-integration/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-magic-program-api/src/args.rsprograms/magicblock/src/schedule_transactions/mod.rsmagicblock-committor-service/src/types.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/intent_executor/mod.rsprograms/magicblock/src/magicblock_processor.rsmagicblock-magic-program-api/src/instruction.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/task_builder.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/Cargo.toml
🧠 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-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:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 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:
programs/magicblock/src/schedule_transactions/mod.rsmagicblock-committor-service/src/types.rsmagicblock-committor-service/src/intent_executor/mod.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-committor-service/src/tasks/task_builder.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:
programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.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:
programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rsmagicblock-committor-service/src/tasks/task_builder.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:
programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.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:
programs/magicblock/src/magicblock_processor.rs
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.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/tasks/task_builder.rs
🧬 Code graph analysis (3)
programs/magicblock/src/schedule_transactions/mod.rs (1)
programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs (1)
process_schedule_commit_finalize(27-277)
magicblock-committor-service/src/tasks/args_task.rs (1)
magicblock-committor-service/src/types.rs (1)
value(23-33)
magicblock-committor-service/src/tasks/task_builder.rs (2)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
new(42-54)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(680-685)
🔇 Additional comments (10)
magicblock-committor-service/src/types.rs (1)
28-31: LGTM!The new
CommitFinalizeandCommitFinalizeAndUndelegatematch arms are consistent with the existing variant handling pattern and use appropriately descriptive metric labels.magicblock-magic-program-api/src/args.rs (1)
88-89: LGTM!The new
CommitFinalizeandCommitFinalizeAndUndelegatevariants appropriately reuse the existingCommitTypeArgsandCommitAndUndelegateArgstypes, maintaining consistency with the parallelCommitandCommitAndUndelegatevariants.magicblock-committor-service/src/tasks/mod.rs (1)
37-37: LGTM!The new
CommitFinalizevariant extends theTaskTypeenum appropriately for the new commit-finalize workflow.Cargo.toml (1)
104-106: Verify../delegation-programis available in CI/builds.Switching to a path dependency outside the repo requires a sibling checkout; if CI doesn’t provision it, builds will fail. Please confirm workflows/docs handle this, or keep a git fallback for non-monorepo builds.
magicblock-committor-service/src/tasks/task_builder.rs (1)
5-8: Finalize/undelegate task assembly looks cleanly consolidated.The new handler reduces duplication and keeps
CommitAndUndelegate/CommitFinalizeAndUndelegateflows aligned.Also applies to: 238-294
magicblock-committor-service/src/tasks/args_task.rs (2)
208-213: Confirm compute budget for CommitFinalize.
CommitFinalizelikely includes commit+finalize work; 25,000 CU may be low vs commit/finalize tasks. Please confirm with measurement or adjust to avoid CU budget errors.
1-10: CommitFinalize wiring across instruction/budget/task_type looks consistent.Instruction builder, size budget, task_type/reset_commit_id, and label mappings align with the new variant.
Also applies to: 20-26, 30-37, 96-130, 184-187, 231-236, 261-270, 277-287, 295-305
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
110-113: CommitFinalize variants are wired consistently across construction and accessors.Construction, predicates, and committed-account accessors correctly incorporate the new variants.
Also applies to: 126-128, 152-163, 166-183, 193-199, 211-217, 231-233
programs/magicblock/src/schedule_transactions/mod.rs (1)
4-14: LGTM: schedule commit finalize module is correctly wired and re-exported.The new module declaration and crate-level re-export look consistent with existing schedule transaction modules.
programs/magicblock/src/magicblock_processor.rs (1)
72-78: LGTM: ScheduleCommitFinalize dispatch wiring is clear and consistent.The new match arm cleanly routes to
process_schedule_commit_finalizewith the expected options.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs
Show resolved
Hide resolved
programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs
Show resolved
Hide resolved
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
ee0518e to
a25f866
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
196-208: Replace.expect()with proper error handling.The
.expect()call on line 199 violates the coding guidelines for production Rust code. While the comment explains the invariant, this should return a proper error instead of panicking.🔧 Proposed fix
- let tasks = accounts + let tasks: Result<Vec<_>, TaskBuilderError> = accounts .iter() .map(|account| { - let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!"); + let commit_id = *commit_ids.get(&account.pubkey).ok_or_else(|| { + error!("Missing commit_id for pubkey: {}", account.pubkey); + TaskBuilderError::CommitTasksBuildError( + TaskInfoFetcherError::MissingCommitId(account.pubkey) + ) + })?; // TODO (snawaz): if accounts do not have duplicate, then we can use remove // instead: // let base_account = base_accounts.remove(&account.pubkey); let base_account = base_accounts.get(&account.pubkey).cloned(); let task = if finalize { Self::create_commit_finalize_task(commit_id, allow_undelegation, account.clone(), base_account) } else { Self::create_commit_task(commit_id, allow_undelegation, account.clone(), base_account) }; - Box::new(task) as Box<dyn BaseTask> - }).collect(); + Ok(Box::new(task) as Box<dyn BaseTask>) + }).collect(); - Ok(tasks) + tasksNote: This requires adding a
MissingCommitId(Pubkey)variant toTaskInfoFetcherErroror using an appropriate existing error variant. As per coding guidelines,.unwrap()and.expect()in production code require proper error handling.magicblock-committor-service/src/tasks/buffer_task.rs (1)
119-130: Test-onlyFromimplementation doesn't handleCommitFinalize.The
From<ArgsTaskType>implementation forBufferTaskTypeonly handlesCommitandCommitDiff, withunimplemented!()for other variants. SinceArgsTaskType::CommitFinalizenow exists, tests that attempt to convert it will panic with a non-descriptive message.♻️ Suggested improvement
#[cfg(any(test, feature = "dev-context-only-utils"))] impl From<ArgsTaskType> for BufferTaskType { fn from(value: ArgsTaskType) -> Self { match value { ArgsTaskType::Commit(task) => BufferTaskType::Commit(task), ArgsTaskType::CommitDiff(task) => BufferTaskType::CommitDiff(task), + ArgsTaskType::CommitFinalize(task) => BufferTaskType::CommitFinalize(task), _ => unimplemented!( - "Only commit task can be BufferTask currently. Fix your tests" + "Only Commit, CommitDiff, and CommitFinalize tasks can be BufferTask. Fix your tests" ), } } }
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Around line 104-106: The Cargo.toml entry for the dependency
"magicblock-delegation-program" uses a local path "../delegation-program" which
points outside the repo; verify CI and developer machines have that sibling
directory available or change the setup: either document the required sibling
directory location and setup steps in README/CONTRIBUTING, or convert the
project to a Cargo workspace and add "../delegation-program" as a workspace
member (or replace the path with a published crate/version) so CI can resolve it
reliably; update any CI config to ensure the sibling directory is checked out
before cargo build.
In `@magicblock-committor-service/src/intent_executor/mod.rs`:
- Line 734: Replace the unstructured println!("prepared_message: {:#?}",
prepared_message) in intent_executor::mod.rs with the tracing macro to avoid
stdout noise; use the imported trace! macro to emit the same debug output (e.g.,
trace!("prepared_message: {:#?}", prepared_message) or trace!(?prepared_message)
depending on preferred formatting), ensuring the call remains in the same
location so the hot path uses structured logging and respects log-level
filtering.
In `@magicblock-committor-service/src/tasks/buffer_task.rs`:
- Around line 178-212: BufferTaskType::CommitFinalize currently recomputes the
data payload inline in the instruction builder using
compute_diff(base_account.data(), value.committed_account.account.data()) /
value.committed_account.account.data.clone(), duplicating the logic from
preparation_required (and differing from Commit/CommitDiff which use the buffer
PDA). Refactor by extracting the data assembly into a single helper (e.g., a
private fn build_commit_data(value: &CommitFinalizePayload) -> (Vec<u8>, i32))
and call it from both preparation_required and the
BufferTaskType::CommitFinalize arm in instruction (or, if CommitFinalize
intentionally bypasses buffers, at least call this helper there) so the diff
computation and data_is_diff flag are derived from one place (referencing
compute_diff, base_account, committed_account, and preparation_required).
In `@test-integration/Cargo.toml`:
- Around line 39-41: The Cargo.toml uses a path dependency
"ephemeral-rollups-sdk" pointing at ../../ephemeral-rollups-sdk/rust/sdk; update
the repo README or setup docs to state that the sibling ephemeral-rollups-sdk
repository must be cloned at that relative path (or provide alternative steps)
before running cargo build/test, include exact dependency name
"ephemeral-rollups-sdk" and the relative path used, and optionally document a
scripted setup (git clone instructions or a symlink step) so developers cloning
this repo in isolation won’t encounter missing-path build failures.
In `@test-integration/programs/schedulecommit/src/lib.rs`:
- Around line 580-593: The test contains a temporary branch guarded by a TODO
"UNDO THIS" that switches between commit_finalize_and_undelegate_accounts and
commit_finalize_accounts; remove or track this temporary change by either
deleting the alternate branch and always calling the intended function
(commit_finalize_accounts or commit_finalize_and_undelegate_accounts) or add a
TODO with a referenced follow-up issue/PR ID and a clear comment explaining when
to revert; update the call site in the function that currently contains the
conditional so only the chosen finalize function remains (or replace the ad‑hoc
TODO with a tracked issue reference) and run the tests to confirm behavior
remains correct.
In `@test-integration/run-magicblock`:
- Around line 1-12: The script uses relative paths (rm -rf
./test-ledger-magicblock and ../target/debug/magicblock-validator) which can be
dangerous if run from the wrong CWD; fix by anchoring operations to the script
directory (compute the script's dir via dirname/$BASH_SOURCE and use that base
for the test-ledger-magicblock path and for invoking magicblock-validator) so rm
and the binary invocation always target "$SCRIPT_DIR/test-ledger-magicblock" and
"$SCRIPT_DIR/../target/debug/magicblock-validator" (or change to that dir first)
instead of relying on the caller's working directory.
In `@test-integration/run-test-validator-for-schedulecommit.sh`:
- Around line 11-60: The script hard-codes absolute /Users/... paths for
deployed programs and account JSONs (seen in the --upgradeable-program entries
and --account entries); make these repo-relative or env-configurable by
resolving a repo root or script directory variable (e.g., REPO_ROOT or
SCRIPT_DIR derived from the script location) and use it to build paths to the
.so files and configs instead of literal /Users/... strings, or accept a
configurable env var (e.g., MB_PROJECT_ROOT) and fall back to a sensible
default; update every occurrence (all --upgradeable-program file arguments and
--account file arguments) to use the new variable-based paths.
In `@test-integration/run-validator.sh`:
- Around line 1-2: Add the missing shebang to the top of the run-validator.sh
script so it runs under the intended shell; insert the line "#!/usr/bin/env
bash" as the very first line of run-validator.sh to match the template generated
by tools/genx/src/test_validator.rs and ensure consistent execution environment.
♻️ Duplicate comments (5)
magicblock-committor-service/src/intent_executor/mod.rs (2)
163-167: TODO in execution path needs tracking (already noted).Duplicate of earlier review feedback—please link this TODO to a tracked issue.
190-277: Remove the commented-out finalize branch (already noted).Duplicate of earlier review feedback—please delete the dead block.
test-integration/Cargo.toml (1)
63-65: Prefer a workspace dependency formagicblock-delegation-program.programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs (1)
40-42: VerifyTransactionContext::clonesemantics.This clones
transaction_contextand subsequent operations use this clone. While the account mutations appear to go throughRefCell::borrow_mut()on accounts retrieved from the clone, please verify that this pattern is intentional and that mutations propagate correctly. The same pattern exists inprocess_schedule_commit.rs.magicblock-committor-service/src/tasks/mod.rs (1)
123-129: Consider addingDebugderive for observability consistency.
CommitFinalizeTaskonly derivesClone, whileCommitDiffTask(line 115) derives bothCloneandDebug. AddingDebugwould improve logging and troubleshooting capabilities for this new task type.♻️ Suggested change
-#[derive(Clone)] +#[derive(Clone, Debug)] pub struct CommitFinalizeTask {
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
a25f866 to
0991825
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
202-202: Replace.expect()with proper error handling.Per coding guidelines,
.expect()in production Rust code undermagicblock-*paths is a major issue. This should return an error or use a safer alternative.🔧 Proposed fix
- let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!"); + let commit_id = match commit_ids.get(&account.pubkey) { + Some(id) => *id, + None => { + error!("Missing commit_id for pubkey {}", account.pubkey); + return Err(TaskBuilderError::CommitTasksBuildError( + TaskInfoFetcherError::MissingCommitId(account.pubkey), + )); + } + };This requires adding a new
MissingCommitId(Pubkey)variant toTaskInfoFetcherError, or alternatively using an existing error variant that fits this case. The current.expect()could cause a panic in production if the invariant is violated.magicblock-committor-service/src/tasks/buffer_task.rs (1)
120-131: Consider addingCommitFinalizeto theFrom<ArgsTaskType>conversion.The conversion currently panics with
unimplemented!()for any variant other thanCommitandCommitDiff. While this is test/dev-only code, addingCommitFinalizewould prevent test failures if this conversion is used for the new task type.♻️ Proposed improvement
impl From<ArgsTaskType> for BufferTaskType { fn from(value: ArgsTaskType) -> Self { match value { ArgsTaskType::Commit(task) => BufferTaskType::Commit(task), ArgsTaskType::CommitDiff(task) => BufferTaskType::CommitDiff(task), + ArgsTaskType::CommitFinalize(task) => BufferTaskType::CommitFinalize(task), _ => unimplemented!( "Only commit task can be BufferTask currently. Fix your tests" ), } } }
🤖 Fix all issues with AI agents
In `@magicblock-committor-service/src/tasks/args_task.rs`:
- Line 97: Remove the stray debug println!("ArgsTaskType CommitFinalize");
statement introduced in the handling of the ArgsTaskType CommitFinalize case;
delete this println from the code (or replace it with the project's standard
logger call, e.g., trace!/debug! using the existing logging crate) so no raw
stdout debug prints remain in production.
- Line 187: Remove the debug println! from the CommitFinalize branch inside the
try_optimize_tx_size logic in args_task.rs; locate the
println!("try_optimize_tx_size CommitFinalize"); inside the try_optimize_tx_size
function (or the CommitFinalize match/arm) and delete it (or replace with an
appropriate structured log call if persistent logging is required, e.g., using
the crate's logger), ensuring no leftover debug prints remain.
In `@magicblock-committor-service/src/tasks/buffer_task.rs`:
- Line 180: Remove the stray debug println!("BufferTaskType CommitFinalize")
from the BufferTaskType::CommitFinalize handling in buffer_task.rs; delete the
println! call (or replace it with a structured log via the crate logger if
persistent runtime visibility is required) so no debug stdout is left in
production code.
- Line 95: Remove the debug println! statement println!("CommitFinalize
preparation_required"); (the temporary debug left in the CommitFinalize handling
code) — delete that line from buffer_task.rs and, if you need structured debug
output instead, replace it with the project's logging macro (e.g.
tracing::debug! or log::debug!) consistent with existing logging elsewhere;
ensure no raw println! calls remain in the commit finalization code.
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Line 68: Remove the debug println!("create_commit_task") from the
create_commit_task function (or the surrounding scope where it's declared) so no
debug stdout is emitted in production; if you need runtime visibility keep a
structured logger call (e.g., trace/debug via your crate's logger) instead of
println! and ensure any replacement uses the existing logging facility.
- Line 99: Remove the debug println! call in task_builder.rs (the
"println!(\"create_commit_finalize_task\")" inside the
create_commit_finalize_task flow); delete the println! or replace it with the
appropriate structured logging call (e.g., using the project's logger) so no raw
debug prints remain in production code, and ensure any necessary context is
logged via the existing logger used elsewhere in
TaskBuilder/TaskBuilder::create_commit_finalize_task.
0991825 to
a92678c
Compare
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/tasks/buffer_task.rs (1)
119-129: MissingCommitFinalizeinFrom<ArgsTaskType>conversion.The
From<ArgsTaskType>implementation doesn't handleArgsTaskType::CommitFinalize, which will cause a panic if this conversion is attempted in tests. SinceCommitFinalizecan be converted toBufferTaskviatry_optimize_tx_size, this conversion should be supported.🐛 Proposed fix
impl From<ArgsTaskType> for BufferTaskType { fn from(value: ArgsTaskType) -> Self { match value { ArgsTaskType::Commit(task) => BufferTaskType::Commit(task), ArgsTaskType::CommitDiff(task) => BufferTaskType::CommitDiff(task), + ArgsTaskType::CommitFinalize(task) => BufferTaskType::CommitFinalize(task), _ => unimplemented!( "Only commit task can be BufferTask currently. Fix your tests" ), } } }
🤖 Fix all issues with AI agents
In `@magicblock-committor-service/src/tasks/args_task.rs`:
- Around line 111-121: Summary: Boolean-to-u8 conversions are inconsistent —
`data_is_diff` uses `.map(|_| 1).unwrap_or(0)` while `allow_undelegation` uses
an `if` expression; make them consistent. Change both conversions to the same
pattern (recommended: use `.is_some()` for option checks and `if` for plain
bools) — e.g., replace `data_is_diff: task.base_account.as_ref().map(|_|
1).unwrap_or(0)` with `data_is_diff: if task.base_account.is_some() { 1 } else {
0 }`, and keep `allow_undelegation` as is (or conversely convert
`allow_undelegation` to `.then(|| 1).unwrap_or(0)` if you prefer the map/unwrap
style). Update the `data_is_diff` and `allow_undelegation` initializations
accordingly for consistent, readable boolean-to-u8 conversion.
In `@magicblock-committor-service/src/tasks/buffer_task.rs`:
- Around line 190-200: Inconsistent boolean-to-u8 conversion: replace the
map/unwrap pattern used for data_is_diff and the if/else for allow_undelegation
with consistent u8::from conversions; specifically, change the data_is_diff
assignment that currently uses task.base_account.as_ref().map(|_|
1).unwrap_or(0) to use u8::from(task.base_account.is_some()), and change the
allow_undelegation block that uses if task.allow_undelegation { 1 } else { 0 }
to u8::from(task.allow_undelegation) so both fields use the same conversion
pattern.
♻️ Duplicate comments (1)
test-integration/programs/schedulecommit/src/lib.rs (1)
607-614: Track or remove the temporary finalize/undelegate switch.The
TODO (snawaz): temporary change. UNDO THIScomment should be tied to a follow-up issue or removed once the intended path is ready.
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
a92678c to
c3ecd8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
323-385: Gate undelegation assertion on commit type.
The assertion at line 383 unconditionally checks for undelegation, butScheduleCommitType::CommitFinalizeshould not undelegate. This causestest_commit_finalize_huge_order_book_account()to fail. Condition the assertion on commit type—assert undelegation only forCommitAndUndelegateandCommitFinalizeAndUndelegate.🔧 Proposed fix
assert_one_committee_was_committed(&ctx, &res, true); - assert_one_committee_account_was_undelegated_on_chain(&ctx); + if matches!( + commit_type, + ScheduleCommitType::CommitAndUndelegate + | ScheduleCommitType::CommitFinalizeAndUndelegate + ) { + assert_one_committee_account_was_undelegated_on_chain(&ctx); + }
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
c3ecd8b to
97c1624
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
196-210: Replace.expect()with proper error handling.Per coding guidelines,
.expect()in production code undermagicblock-*is a major issue. While the comment explains the invariant, this should use proper error handling to avoid potential panics.🔧 Proposed fix
let tasks = accounts .iter() - .map(|account| { - let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!"); + .filter_map(|account| { + let commit_id = match commit_ids.get(&account.pubkey) { + Some(id) => *id, + None => { + error!("Missing commit_id for pubkey {}, this should not happen", account.pubkey); + return None; + } + }; // TODO (snawaz): if accounts do not have duplicate, then we can use remove // instead: // let base_account = base_accounts.remove(&account.pubkey); let base_account = base_accounts.get(&account.pubkey).cloned(); let task = if finalize { Self::create_commit_finalize_task(commit_id, allow_undelegation, account.clone(), base_account) } else { Self::create_commit_task(commit_id, allow_undelegation, account.clone(), base_account) }; - Box::new(task) as Box<dyn BaseTask> + Some(Box::new(task) as Box<dyn BaseTask>) }).collect();Alternatively, if the invariant is truly guaranteed by the upstream
fetch_next_commit_idscall, consider returning an error fromcommit_taskswhen a commit_id is missing:let commit_id = *commit_ids.get(&account.pubkey) .ok_or_else(|| TaskBuilderError::CommitTasksBuildError( TaskInfoFetcherError::MissingCommitId(account.pubkey) ))?;magicblock-committor-service/src/tasks/buffer_task.rs (1)
119-130: Update theFrom<ArgsTaskType>impl to handleCommitFinalize.The test/dev-only conversion doesn't handle
CommitFinalize, which will cause tests to panic if they attempt to convert aCommitFinalizeargs task to a buffer task. SinceBufferTaskType::CommitFinalizenow exists, this conversion should be supported.🔧 Proposed fix
#[cfg(any(test, feature = "dev-context-only-utils"))] impl From<ArgsTaskType> for BufferTaskType { fn from(value: ArgsTaskType) -> Self { match value { ArgsTaskType::Commit(task) => BufferTaskType::Commit(task), ArgsTaskType::CommitDiff(task) => BufferTaskType::CommitDiff(task), + ArgsTaskType::CommitFinalize(task) => BufferTaskType::CommitFinalize(task), _ => unimplemented!( - "Only commit task can be BufferTask currently. Fix your tests" + "Only commit/commit_finalize tasks can be BufferTask. Fix your tests" ), } } }
♻️ Duplicate comments (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
96-130: Instruction generation logic is correct.The conditional data computation (diff when
base_accountexists, raw data otherwise) and thecommit_finalizeinstruction call are properly implemented.Minor: The boolean-to-u8 conversion inconsistency (
.map(|_| 1).unwrap_or(0)vsif-else) was flagged in a previous review. Consider usingu8::from()for both for consistency.magicblock-committor-service/src/tasks/buffer_task.rs (1)
177-210: Instruction generation forCommitFinalizeis correctly implemented.The buffer PDA derivation and
commit_finalize_from_buffercall follow the established patterns.Minor: The boolean-to-u8 conversion inconsistency (
.map(|_| 1).unwrap_or(0)vsif-else) was flagged in a previous review. Consider usingu8::from()for both for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test-integration/programs/schedulecommit/src/lib.rs (1)
15-28: DuplicateProgramResultimport will cause a compilation error.
ProgramResultis imported from bothsolana_program::entrypoint(line 18) andsolana_program::entrypoint_deprecated(line 19). This creates a conflicting import.🐛 Proposed fix
use solana_program::{ account_info::{next_account_info, AccountInfo}, declare_id, entrypoint::{self, ProgramResult}, - entrypoint_deprecated::ProgramResult, instruction::{AccountMeta, Instruction}, msg, program::{invoke, invoke_signed}, program_error::ProgramError, pubkey::Pubkey, rent::Rent, system_instruction, sysvar::Sysvar, };test-integration/programs/schedulecommit/src/api.rs (1)
177-195: Compilation error: call site not updated after signature change.
schedule_commit_cpi_instruction_implnow accepts(commit_payer: bool, commit_type: ScheduleCommitType)but this call site still passesScheduleCommitCpiInstructionImplArgswhich no longer exists.🐛 Proposed fix
pub fn schedule_commit_cpi_instruction( payer: Pubkey, magic_program_id: Pubkey, magic_context_id: Pubkey, players: &[Pubkey], committees: &[Pubkey], ) -> Instruction { schedule_commit_cpi_instruction_impl( payer, magic_program_id, magic_context_id, players, committees, - ScheduleCommitCpiInstructionImplArgs { - undelegate: false, - commit_payer: false, - }, + false, + ScheduleCommitType::Commit, ) }
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
6a60456 to
3fa9ade
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs`:
- Around line 389-401: The match arm for ScheduleCommitType::Commit currently
panics (assert!(false)); replace that hard-fail with an assertion checking the
expected undelegation state for a plain Commit, e.g. call the existing helper
assert_one_committee_account_was_not_undelegated_on_chain(&ctx) so the helper
can be reused for Commit mode; keep the other ScheduleCommitType arms as-is.
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Show resolved
Hide resolved
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
3fa9ade to
3e08a68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test-integration/programs/schedulecommit/src/lib.rs`:
- Around line 164-212: The invoke_commit function currently ends with an
explicit Ok(()) after a match; instead make the match itself return
ProgramResult by having each arm end with Ok(()) (after the ? calls) and then
remove the trailing Ok(()) and the extra semicolon so the match is the final
expression. Update ScheduleCommitType::invoke_commit so each arm (e.g., Commit,
CommitAndUndelegate, CommitFinalize, CommitFinalizeAndUndelegate) ends with
Ok(()) and delete the final Ok(()) return.
♻️ Duplicate comments (1)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
390-401: Avoid hard-failing onScheduleCommitType::Commit.The
assert!(false)for theCommitvariant will unconditionally panic if this helper is ever called with that mode. This limits reusability and produces a confusing failure message. Consider asserting the expected undelegation state instead.🔧 Proposed fix
match commit_type { - ScheduleCommitType::Commit => { - assert!(false); - } - ScheduleCommitType::CommitFinalize => { + ScheduleCommitType::Commit | ScheduleCommitType::CommitFinalize => { assert_one_committee_account_was_not_undelegated_on_chain(&ctx); } ScheduleCommitType::CommitAndUndelegate | ScheduleCommitType::CommitFinalizeAndUndelegate => { assert_one_committee_account_was_undelegated_on_chain(&ctx); } }
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
3e08a68 to
1826c3f
Compare
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)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (2)
520-520: Compilation error: missingcommit_typeargument.The function
commit_and_undelegate_one_accountnow requires two arguments, but this call only provides one.🐛 Proposed fix
- let (ctx, sig, tx_res) = commit_and_undelegate_one_account(false); + let (ctx, sig, tx_res) = commit_and_undelegate_one_account(false, ScheduleCommitType::CommitAndUndelegate);
687-687: Compilation error: missingcommit_typeargument.Same issue as line 520 - missing the required
commit_typeargument.🐛 Proposed fix
- let (ctx, sig, tx_res) = commit_and_undelegate_one_account(true); + let (ctx, sig, tx_res) = commit_and_undelegate_one_account(true, ScheduleCommitType::CommitAndUndelegate);
🤖 Fix all issues with AI agents
In
`@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs`:
- Around line 53-55: The commit_and_undelegate_one_account function declares an
unused parameter commit_type; either remove this parameter from the signature or
make use of it by branching the logic based on its value and calling the
appropriate helper (e.g., pass commit_type into or select between
schedule_commit_and_undelegate_cpi_instruction and any alternate commit helper)
so the behavior reflects the selected ScheduleCommitType; update any callers
accordingly and ensure the function signature and internal calls reference
commit_type consistently.
- Around line 295-296: The test calls commit_and_undelegate_one_account passing
the enum type ScheduleCommitType instead of a variant; change the call in
commit_and_undelegate_one_account(...) to pass the appropriate variant
ScheduleCommitType::CommitAndUndelegate (reference the enum ScheduleCommitType
and the test helper function commit_and_undelegate_one_account to locate and
update the argument).
- Line 15: Remove the duplicate import of ScheduleCommitType from the import
list so the same symbol is only imported once; locate the import statement that
currently reads "BookUpdate, OrderLevel, ScheduleCommitType,
ScheduleCommitType," and delete the redundant ScheduleCommitType entry to avoid
the duplicate-import compilation error.
♻️ Duplicate comments (1)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
393-404: Avoid hard-failing onScheduleCommitType::Commit.The
assert!(false)forCommitwill cause an unconditional test failure if this helper is ever called with that variant. Consider handling it consistently withCommitFinalizesince neither should undelegate.🧩 Proposed fix
match commit_type { - ScheduleCommitType::Commit => { - assert!(false); - } - ScheduleCommitType::CommitFinalize => { + ScheduleCommitType::Commit | ScheduleCommitType::CommitFinalize => { assert_one_committee_account_was_not_undelegated_on_chain(&ctx); } ScheduleCommitType::CommitAndUndelegate | ScheduleCommitType::CommitFinalizeAndUndelegate => { assert_one_committee_account_was_undelegated_on_chain(&ctx); } }
| update_order_book_instruction, UserSeeds, | ||
| }, | ||
| BookUpdate, OrderLevel, FAIL_UNDELEGATION_COUNT, | ||
| BookUpdate, OrderLevel, ScheduleCommitType, ScheduleCommitType, |
There was a problem hiding this comment.
Duplicate import will cause compilation error.
ScheduleCommitType is imported twice on the same line.
🐛 Proposed fix
- BookUpdate, OrderLevel, ScheduleCommitType, ScheduleCommitType,
+ BookUpdate, OrderLevel, ScheduleCommitType,📝 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.
| BookUpdate, OrderLevel, ScheduleCommitType, ScheduleCommitType, | |
| BookUpdate, OrderLevel, ScheduleCommitType, |
🤖 Prompt for AI Agents
In
`@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs`
at line 15, Remove the duplicate import of ScheduleCommitType from the import
list so the same symbol is only imported once; locate the import statement that
currently reads "BookUpdate, OrderLevel, ScheduleCommitType,
ScheduleCommitType," and delete the redundant ScheduleCommitType entry to avoid
the duplicate-import compilation error.
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Outdated
Show resolved
Hide resolved
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Outdated
Show resolved
Hide resolved
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
1826c3f to
6f3e1fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
119-161: Consider adding tests forScheduleCommitType::CommitAndUndelegate.All
commit_book_order_accounttests only useScheduleCommitType::Commit. For consistency withcommit_single_account(which has separate*_and_undelegatetests), consider adding test cases that exerciseScheduleCommitType::CommitAndUndelegateto ensure full coverage of the new enum-driven flow.Would you like me to generate additional test functions for the
CommitAndUndelegatevariant?
163-167: Inconsistent parameter style between test helpers.
commit_single_accountstill uses a booleanundelegateparameter (line 166), whilecommit_book_order_accountnow usesScheduleCommitType. Consider unifying both helpers to useScheduleCommitTypefor consistency and to align with the new enum-driven flow introduced in this PR.♻️ Proposed signature change
async fn commit_single_account( bytes: usize, expected_strategy: CommitStrategy, - undelegate: bool, + commit_type: ScheduleCommitType, ) {Then update the
base_intentconstruction (lines 202-209) to use a match similar tocommit_book_order_account.
🤖 Fix all issues with AI agents
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 269-280: The match on ScheduleCommitType uses a catch-all `_ =>
todo!("not done yet")` which will panic if new variants are introduced; replace
the catch-all with explicit arms for the remaining variants (e.g.,
CommitFinalize and CommitFinalizeAndUndelegate) that construct the appropriate
MagicBaseIntent variants, or if those implementations are pending, replace the
todo! with an explicit TODO comment and a clear panic message (e.g.,
panic!("unhandled ScheduleCommitType: {:?}", schedule_commit_type)) so failures
are explicit, and add a tracking TODO/issue reference; update the match around
ScheduleCommitType and
MagicBaseIntent/CommitAndUndelegate/CommitType/UndelegateType accordingly.
♻️ Duplicate comments (3)
magicblock-committor-service/src/intent_executor/mod.rs (3)
163-167: Track the BaseActions TODO and fix the typo.Please file a follow-up issue for this TODO and replace it with a reference, and fix the “BaseActionse” typo to “BaseActions.” Happy to help draft the issue or a follow-up PR.
190-229: Remove the dead commented-out branch and verify empty-finalize handling.The commented-out flow adds noise; please remove it. Also verify
TaskStrategist::build_execution_strategynever selects a two-stage path whenfinalize_tasksis empty so you don’t emit an empty finalize transaction.♻️ Proposed cleanup
- // if base_intent.is_commit_finalize() { - // // Build tasks for commit & finalize stages - // let commit_tasks = TaskBuilderImpl::commit_tasks( - // &self.task_info_fetcher, - // &base_intent, - // persister, - // ) - // .await?; - - // // Build execution strategy - // match TaskStrategist::build_execution_strategy( - // commit_tasks, - // Vec::new(), - // &self.authority.pubkey(), - // persister, - // )? { - // StrategyExecutionMode::SingleStage(strategy) => { - // trace!("Executing intent in single stage"); - // self.single_stage_execution_flow( - // base_intent, - // strategy, - // persister, - // ) - // .await - // } - // StrategyExecutionMode::TwoStage { - // commit_stage, - // finalize_stage, - // } => { - // trace!("Executing intent in two stages"); - // self.two_stage_execution_flow( - // &committed_pubkeys, - // commit_stage, - // finalize_stage, - // persister, - // ) - // .await - // } - // } - // } else { // Build tasks for commit & finalize stagesTo inspect the strategy logic:
#!/bin/bash # Locate TaskStrategist::build_execution_strategy and verify empty-finalize behavior. rg -nP --type=rust 'fn\s+build_execution_strategy' -C3 rg -nP --type=rust 'finalize_tasks|StrategyExecutionMode::TwoStage' -C3
734-734: Usetracinginstead ofprintln!in the hot path.Swap to
trace!(already imported) so logs are structured and respect log-level filtering.♻️ Suggested change
- println!("prepared_message: {:?}", prepared_message); + trace!("prepared_message: {:?}", prepared_message);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 121-126: Add test coverage for the other ScheduleCommitType
variants by invoking commit_book_order_account with
ScheduleCommitType::CommitFinalize and
ScheduleCommitType::CommitFinalizeAndUndelegate in the same spots where you
currently call ScheduleCommitType::Commit (e.g., the calls around the existing
blocks that call commit_book_order_account). Update the assertions after each
call to validate the expected finalization and undelegate behaviors for those
paths (use the same verification helpers/assertions used for Commit), and repeat
this for the other occurrences noted (the calls around lines 131-136, 145-150,
and 155-160) so all three ScheduleCommitType variants are exercised; reference
the commit_book_order_account function and the ScheduleCommitType enum to locate
and add the new scenarios.
♻️ Duplicate comments (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
269-279: Avoid catch‑alltodo!for new commit variants.
The_arm will panic for new variants; make the match explicit so the compiler forces updates.🛠️ Suggested change
- _ => todo!("not done yet"), + ScheduleCommitType::CommitFinalize + | ScheduleCommitType::CommitFinalizeAndUndelegate => { + todo!("not done yet") + }
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
89d94b4 to
75086b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test-integration/run-test-validator-for-schedulecommit.sh`:
- Around line 1-4: Add shell error handling to the script by enabling immediate
exit on errors and failures from pipelines before running commands like rm -rf
test-ledger and solana-test-validator; update the top of the script (after the
#!/bin/bash shebang) to enable strict error handling (e.g., set -e, optionally
set -o pipefail and set -u) so the script stops on any command failure and
avoids silent test setup failures.
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 857-862: Replace the println! call in the test (the line printing
"{matches_data} / {matches_undelegation} - {} == {}" with acc.owner() and
expected_owner) with a tracing macro (e.g., tracing::info! or tracing::debug!)
and ensure the test module imports the tracing macro (use tracing::info; or
tracing::debug;) so logging is consistent with the project's tracing setup; keep
the existing interpolation/arguments and message text when converting to the
tracing macro.
♻️ Duplicate comments (5)
test-integration/run-test-validator-for-schedulecommit.sh (1)
5-60: Hard-coded absolute paths need to be made portable.The absolute
/Users/snawaz/...paths will fail on other machines and in CI environments. This issue was already flagged in a previous review with a comprehensive fix usingSCRIPT_DIRandROOT_DIRvariables.magicblock-committor-service/src/intent_executor/mod.rs (2)
163-167: Track the BaseActions TODO with an issue link.Line 163-167 leaves a TODO in the hot path; please link it to a tracking issue (or replace with a short issue reference) so it doesn’t get lost. Happy to draft the issue if you want.
190-229: Remove the commented-out commit-finalize branch.Lines 190-229 and Line 277 are dead code. Please delete the block to keep the execution flow readable; VCS preserves history.
♻️ Proposed cleanup
- // if base_intent.is_commit_finalize() { - // // Build tasks for commit & finalize stages - // let commit_tasks = TaskBuilderImpl::commit_tasks( - // &self.task_info_fetcher, - // &base_intent, - // persister, - // ) - // .await?; - - // // Build execution strategy - // match TaskStrategist::build_execution_strategy( - // commit_tasks, - // Vec::new(), - // &self.authority.pubkey(), - // persister, - // )? { - // StrategyExecutionMode::SingleStage(strategy) => { - // trace!("Executing intent in single stage"); - // self.single_stage_execution_flow( - // base_intent, - // strategy, - // persister, - // ) - // .await - // } - // StrategyExecutionMode::TwoStage { - // commit_stage, - // finalize_stage, - // } => { - // trace!("Executing intent in two stages"); - // self.two_stage_execution_flow( - // &committed_pubkeys, - // commit_stage, - // finalize_stage, - // persister, - // ) - // .await - // } - // } - // } else { ... - //}Also applies to: 277-277
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
121-126: Add coverage for CommitFinalize variants in these test cases.
Only Commit / CommitAndUndelegate are exercised here; include CommitFinalize and CommitFinalizeAndUndelegate scenarios.Also applies to: 131-136, 145-150, 155-160
270-280: Replace thetodo!catch‑all for ScheduleCommitType.
Unhandled variants will panic when CommitFinalize paths are used.
test-integration/test-committor-service/tests/test_ix_commit_local.rs
Outdated
Show resolved
Hide resolved
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
75086b4 to
e6e4ca9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (2)
44-66: Consider extracting the repeated CommitMeta construction.The
Commit,CommitDiff, andCommitFinalizebranches have identical logic for constructingCommitMeta. This duplication could be reduced if the task types share a common trait or accessor.♻️ Optional: Extract common logic
If the commit-related task types implement a common trait (e.g.,
HasCommittedAccount), the visitor could be simplified:// If a trait like this exists or could be added: // trait HasCommittedAccount { // fn committed_account(&self) -> &CommittedAccountInfo; // fn commit_id(&self) -> u64; // } // Then the match could become: match &task.task_type { ArgsTaskType::Commit(t) | ArgsTaskType::CommitDiff(t) | ArgsTaskType::CommitFinalize(t) => { *commit_meta = Some(CommitMeta { committed_pubkey: t.committed_account.pubkey, commit_id: t.commit_id, remote_slot: t.committed_account.remote_slot, }) } _ => *commit_meta = None, }This is optional—the current explicit match arms are clear and work correctly.
72-94: Same pattern applies to BufferTaskType handling.The
BufferTaskType::CommitFinalizebranch mirrors the duplication noted above.magicblock-committor-service/src/tasks/task_builder.rs (1)
196-210: Use of.expect()in production code violates coding guidelines.Line 199 uses
.expect()which should be avoided in production Rust code undermagicblock-*paths. Although the comment suggests this is an invariant, proper error handling should be used.🔧 Proposed fix
let tasks = accounts .iter() .map(|account| { - let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!"); + let commit_id = *commit_ids.get(&account.pubkey).ok_or_else(|| { + error!(pubkey = %account.pubkey, "Missing commit_id for account"); + TaskBuilderError::CommitTasksBuildError( + TaskInfoFetcherError::MissingCommitId(account.pubkey) + ) + })?;Note: This would require changing the closure to return a
Resultand collecting withtry_collect()or similar, and potentially adding aMissingCommitIdvariant toTaskInfoFetcherError.As per coding guidelines,
.expect()in production code undermagicblock-*paths should be treated as a major issue requiring proper error handling or explicit justification with invariants.
🤖 Fix all issues with AI agents
In `@test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs`:
- Around line 244-261: The second assertion in
assert_account_was_not_undelegated_on_chain is redundant because owner ==
DELEGATION_PROGRAM_ID already guarantees owner != program_id unless program_id
equals DELEGATION_PROGRAM_ID; update the function by removing the
assert_ne!(owner, program_id, ...) or, if you intended to ensure the provided
program_id is not the delegation program, replace that assertion with a check on
program_id (e.g., assert_ne!(program_id, DELEGATION_PROGRAM_ID, ...)) so the
intent is explicit; keep the primary ownership check using
fetch_chain_account_owner and DELEGATION_PROGRAM_ID.
♻️ Duplicate comments (12)
test-integration/run-magicblock (1)
1-12: Anchor paths to the script directory to avoid accidental deletes.The script uses relative paths (
./test-ledger-magicblock,../target/debug/magicblock-validator) which assume the caller's CWD istest-integration/. This can cause issues if invoked from a different directory.♻️ Suggested adjustment
#!/usr/bin/env bash set -xeuo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + -rm -rf ./test-ledger-magicblock +rm -rf "${SCRIPT_DIR}/test-ledger-magicblock" # solana airdrop 20 mAGicPQYBMvcYveUZA5F5UNNwyHvfYh5xkLS2Fr1mev -solana airdrop 100 tEsT3eV6RFCWs1BZ7AXTzasHqTtMnMLCB2tjQ42TDXD --url "http://127.0.0.1:7799" -solana airdrop 100 mAGicPQYBMvcYveUZA5F5UNNwyHvfYh5xkLS2Fr1mev --url "http://127.0.0.1:7799" +solana airdrop 100 tEsT3eV6RFCWs1BZ7AXTzasHqTtMnMLCB2tjQ42TDXD --url "http://127.0.0.1:7799" +solana airdrop 100 mAGicPQYBMvcYveUZA5F5UNNwyHvfYh5xkLS2Fr1mev --url "http://127.0.0.1:7799" # RUST_LOG=info ephemeral-validator --accounts-lifecycle ephemeral --rpc-port 7799 --remote-url "http://localhost:8899" -RUST_LOG=info ../target/debug/magicblock-validator --lifecycle ephemeral --listen 0.0.0.0:8899 --remotes "http://localhost:7799" +RUST_LOG=info "${SCRIPT_DIR}/../target/debug/magicblock-validator" --lifecycle ephemeral --listen 0.0.0.0:8899 --remotes "http://localhost:7799"test-integration/run-test-validator-for-schedulecommit.sh (2)
1-4: Add a shebang and shell error handling.The script lacks a shebang and error handling (
set -e), which can cause silent failures during test setup.♻️ Suggested fix
-#!/bin/bash +#!/usr/bin/env bash +set -euo pipefail rm -rf test-ledger
11-60: Replace hard-coded absolute paths with repo-relative or env-configurable paths.The
/Users/snawaz/...paths are developer-specific and will fail in CI or on other machines.♻️ Proposed fix
#!/usr/bin/env bash +set -euo pipefail + +SCRIPT_DIR="$(cd -- "$(dirname "$0")" && pwd)" +ROOT_DIR="$(cd -- "$SCRIPT_DIR/.." && pwd)" +DELEGATION_PROGRAM_DIR="${DELEGATION_PROGRAM_DIR:-"$ROOT_DIR/../delegation-program"}" -rm -rf test-ledger +rm -rf "$SCRIPT_DIR/test-ledger" solana-test-validator \ --rpc-port \ 7799 \ -r \ --limit-ledger-size \ 10000 \ --upgradeable-program \ DELeGGvXpWV2fqJUhqcF5ZSYMS4JTLjteaAMARRSaeSh \ - /Users/snawaz/projects/mb/delegation-program/target/deploy/dlp.so \ + "$DELEGATION_PROGRAM_DIR/target/deploy/dlp.so" \ none \ --upgradeable-program \ DmnRGfyyftzacFb1XadYhWF6vWqXwtQk5tbr6XgR3BA1 \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/schedulecommit/elfs/mdp.so \ + "$SCRIPT_DIR/schedulecommit/elfs/mdp.so" \ none \ # ... apply similar changes to all other absolute pathstest-integration/run-validator.sh (1)
1-2: Add a shebang so the script runs under the intended shell.Without it, executing the file directly may default to
/bin/shor fail depending on environment. The static analysis tool also flags this (SC2148).♻️ Proposed fix
- +#!/usr/bin/env bash +set -euo pipefail + rm -rf ./test-ledgermagicblock-committor-service/src/tasks/mod.rs (1)
123-129: Consider addingDebugderive for observability.The struct design is sound, with clear semantics documented in the comment. However,
CommitDiffTaskderivesDebugwhile this struct does not, creating inconsistency that affects logging and troubleshooting capabilities.♻️ Suggested change
-#[derive(Clone)] +#[derive(Clone, Debug)] pub struct CommitFinalizeTask {magicblock-committor-service/src/intent_executor/mod.rs (2)
163-167: Track the TODO with an issue reference.The TODO comment about
MagicBaseIntent::BaseActionsscenario remains untracked. Consider filing an issue and referencing it here to prevent this from being forgotten.
190-229: Remove the commented-out code block.This large commented-out block adds noise and maintenance burden. The past review indicated this was "addressed," but the code is still present. Please delete it.
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
390-393: Avoid panic! for unimplemented ScheduleCommitType::Commit.The
panic!will break if this helper is reused forCommitmode. Consider asserting the expected undelegation state instead.🧩 Suggested fix
match commit_type { - ScheduleCommitType::Commit => { - panic!("ScheduleCommitType::Commit is not implemented"); - } + ScheduleCommitType::Commit | ScheduleCommitType::CommitFinalize => { + assert_one_committee_account_was_not_undelegated_on_chain(&ctx); + } - ScheduleCommitType::CommitFinalize => { - assert_one_committee_account_was_not_undelegated_on_chain(&ctx); - } ScheduleCommitType::CommitAndUndelegate | ScheduleCommitType::CommitFinalizeAndUndelegate => { assert_one_committee_account_was_undelegated_on_chain(&ctx); } }programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs (1)
40-42: VerifyTransactionContext::clonesemantics before mutating accounts.Line 40 clones
transaction_contextand all subsequent account mutations (e.g., undelegation marking, MagicContext state updates) go through that clone. IfTransactionContext::cloneis deep, these mutations will not persist to the real invoke context. Even if it's shallow, cloning here is potentially expensive on a hot path. Please confirm clone semantics and consider borrowing with tighter scopes if possible.test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
233-301: Add coverage for CommitFinalize variants and address thetodo!().The
todo!("not done yet")at line 280 will panic ifCommitFinalizeorCommitFinalizeAndUndelegatevariants are passed. Consider adding explicit handling for these variants to validate the new paths introduced in this PR.♻️ Proposed fix to handle all variants
let base_intent = match commit_type { ScheduleCommitType::CommitAndUndelegate => { MagicBaseIntent::CommitAndUndelegate(CommitAndUndelegate { commit_action: CommitType::Standalone(vec![account]), undelegate_action: UndelegateType::Standalone, }) } ScheduleCommitType::Commit => { MagicBaseIntent::Commit(CommitType::Standalone(vec![account])) } - _ => todo!("not done yet"), + ScheduleCommitType::CommitFinalize => { + MagicBaseIntent::CommitFinalize(CommitType::Standalone(vec![account])) + } + ScheduleCommitType::CommitFinalizeAndUndelegate => { + MagicBaseIntent::CommitFinalizeAndUndelegate(CommitAndUndelegate { + commit_action: CommitType::Standalone(vec![account]), + undelegate_action: UndelegateType::Standalone, + }) + } };
856-862: Prefer tracing macros overprintln!in tests.Keeps output controlled and consistent with the existing logging style used elsewhere in the test file.
♻️ Suggested change
- println!( + debug!( "{matches_data} / {matches_undelegation} - {} == {}", acc.owner(), expected_owner );magicblock-committor-service/src/tasks/args_task.rs (1)
96-130: Minor inconsistency in boolean-to-u8 conversion patterns.
data_is_diffuses.map(|_| 1).unwrap_or(0)whileallow_undelegationusesif-else. Consider using a consistent pattern for clarity.♻️ Suggested refactor for consistency
- data_is_diff: task - .base_account - .as_ref() - .map(|_| 1) - .unwrap_or(0), - - allow_undelegation: if task.allow_undelegation { - 1 - } else { - 0 - }, + data_is_diff: u8::from(task.base_account.is_some()), + allow_undelegation: u8::from(task.allow_undelegation),
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
119-130: Update theFrom<ArgsTaskType>impl to handleCommitFinalize.The conversion from
ArgsTaskType::CommitFinalizeis missing. SinceBufferTaskType::CommitFinalizenow exists andtry_optimize_tx_sizeinargs_task.rsconvertsArgsTaskType::CommitFinalizetoBufferTaskType::CommitFinalize, thisFromimpl should be updated for completeness in test/dev contexts.🔧 Proposed fix
impl From<ArgsTaskType> for BufferTaskType { fn from(value: ArgsTaskType) -> Self { match value { ArgsTaskType::Commit(task) => BufferTaskType::Commit(task), ArgsTaskType::CommitDiff(task) => BufferTaskType::CommitDiff(task), + ArgsTaskType::CommitFinalize(task) => BufferTaskType::CommitFinalize(task), _ => unimplemented!( "Only commit task can be BufferTask currently. Fix your tests" ), } } }
♻️ Duplicate comments (1)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
186-211: Minor inconsistency in boolean-to-u8 conversion patterns persists.The
data_is_difffield uses.map(|_| 1).unwrap_or(0)whileallow_undelegationusesif-else. For consistency, consider usingu8::from()for both conversions as suggested in the previous review.
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
103870e to
a759cfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@magicblock-committor-service/src/tasks/buffer_task.rs`:
- Around line 31-35: The cfg(test/dev) conversion from ArgsTaskType to
BufferTaskType currently panics for the CommitFinalize variant; update the
conversion logic to handle ArgsTaskType::CommitFinalize by returning
BufferTaskType::CommitFinalize constructed from the equivalent
CommitFinalizeTask (use the same conversion used for Commit and CommitDiff), and
mirror this change in the other conversion block that still panics on
CommitFinalize (the other impl handling ArgsTaskType→BufferTaskType). Ensure you
reference BufferTaskType::CommitFinalize and the CommitFinalizeTask conversion
path so tests/utilities no longer panic.
a759cfa to
a2632b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
139-146: Keep the log-matching comment in sync with the CommitFinalize check.
Behavior now treatsCommitFinalizeas a match forCommitState, but the comment still mentions onlyCommitState/CommitDiff.✏️ Suggested comment update
- // could invoke CommitState or CommitDiff depending on the size of the account, we also look for "CommitDiff" - // in the logs when needle == CommitState. It's easier to make this little adjustment here than computing - // the decision and passing either CommitState or CommitDiff from the tests themselves. + // could invoke CommitState, CommitDiff, or CommitFinalize depending on the size/flow, so we also look for + // "CommitDiff" or "CommitFinalize" in the logs when needle == CommitState. It's easier to make this little + // adjustment here than computing the exact variant in each test.
♻️ Duplicate comments (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
937-945: Usetracing::debug!instead ofprintln!for test diagnostics.
Keeps logs consistent with the tracing setup.♻️ Suggested change
- println!( + debug!( "{matches_data} / {matches_undelegation} - {} == {}", acc.owner(), expected_owner );
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
db9b213 to
4bb4e53
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test-integration/programs/schedulecommit/src/lib.rs`:
- Around line 160-162: Update the doc comment for the enum variant
ScheduleCommitForOrderBook to remove the outdated "ScheduleCommitDiffCpi" text
and describe that this variant accepts a ScheduleCommitType and is used to
schedule commits for an order book (not diff-specific); locate the comment
directly above the ScheduleCommitForOrderBook variant and replace it with a
concise description mentioning ScheduleCommitType and the variant's purpose.
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 262-283: The match that maps a ScheduleCommitType to a
MagicBaseIntent is duplicated in commit_single_account and
commit_book_order_account; extract this logic into a small helper function
(e.g., fn build_base_intent(commit_type: ScheduleCommitType, account:
AccountType) -> MagicBaseIntent) and replace the duplicated matches with calls
to that helper; ensure the helper covers all ScheduleCommitType variants
(Commit, CommitAndUndelegate, CommitFinalize, CommitFinalizeAndUndelegate) and
constructs the same MagicBaseIntent shapes (Commit(CommitType::Standalone),
CommitAndUndelegate(CommitAndUndelegate { commit_action: CommitType::Standalone,
undelegate_action: UndelegateType::Standalone }), etc.) so both
commit_single_account and commit_book_order_account call
build_base_intent(commit_type, account).
In `@test-integration/test-committor-service/tests/utils/transactions.rs`:
- Around line 143-146: The check in transactions.rs conflates "CommitState" with
"CommitFinalize" by returning true for either when needle == "CommitState";
update the logic in the function that evaluates logs (the block referencing
needle, CommitState, CommitDiff, CommitFinalize) so it no longer treats
CommitFinalize as a match for "CommitState". Either add a new explicit
branch/variant (e.g., "CommitStateOrFinalize") that callers can use when both
are acceptable, or change callers to pass "CommitFinalize" when that is the
expected outcome and keep the "CommitState" case only matching CommitState (and
optionally CommitDiff) to preserve assertion specificity.
♻️ Duplicate comments (3)
magicblock-committor-service/src/intent_executor/mod.rs (3)
163-164: Track the TODO comment with an issue.This TODO about
MagicBaseIntent::BaseActionsbeing handled in the single-stage execution path should be tracked with a follow-up issue to ensure it doesn't get forgotten.
188-227: Remove the commented-out code block.This ~40-line commented-out block deactivates the
is_commit_finalize()two-stage path but retains it as dead code. Commented-out code should be removed from the codebase — version control preserves history if it's needed later.If this logic is intentionally disabled temporarily pending further work, please track it in an issue and remove the commented code.
♻️ Proposed fix: remove dead code
- // if base_intent.is_commit_finalize() { - // // Build tasks for commit & finalize stages - // let commit_tasks = TaskBuilderImpl::commit_tasks( - // &self.task_info_fetcher, - // &base_intent, - // persister, - // ) - // .await?; - - // // Build execution strategy - // match TaskStrategist::build_execution_strategy( - // commit_tasks, - // Vec::new(), - // &self.authority.pubkey(), - // persister, - // )? { - // StrategyExecutionMode::SingleStage(strategy) => { - // trace!("Executing intent in single stage"); - // self.single_stage_execution_flow( - // base_intent, - // strategy, - // persister, - // ) - // .await - // } - // StrategyExecutionMode::TwoStage { - // commit_stage, - // finalize_stage, - // } => { - // trace!("Executing intent in two stages"); - // self.two_stage_execution_flow( - // &committed_pubkeys, - // commit_stage, - // finalize_stage, - // persister, - // ) - // .await - // } - // } - // } else { // Build tasks for commit & finalize stages
275-275: Remove the trailing commented brace.This commented closing brace is a remnant of the deactivated
is_commit_finalize()block and should be removed along with the commented code above.♻️ Proposed fix
- //}
| let base_intent = match commit_type { | ||
| ScheduleCommitType::Commit => { | ||
| MagicBaseIntent::Commit(CommitType::Standalone(vec![account])) | ||
| } | ||
| ScheduleCommitType::CommitAndUndelegate => { | ||
| MagicBaseIntent::CommitAndUndelegate(CommitAndUndelegate { | ||
| commit_action: CommitType::Standalone(vec![account]), | ||
| undelegate_action: UndelegateType::Standalone, | ||
| }) | ||
| } | ||
| ScheduleCommitType::CommitFinalize => { | ||
| MagicBaseIntent::CommitFinalize(CommitType::Standalone(vec![ | ||
| account, | ||
| ])) | ||
| } | ||
| ScheduleCommitType::CommitFinalizeAndUndelegate => { | ||
| MagicBaseIntent::CommitFinalizeAndUndelegate(CommitAndUndelegate { | ||
| commit_action: CommitType::Standalone(vec![account]), | ||
| undelegate_action: UndelegateType::Standalone, | ||
| }) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DRY: extract a base-intent builder for ScheduleCommitType
The match commit_type → MagicBaseIntent mapping is duplicated in commit_single_account and commit_book_order_account. A small helper would prevent drift as new variants are added.
♻️ Suggested refactor
+fn base_intent_for_commit_type(
+ commit_type: ScheduleCommitType,
+ account: CommittedAccount,
+) -> MagicBaseIntent {
+ match commit_type {
+ ScheduleCommitType::Commit => {
+ MagicBaseIntent::Commit(CommitType::Standalone(vec![account]))
+ }
+ ScheduleCommitType::CommitAndUndelegate => {
+ MagicBaseIntent::CommitAndUndelegate(CommitAndUndelegate {
+ commit_action: CommitType::Standalone(vec![account]),
+ undelegate_action: UndelegateType::Standalone,
+ })
+ }
+ ScheduleCommitType::CommitFinalize => {
+ MagicBaseIntent::CommitFinalize(CommitType::Standalone(vec![account]))
+ }
+ ScheduleCommitType::CommitFinalizeAndUndelegate => {
+ MagicBaseIntent::CommitFinalizeAndUndelegate(CommitAndUndelegate {
+ commit_action: CommitType::Standalone(vec![account]),
+ undelegate_action: UndelegateType::Standalone,
+ })
+ }
+ }
+}
...
- let base_intent = match commit_type {
- ScheduleCommitType::Commit => {
- MagicBaseIntent::Commit(CommitType::Standalone(vec![account]))
- }
- ScheduleCommitType::CommitAndUndelegate => {
- MagicBaseIntent::CommitAndUndelegate(CommitAndUndelegate {
- commit_action: CommitType::Standalone(vec![account]),
- undelegate_action: UndelegateType::Standalone,
- })
- }
- ScheduleCommitType::CommitFinalize => {
- MagicBaseIntent::CommitFinalize(CommitType::Standalone(vec![
- account,
- ]))
- }
- ScheduleCommitType::CommitFinalizeAndUndelegate => {
- MagicBaseIntent::CommitFinalizeAndUndelegate(CommitAndUndelegate {
- commit_action: CommitType::Standalone(vec![account]),
- undelegate_action: UndelegateType::Standalone,
- })
- }
- };
+ let base_intent = base_intent_for_commit_type(commit_type, account);
...
- let base_intent = match commit_type {
- ScheduleCommitType::Commit => {
- MagicBaseIntent::Commit(CommitType::Standalone(vec![account]))
- }
- ScheduleCommitType::CommitAndUndelegate => {
- MagicBaseIntent::CommitAndUndelegate(CommitAndUndelegate {
- commit_action: CommitType::Standalone(vec![account]),
- undelegate_action: UndelegateType::Standalone,
- })
- }
- ScheduleCommitType::CommitFinalize => {
- MagicBaseIntent::CommitFinalize(CommitType::Standalone(vec![
- account,
- ]))
- }
- ScheduleCommitType::CommitFinalizeAndUndelegate => {
- MagicBaseIntent::CommitFinalizeAndUndelegate(CommitAndUndelegate {
- commit_action: CommitType::Standalone(vec![account]),
- undelegate_action: UndelegateType::Standalone,
- })
- }
- };
+ let base_intent = base_intent_for_commit_type(commit_type, account);Also applies to: 344-365
🤖 Prompt for AI Agents
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs` around
lines 262 - 283, The match that maps a ScheduleCommitType to a MagicBaseIntent
is duplicated in commit_single_account and commit_book_order_account; extract
this logic into a small helper function (e.g., fn build_base_intent(commit_type:
ScheduleCommitType, account: AccountType) -> MagicBaseIntent) and replace the
duplicated matches with calls to that helper; ensure the helper covers all
ScheduleCommitType variants (Commit, CommitAndUndelegate, CommitFinalize,
CommitFinalizeAndUndelegate) and constructs the same MagicBaseIntent shapes
(Commit(CommitType::Standalone), CommitAndUndelegate(CommitAndUndelegate {
commit_action: CommitType::Standalone, undelegate_action:
UndelegateType::Standalone }), etc.) so both commit_single_account and
commit_book_order_account call build_base_intent(commit_type, account).
| if needle == "CommitState" { | ||
| log.contains(needle) || log.contains("CommitDiff") | ||
| log.contains(needle) | ||
| || log.contains("CommitDiff") | ||
| || log.contains("CommitFinalize") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid conflating CommitFinalize with CommitState checks
Treating CommitFinalize as a hit for needle == "CommitState" reduces assertion specificity and can mask missing CommitState/CommitDiff executions. Consider adding an explicit variant (e.g., CommitStateOrFinalize) or updating callers to pass "CommitFinalize" when that’s the real expectation.
🤖 Prompt for AI Agents
In `@test-integration/test-committor-service/tests/utils/transactions.rs` around
lines 143 - 146, The check in transactions.rs conflates "CommitState" with
"CommitFinalize" by returning true for either when needle == "CommitState";
update the logic in the function that evaluates logs (the block referencing
needle, CommitState, CommitDiff, CommitFinalize) so it no longer treats
CommitFinalize as a match for "CommitState". Either add a new explicit
branch/variant (e.g., "CommitStateOrFinalize") that callers can use when both
are acceptable, or change callers to pass "CommitFinalize" when that is the
expected outcome and keep the "CommitState" case only matching CommitState (and
optionally CommitDiff) to preserve assertion specificity.
4bb4e53 to
d061cbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs`:
- Around line 216-229: The code mutates account state (marks accounts as
undelegating) before attempting MagicContext::deserialize, which can leave state
inconsistent if deserialization fails; move the call to
get_instruction_account_with_idx(..., MAGIC_CONTEXT_IDX) and the
MagicContext::deserialize(...) to occur and succeed before performing any
account mutations, so that you only alter account state after successful
deserialization; ensure the error path from MagicContext::deserialize returns
early without applying mutations and keep the existing ic_msg! logging and
InstructionError mapping.
In `@test-integration/run-validator.sh`:
- Around line 17-31: The BPF program path entries are inconsistent (e.g.,
../target/deploy/magicblock_committor_program.so vs
./target/deploy/program_flexi_counter.so and
./target/deploy/program_schedulecommit.so) which breaks resolution depending on
the working directory; pick a single convention and update all --bpf-program
arguments to match it (either change the ./target/deploy/* entries to
../target/deploy/* to be repo-root relative, or change
../target/deploy/magicblock_committor_program.so to
./target/deploy/magicblock_committor_program.so to be test-integration
relative), leaving the already-correct ./schedulecommit/elfs/dlp.so path intact.
Ensure every occurrence of ./target/deploy/* or ../target/deploy/* in the script
is updated consistently so all referenced .so files resolve from the same
working directory.
♻️ Duplicate comments (13)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
139-146: Avoid treating CommitFinalize as CommitState in log checks.This change broadens the
needle == "CommitState"match to includeCommitFinalize, which can mask missingCommitState/CommitDifflogs and weaken assertions. Consider a distinct expectation (e.g.,CommitStateOrFinalize) or require callers to pass"CommitFinalize"when that is intended.magicblock-committor-service/src/intent_executor/mod.rs (2)
163-164: Track this TODO with a follow-up issue.The TODO comment identifies a naming/conceptual mismatch (BaseActions vs Commit scenario) that should be tracked externally rather than left as an inline comment that may be forgotten.
188-227: Remove commented-out code before merging.This ~40-line commented-out block adds noise and maintenance burden. If needed for reference, it's preserved in git history. The active code path (lines 228-274) already handles both single-stage and two-stage execution via
build_execution_strategy.🧹 Suggested removal
- // if base_intent.is_commit_finalize() { - // // Build tasks for commit & finalize stages - // let commit_tasks = TaskBuilderImpl::commit_tasks( - // &self.task_info_fetcher, - // &base_intent, - // persister, - // ) - // .await?; - - // // Build execution strategy - // match TaskStrategist::build_execution_strategy( - // commit_tasks, - // Vec::new(), - // &self.authority.pubkey(), - // persister, - // )? { - // StrategyExecutionMode::SingleStage(strategy) => { - // trace!("Executing intent in single stage"); - // self.single_stage_execution_flow( - // base_intent, - // strategy, - // persister, - // ) - // .await - // } - // StrategyExecutionMode::TwoStage { - // commit_stage, - // finalize_stage, - // } => { - // trace!("Executing intent in two stages"); - // self.two_stage_execution_flow( - // &committed_pubkeys, - // commit_stage, - // finalize_stage, - // persister, - // ) - // .await - // } - // } - // } else { // Build tasks for commit & finalize stages let (commit_tasks, finalize_tasks) = {And at the end (line 275):
} - //} }Also applies to: 275-275
test-integration/run-magicblock (1)
1-12: Relative paths should be anchored to script directory.The use of relative paths (
./test-ledger-magicblock,../target/debug/magicblock-validator) can cause issues if the script is executed from an unexpected working directory. This was previously flagged and remains unaddressed.test-integration/run-validator.sh (1)
1-2: Missing shebang line.The script lacks a shebang (
#!/usr/bin/env bash), which means execution behavior depends on the invoking shell. Static analysis also flags this as SC2148. This was previously flagged and remains unaddressed.programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs (1)
40-42: VerifyTransactionContext::clone()semantics.The
transaction_contextis cloned on line 40, and subsequent operations reference this clone. However,mark_account_as_undelegated(acc)at line 204 mutates account state throughRefCell<AccountSharedData>references obtained from the cloned context. IfTransactionContext::clone()performs a deep copy, these mutations won't propagate back to the original context ininvoke_context.Please verify that:
RefCell<AccountSharedData>references from the cloned context share the same underlying data as the original (shallow clone of theRefCellpointers), OR- The clone is intentional and mutations are expected to persist through the shared
RefCellreferences#!/bin/bash # Check TransactionContext Clone implementation echo "=== Searching for TransactionContext Clone implementation ===" rg -n "impl.*Clone.*for.*TransactionContext" --type rust -A 20 echo -e "\n=== Check how account references are stored in TransactionContext ===" ast-grep --pattern 'struct TransactionContext { $$$ }'magicblock-committor-service/src/tasks/mod.rs (1)
123-129: Consider addingDebugderive for consistency withCommitDiffTask.
CommitDiffTask(line 115) derivesDebug, which aids in logging and troubleshooting.CommitFinalizeTaskonly derivesClone.♻️ Suggested change
-#[derive(Clone)] +#[derive(Clone, Debug)] pub struct CommitFinalizeTask {magicblock-committor-service/src/tasks/buffer_task.rs (1)
119-129: Extend test/dev conversion to handleCommitFinalize.The
From<ArgsTaskType> for BufferTaskTypeconversion under#[cfg(any(test, feature = "dev-context-only-utils"))]still panics onCommitFinalize, which is now a buffer-capable task. This can break tests that rely on this conversion.🛠️ Proposed fix
impl From<ArgsTaskType> for BufferTaskType { fn from(value: ArgsTaskType) -> Self { match value { ArgsTaskType::Commit(task) => BufferTaskType::Commit(task), ArgsTaskType::CommitDiff(task) => BufferTaskType::CommitDiff(task), + ArgsTaskType::CommitFinalize(task) => { + BufferTaskType::CommitFinalize(task) + } _ => unimplemented!( - "Only commit task can be BufferTask currently. Fix your tests" + "Only commit-related tasks can be BufferTask currently. Fix your tests" ), } } }test-integration/run-test-validator-for-schedulecommit.sh (2)
1-4: Enable strict error handling.
Addingset -eprevents silent failures during test setup.♻️ Proposed change
#!/bin/bash +set -e rm -rf test-ledger
11-60: Replace absolute paths with repo-relative or env-configurable paths.
Hard-coded/Users/...paths won’t work outside the author’s machine.🐛 Proposed fix
#!/bin/bash +SCRIPT_DIR="$(cd -- "$(dirname "$0")" && pwd)" +ROOT_DIR="$(cd -- "$SCRIPT_DIR/.." && pwd)" +DELEGATION_PROGRAM_DIR="${DELEGATION_PROGRAM_DIR:-"$ROOT_DIR/../delegation-program"}" -rm -rf test-ledger +rm -rf "$SCRIPT_DIR/test-ledger" solana-test-validator \ @@ --upgradeable-program \ DELeGGvXpWV2fqJUhqcF5ZSYMS4JTLjteaAMARRSaeSh \ - /Users/snawaz/projects/mb/delegation-program/target/deploy/dlp.so \ + "$DELEGATION_PROGRAM_DIR/target/deploy/dlp.so" \ none \ --upgradeable-program \ DmnRGfyyftzacFb1XadYhWF6vWqXwtQk5tbr6XgR3BA1 \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/schedulecommit/elfs/mdp.so \ + "$SCRIPT_DIR/schedulecommit/elfs/mdp.so" \ none \ --upgradeable-program \ ComtrB2KEaWgXsW1dhr1xYL4Ht4Bjj3gXnnL6KMdABq \ - /Users/snawaz/projects/mb/magicblock-validator/target/deploy/magicblock_committor_program.so \ + "$ROOT_DIR/target/deploy/magicblock_committor_program.so" \ none \ --upgradeable-program \ 9hgprgZiRWmy8KkfvUuaVkDGrqo9GzeXMohwq6BazgUY \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/target/deploy/program_schedulecommit.so \ + "$SCRIPT_DIR/target/deploy/program_schedulecommit.so" \ none \ --upgradeable-program \ f1exzKGtdeVX3d6UXZ89cY7twiNJe9S5uq84RTA4Rq4 \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/target/deploy/program_flexi_counter.so \ + "$SCRIPT_DIR/target/deploy/program_flexi_counter.so" \ none \ --upgradeable-program \ 4RaQH3CUBMSMQsSHPVaww2ifeNEEuaDZjF9CUdFwr3xr \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/target/deploy/program_schedulecommit_security.so \ + "$SCRIPT_DIR/target/deploy/program_schedulecommit_security.so" \ none \ @@ --account \ tEsT3eV6RFCWs1BZ7AXTzasHqTtMnMLCB2tjQ42TDXD \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/configs/accounts/validator-authority.json \ + "$SCRIPT_DIR/configs/accounts/validator-authority.json" \ --account \ LUzidNSiPNjYNkxZcUm5hYHwnWPwsUfh2US1cpWwaBm \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/configs/accounts/luzid-authority.json \ + "$SCRIPT_DIR/configs/accounts/luzid-authority.json" \ --account \ DUH8h7rYjdTPYyBUEGAUwZv9ffz5wiM45GdYWYzogXjp \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/configs/accounts/validator-fees-vault.json \ + "$SCRIPT_DIR/configs/accounts/validator-fees-vault.json" \ --account \ 7JrkjmZPprHwtuvtuGTXp9hwfGYFAQLnLeFM52kqAgXg \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/configs/accounts/protocol-fees-vault.json \ + "$SCRIPT_DIR/configs/accounts/protocol-fees-vault.json" \ --account \ 9yXjZTevvMp1XgZSZEaziPRgFiXtAQChpnP2oX9eCpvt \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/configs/accounts/non-delegated-cloneable-account1.json \ + "$SCRIPT_DIR/configs/accounts/non-delegated-cloneable-account1.json" \ --account \ BHBuATGifAD4JbRpM5nVdyhKzPgv3p2CxLEHAqwBzAj5 \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/configs/accounts/non-delegated-cloneable-account2.json \ + "$SCRIPT_DIR/configs/accounts/non-delegated-cloneable-account2.json" \ --account \ 2o48ieM95rmHqMWC5B3tTX4DL7cLm4m1Kuwjay3keQSv \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/configs/accounts/non-delegated-cloneable-account3.json \ + "$SCRIPT_DIR/configs/accounts/non-delegated-cloneable-account3.json" \ --account \ 2EmfL3MqL3YHABudGNmajjCpR13NNEn9Y4LWxbDm6SwR \ - /Users/snawaz/projects/mb/magicblock-validator/test-integration/configs/accounts/non-delegated-cloneable-account4.json + "$SCRIPT_DIR/configs/accounts/non-delegated-cloneable-account4.json"test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
223-311: DRY: extract a helper for ScheduleCommitType → MagicBaseIntent mapping.
The same match appears in both commit_single_account and commit_book_order_account.Also applies to: 344-365
test-integration/programs/schedulecommit/src/lib.rs (1)
160-162: Update the ScheduleCommitForOrderBook doc comment.
It still referencesScheduleCommitDiffCpi, which no longer matches this variant.✏️ Proposed doc update
- /// ScheduleCommitDiffCpi + /// ScheduleCommitForOrderBooktest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
395-406: Consider combining match arms for readability.Both
CommitandCommitFinalizearms call the same assertion. Combining them would reduce repetition.♻️ Suggested simplification
match commit_type { - ScheduleCommitType::Commit => { - assert_one_committee_account_was_not_undelegated_on_chain(&ctx); - } - ScheduleCommitType::CommitFinalize => { + ScheduleCommitType::Commit | ScheduleCommitType::CommitFinalize => { assert_one_committee_account_was_not_undelegated_on_chain(&ctx); } ScheduleCommitType::CommitAndUndelegate | ScheduleCommitType::CommitFinalizeAndUndelegate => { assert_one_committee_account_was_undelegated_on_chain(&ctx); } }
| let context_acc = get_instruction_account_with_idx( | ||
| transaction_context, | ||
| MAGIC_CONTEXT_IDX, | ||
| )?; | ||
| let context_data = &mut context_acc.borrow_mut(); | ||
| let mut context = | ||
| MagicContext::deserialize(context_data).map_err(|err| { | ||
| ic_msg!( | ||
| invoke_context, | ||
| "Failed to deserialize MagicContext: {}", | ||
| err | ||
| ); | ||
| InstructionError::GenericError | ||
| })?; |
There was a problem hiding this comment.
Consider edge case: deserialization failure after account mutations.
The comment on lines 213-215 acknowledges that if the instruction fails after accounts are mutated (e.g., marked as undelegating at line 204), the commit stays scheduled. However, if MagicContext::deserialize fails at line 222, accounts may already be marked as undelegating without the scheduled commit being recorded.
This could leave accounts in an inconsistent state where they're marked as undelegating but no commit is scheduled.
🔧 Suggested mitigation
Consider reordering operations to validate/deserialize the MagicContext before mutating account states:
+ // Deserialize and validate MagicContext first, before mutating accounts
+ let context_acc = get_instruction_account_with_idx(
+ transaction_context,
+ MAGIC_CONTEXT_IDX,
+ )?;
+ let context_data = &mut context_acc.borrow_mut();
+ let mut context =
+ MagicContext::deserialize(context_data).map_err(|err| {
+ ic_msg!(
+ invoke_context,
+ "Failed to deserialize MagicContext: {}",
+ err
+ );
+ InstructionError::GenericError
+ })?;
+
// ... then iterate and validate accounts ...
// ... mark accounts as undelegating only after context is validated ...🤖 Prompt for AI Agents
In
`@programs/magicblock/src/schedule_transactions/process_schedule_commit_finalize.rs`
around lines 216 - 229, The code mutates account state (marks accounts as
undelegating) before attempting MagicContext::deserialize, which can leave state
inconsistent if deserialization fails; move the call to
get_instruction_account_with_idx(..., MAGIC_CONTEXT_IDX) and the
MagicContext::deserialize(...) to occur and succeed before performing any
account mutations, so that you only alter account state after successful
deserialization; ensure the error path from MagicContext::deserialize returns
early without applying mutations and keep the existing ic_msg! logging and
InstructionError mapping.
| --bpf-program \ | ||
| ComtrB2KEaWgXsW1dhr1xYL4Ht4Bjj3gXnnL6KMdABq \ | ||
| ../target/deploy/magicblock_committor_program.so \ | ||
| --bpf-program \ | ||
| f1exzKGtdeVX3d6UXZ89cY7twiNJe9S5uq84RTA4Rq4 \ | ||
| ./target/deploy/program_flexi_counter.so \ | ||
| --bpf-program \ | ||
| DELeGGvXpWV2fqJUhqcF5ZSYMS4JTLjteaAMARRSaeSh \ | ||
| ./schedulecommit/elfs/dlp.so \ | ||
| --bpf-program \ | ||
| 9hgprgZiRWmy8KkfvUuaVkDGrqo9GzeXMohwq6BazgUY \ | ||
| ./target/deploy/program_schedulecommit.so \ | ||
| --bpf-program \ | ||
| 4RaQH3CUBMSMQsSHPVaww2ifeNEEuaDZjF9CUdFwr3xr \ | ||
| ./target/deploy/program_schedulecommit_security.so |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the directory structure to understand the path inconsistency
echo "=== Checking if there are multiple target directories ==="
fd -t d "^target$" --max-depth 3
echo ""
echo "=== Checking for .so files in target directories ==="
find . -name "*.so" -type f 2>/dev/null | grep -E "(target|schedulecommit)"
echo ""
echo "=== Current directory structure ==="
ls -la | head -20
echo ""
echo "=== Checking test-integration directory ==="
ls -la test-integration/ 2>/dev/null || echo "test-integration directory not found or cannot be listed"
echo ""
echo "=== Checking if schedulecommit/elfs exists ==="
ls -la schedulecommit/elfs/ 2>/dev/null || echo "schedulecommit/elfs not found"
echo ""
echo "=== Checking git root and relative paths ==="
git rev-parse --show-toplevel 2>/dev/null || echo "Not a git repo"
pwd
echo ""
echo "=== Content of run-validator.sh around lines 17-31 ==="
cat -n test-integration/run-validator.sh 2>/dev/null | sed -n '15,35p'Repository: magicblock-labs/magicblock-validator
Length of output: 4281
Inconsistent program paths prevent correct file resolution.
The BPF program paths use conflicting directory conventions that cannot both be correct:
- Line 19:
../target/deploy/magicblock_committor_program.so(resolves to repo root) - Lines 22, 28, 31:
./target/deploy/...(resolves to test-integration/)
If this script runs from test-integration/, these point to different locations. Standardize all paths to use a single convention—either all relative to repo root (../target/deploy/...) or all relative to test-integration/ (./target/deploy/...). Only the dlp.so at line 25 has a consistent, verifiable path.
🤖 Prompt for AI Agents
In `@test-integration/run-validator.sh` around lines 17 - 31, The BPF program path
entries are inconsistent (e.g., ../target/deploy/magicblock_committor_program.so
vs ./target/deploy/program_flexi_counter.so and
./target/deploy/program_schedulecommit.so) which breaks resolution depending on
the working directory; pick a single convention and update all --bpf-program
arguments to match it (either change the ./target/deploy/* entries to
../target/deploy/* to be repo-root relative, or change
../target/deploy/magicblock_committor_program.so to
./target/deploy/magicblock_committor_program.so to be test-integration
relative), leaving the already-correct ./schedulecommit/elfs/dlp.so path intact.
Ensure every occurrence of ./target/deploy/* or ../target/deploy/* in the script
is updated consistently so all referenced .so files resolve from the same
working directory.
6f76f6a to
3895680
Compare
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/tasks/task_builder.rs (1)
199-199:⚠️ Potential issue | 🟠 MajorReplace
.expect()with proper error handling.Per coding guidelines,
.expect()in production code undermagicblock-*paths should be treated as a major issue. While the invariant comment is helpful, this should use proper error handling.🛡️ Proposed fix
let tasks = accounts .iter() .map(|account| { - let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!"); + let commit_id = *commit_ids.get(&account.pubkey).ok_or_else(|| { + TaskBuilderError::CommitTasksBuildError( + TaskInfoFetcherError::MissingCommitId(account.pubkey) + ) + })?;This would require changing the closure to return
Resultand collecting withtry_foldor similar, or handling the error case appropriately.As per coding guidelines: "Treat any usage of
.unwrap()or.expect()in production Rust code as a MAJOR issue."
🤖 Fix all issues with AI agents
In `@magicblock-committor-service/src/intent_executor/mod.rs`:
- Around line 163-164: Fix the typo in the TODO comment referencing the intent
variant: change "BaseActionse" to "BaseActions" in the comment that mentions
MagicBaseIntent::BaseActionse inside mod.rs (near the TODO about it being the
BaseActions scenario rather than Commit); just update the comment text to read
MagicBaseIntent::BaseActions.
In `@test-integration/Cargo.toml`:
- Around line 39-41: The dependency entry for ephemeral-rollups-sdk currently
pins to a branch ("snawaz/create-commit-finalize") which can float; if the
intent is stable builds, replace the branch with a specific git rev or tag (or
add a rev = "<sha>" field) for ephemeral-rollups-sdk (and the other similar
entries around the same block) and ensure Cargo.lock is committed/updated in the
repository so the lockfile is used to prevent unintended drift; if floating
branches are intentional, add a comment explaining that and keep Cargo.lock
policy documented.
In `@test-integration/schedulecommit/test-scenarios/tests/01_commits.rs`:
- Line 72: Replace uses of the fully-qualified enum path
program_schedulecommit::ScheduleCommitType::Commit with the already-imported
ScheduleCommitType::Commit (and similarly for other occurrences using
program_schedulecommit::ScheduleCommitType) so the code uses the shorter,
imported name; update both call sites where
program_schedulecommit::ScheduleCommitType is used to reference
ScheduleCommitType directly.
| // TODO (snawaz): it's actually MagicBaseIntent::BaseActionse scenario, not Commit | ||
| // scenario. The related code needs little bit of refactoring. |
There was a problem hiding this comment.
Fix typo in TODO comment.
"BaseActionse" should be "BaseActions".
📝 Proposed fix
- // TODO (snawaz): it's actually MagicBaseIntent::BaseActionse scenario, not Commit
+ // TODO (snawaz): it's actually MagicBaseIntent::BaseActions scenario, not Commit📝 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.
| // TODO (snawaz): it's actually MagicBaseIntent::BaseActionse scenario, not Commit | |
| // scenario. The related code needs little bit of refactoring. | |
| // TODO (snawaz): it's actually MagicBaseIntent::BaseActions scenario, not Commit | |
| // scenario. The related code needs little bit of refactoring. |
🤖 Prompt for AI Agents
In `@magicblock-committor-service/src/intent_executor/mod.rs` around lines 163 -
164, Fix the typo in the TODO comment referencing the intent variant: change
"BaseActionse" to "BaseActions" in the comment that mentions
MagicBaseIntent::BaseActionse inside mod.rs (near the TODO about it being the
BaseActions scenario rather than Commit); just update the comment text to read
MagicBaseIntent::BaseActions.
| ephemeral-rollups-sdk = { git = "https://github.com/magicblock-labs/ephemeral-rollups-sdk.git", branch = "snawaz/create-commit-finalize", default-features = false, features = [ | ||
| "modular-sdk", | ||
| ] } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Branch-based git deps: confirm intent and lockfile stability.
If these are not meant to float, consider pinning to a rev/tag and ensure Cargo.lock is committed/used to avoid unintended drift.
Also applies to: 63-65
🤖 Prompt for AI Agents
In `@test-integration/Cargo.toml` around lines 39 - 41, The dependency entry for
ephemeral-rollups-sdk currently pins to a branch
("snawaz/create-commit-finalize") which can float; if the intent is stable
builds, replace the branch with a specific git rev or tag (or add a rev =
"<sha>" field) for ephemeral-rollups-sdk (and the other similar entries around
the same block) and ensure Cargo.lock is committed/updated in the repository so
the lockfile is used to prevent unintended drift; if floating branches are
intentional, add a comment explaining that and keep Cargo.lock policy
documented.
| .map(|(player, _)| player.pubkey()) | ||
| .collect::<Vec<_>>(), | ||
| &committees.iter().map(|(_, pda)| *pda).collect::<Vec<_>>(), | ||
| program_schedulecommit::ScheduleCommitType::Commit, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using the imported ScheduleCommitType directly.
ScheduleCommitType is already imported on line 9, but these call sites use the fully-qualified path program_schedulecommit::ScheduleCommitType::Commit. Using the shorter ScheduleCommitType::Commit would be more concise and consistent.
♻️ Suggested simplification
- program_schedulecommit::ScheduleCommitType::Commit,
+ ScheduleCommitType::Commit,Also applies to: 127-127
🤖 Prompt for AI Agents
In `@test-integration/schedulecommit/test-scenarios/tests/01_commits.rs` at line
72, Replace uses of the fully-qualified enum path
program_schedulecommit::ScheduleCommitType::Commit with the already-imported
ScheduleCommitType::Commit (and similarly for other occurrences using
program_schedulecommit::ScheduleCommitType) so the code uses the shorter,
imported name; update both call sites where
program_schedulecommit::ScheduleCommitType is used to reference
ScheduleCommitType directly.
1 participant
Coming soon.
Summary
Compatibility
Testing
Checklist
Summary by CodeRabbit
New Features
Public API
Tests
Chores