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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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); } }
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
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.
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.
3895680 to
fac3a53
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 (6)
magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (1)
39-95: 🧹 Nitpick | 🔵 TrivialConsider extracting the repeated
CommitMetaconstruction into a helper.All three commit variants (
Commit,CommitDiff,CommitFinalize) in bothvisit_args_taskandvisit_buffer_taskconstructCommitMetaidentically fromcommitted_accountandcommit_id. A shared trait method or helper on the task types could reduce this six-fold duplication.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs` around lines 39 - 95, The three identical CommitMeta constructions in TaskVisitorUtils::visit_args_task and ::visit_buffer_task should be extracted to a single helper: add a method (e.g., to_commit_meta()) on the task enum variants or implement a small trait (e.g., CommitMetaConvertible) for ArgsTaskType and BufferTaskType that returns CommitMeta from a &self (pulling committed_account.pubkey, commit_id, committed_account.remote_slot), then replace each ArgsTaskType::Commit/CommitDiff/CommitFinalize and BufferTaskType::Commit/CommitDiff/CommitFinalize arm to call that helper and set *commit_meta = Some(self_helper), keeping the existing fallback (_ => None) behavior in visit_args_task; this removes the duplicated construction while preserving semantics in visit_args_task and visit_buffer_task.magicblock-committor-service/src/persist/commit_persister.rs (1)
136-150:⚠️ Potential issue | 🟠 Major
create_commit_rowsdoes not persistcommit_finalize/commit_finalize_and_undelegateaccounts.The method only covers
get_commit_intent_accounts()andget_undelegate_intent_accounts(). If aScheduledIntentBundlecarries accounts under the newcommit_finalizeorcommit_finalize_and_undelegatefields (which have corresponding getter methods), those accounts will not be persisted. By contrast,task_builder.rscorrectly handles all four account variants when constructing execution tasks.Add the two missing getter calls to ensure complete persistence coverage:
.get_commit_finalize_intent_accounts() .get_commit_finalize_and_undelegate_intent_accounts()🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@magicblock-committor-service/src/persist/commit_persister.rs` around lines 136 - 150, The create_commit_rows function currently only iterates intent_bundle.get_commit_intent_accounts() and intent_bundle.get_undelegate_intent_accounts(), so accounts from commit_finalize and commit_finalize_and_undelegate are omitted; update the iterator to include intent_bundle.get_commit_finalize_intent_accounts() and intent_bundle.get_commit_finalize_and_undelegate_intent_accounts() alongside the existing two so the filter_map/flat_map stage will persist those rows as well, keeping the same mapping to (undelegate, accounts) and calling create_row for each account.test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
786-786:⚠️ Potential issue | 🔴 CriticalMissing
program_idargument — will not compile.
ix_commit_localnow requires 4 arguments (Line 820 addsprogram_id: Pubkey), but the call inexecute_intent_bundlestill passes only 3.🐛 Proposed fix
- ix_commit_local(service, vec![intent_bundle], expected_strategies).await; + ix_commit_local(service, vec![intent_bundle], expected_strategies, program_flexi_counter::ID).await;🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs` at line 786, The call to ix_commit_local in execute_intent_bundle is missing the new program_id: Pubkey fourth argument; update the call to pass the correct Pubkey (e.g., the test/service PROGRAM_ID or service.program_id) as the fourth parameter to ix_commit_local, and if execute_intent_bundle does not currently have access to that Pubkey, add a program_id: Pubkey parameter to execute_intent_bundle and thread it through to the ix_commit_local invocation so the signatures match.magicblock-committor-service/src/tasks/buffer_task.rs (1)
119-129:⚠️ Potential issue | 🟠 Major
From<ArgsTaskType>still does not handleCommitFinalize, will panic in tests/dev.
CommitFinalizeis now a fully supported buffer-capable task variant (preparation, instruction, budgeting all implemented), but thecfg(test/dev)conversion fromArgsTaskTypetoBufferTaskTypestill uses a catch-all_ => unimplemented!(...)that will panic if any test attempts 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" ), } } }As per coding guidelines,
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue — whileunimplemented!()is strictly acfg(test/dev)panic, the missing conversion will break tests exercising the new CommitFinalize buffer path.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@magicblock-committor-service/src/tasks/buffer_task.rs` around lines 119 - 129, The From<ArgsTaskType> for BufferTaskType impl currently panics for unsupported variants and does not handle the new CommitFinalize variant; update the match in impl From<ArgsTaskType> for BufferTaskType to include ArgsTaskType::CommitFinalize(task) => BufferTaskType::CommitFinalize(task) (and remove or narrow the catch-all unimplemented branch so tests/dev conversions no longer panic), referencing the impl and the ArgsTaskType/BufferTaskType enum variants to locate the change.test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
168-170: 🧹 Nitpick | 🔵 TrivialMisleading debug message for non-undelegate commit types.
Line 170 always logs
"Commit and Undelegate Transaction result"regardless of thecommit_type. Consider includingcommit_typein the message (e.g.,"commit_order_book_account ({commit_type:?}) result: ...") to avoid confusion when debuggingCommitorCommitFinalizescenarios.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs` around lines 168 - 170, The debug message always prints "Commit and Undelegate Transaction result" which is misleading for non-undelegate commits; update the log to include the commit_type variable and tx_res so the message reflects the actual commit variant (for example replace the fixed string with something like: debug!("commit_order_book_account ({commit_type:?}) result: '{:?}'", commit_type, tx_res)), ensuring you reference commit_type and tx_res in the debug macro where the two debug! calls currently are.programs/magicblock/src/magic_scheduled_base_intent.rs (1)
424-436:⚠️ Potential issue | 🔴 CriticalUpdate six methods in
MagicIntentBundleto includecommit_finalizeandcommit_finalize_and_undelegatefields.The new finalize fields (lines 186–187) are missing from the following methods, causing incorrect behavior:
is_empty()(line 424): Returnstruefor finalize-only bundles, causingpost_validationto reject themhas_undelegate_intent()(line 330): Missescommit_finalize_and_undelegatehas_committed_accounts()(line 334): Returnsfalsewhen only finalize accounts existget_all_committed_accounts()(line 390): Omits finalize accountsget_all_committed_pubkeys()(line 401): Omits finalize pubkeysvalidate()(line 257): Skips index overlap checks involving finalize variantspost_validation()(line 292): Omits finalize accounts from duplicate pubkey validationEach method must check both the non-finalize and finalize variants to correctly handle all intent types. Apply the pattern shown in the proposed fix consistently across all seven methods.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@programs/magicblock/src/magic_scheduled_base_intent.rs` around lines 424 - 436, Update the six methods in MagicIntentBundle to consider the new commit_finalize and commit_finalize_and_undelegate fields: in is_empty(), include checks for commit_finalize and commit_finalize_and_undelegate alongside commit and commit_and_undelegate; in has_undelegate_intent() and has_committed_accounts(), treat commit_finalize_and_undelegate and commit_finalize the same as their non-finalize counterparts; in get_all_committed_accounts() and get_all_committed_pubkeys(), append accounts/pubkeys from commit_finalize and commit_finalize_and_undelegate just like commit and commit_and_undelegate; in validate() and post_validation(), include index overlap and duplicate pubkey checks for the finalize variants in the same pattern used for commit and commit_and_undelegate so finalize-only bundles are correctly validated.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@Cargo.toml`:
- Around line 104-106: The dependency entry for magicblock-delegation-program in
Cargo.toml points to the personal feature branch
"snawaz/commit-finalize-from-buffer"; change this to either reference the
canonical branch (e.g., "main" or "master") or pin to an exact commit by
replacing branch = "snawaz/commit-finalize-from-buffer" with rev =
"<commit-sha>" so workspace builds are stable and reproducible; update the
dependency line for magicblock-delegation-program accordingly.
In `@magicblock-committor-service/src/persist/commit_persister.rs`:
- Around line 136-150: The create_commit_rows function currently only iterates
intent_bundle.get_commit_intent_accounts() and
intent_bundle.get_undelegate_intent_accounts(), so accounts from commit_finalize
and commit_finalize_and_undelegate are omitted; update the iterator to include
intent_bundle.get_commit_finalize_intent_accounts() and
intent_bundle.get_commit_finalize_and_undelegate_intent_accounts() alongside the
existing two so the filter_map/flat_map stage will persist those rows as well,
keeping the same mapping to (undelegate, accounts) and calling create_row for
each account.
In `@magicblock-committor-service/src/tasks/buffer_task.rs`:
- Around line 119-129: The From<ArgsTaskType> for BufferTaskType impl currently
panics for unsupported variants and does not handle the new CommitFinalize
variant; update the match in impl From<ArgsTaskType> for BufferTaskType to
include ArgsTaskType::CommitFinalize(task) =>
BufferTaskType::CommitFinalize(task) (and remove or narrow the catch-all
unimplemented branch so tests/dev conversions no longer panic), referencing the
impl and the ArgsTaskType/BufferTaskType enum variants to locate the change.
In `@magicblock-committor-service/src/tasks/task_builder.rs`:
- Around line 175-188: Replace the opaque tuple triple used to build
flagged_accounts with a small named struct to improve readability: define a
struct FlaggedAccount { allow_undelegation: bool, finalize: bool, account:
CommittedAccount } and construct Vec<FlaggedAccount> instead of
Vec<(bool,bool,CommittedAccount)> in task_builder.rs (the current
flagged_accounts creation). Update all call sites and helper signatures that
expect the tuple—e.g., fetch_commit_nonces and fetch_diffable_accounts—to accept
&FlaggedAccount (or FlaggedAccount) and unpack fields by name, and change any
pattern matches that used (allow_undelegation, finalize, account) to reference
the struct fields for clarity.
In `@magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs`:
- Around line 39-95: The three identical CommitMeta constructions in
TaskVisitorUtils::visit_args_task and ::visit_buffer_task should be extracted to
a single helper: add a method (e.g., to_commit_meta()) on the task enum variants
or implement a small trait (e.g., CommitMetaConvertible) for ArgsTaskType and
BufferTaskType that returns CommitMeta from a &self (pulling
committed_account.pubkey, commit_id, committed_account.remote_slot), then
replace each ArgsTaskType::Commit/CommitDiff/CommitFinalize and
BufferTaskType::Commit/CommitDiff/CommitFinalize arm to call that helper and set
*commit_meta = Some(self_helper), keeping the existing fallback (_ => None)
behavior in visit_args_task; this removes the duplicated construction while
preserving semantics in visit_args_task and visit_buffer_task.
In `@programs/magicblock/src/magic_scheduled_base_intent.rs`:
- Around line 156-159: Remove the dead commented-out is_commit_finalize method
block; either delete the three commented lines entirely or replace them with a
real implementation such as pub fn is_commit_finalize(&self) -> bool {
self.intent_bundle.is_commit_finalize() } so the API is either gone or correctly
delegates to intent_bundle instead of leaving a todo in comments.
- Around line 363-379: Update the incorrect doc comments for the
MagicIntentBundle accessor methods: change the doc for
get_commit_finalize_and_undelegate_intent_accounts to read "Returns
`CommitFinalizeAndUndelegate` intent's accounts" (or similar) and change the doc
for get_commit_finalize_intent_accounts to read "Returns `CommitFinalize`
intent's accounts" so the comment text matches the actual intent names returned
by those functions.
- Around line 122-135: The doc comments are incorrect copies; update the
comments for the two methods so they describe the actual intents they return:
change the comment for get_commit_finalize_and_undelegate_intent_accounts to
indicate it returns CommitFinalizeAndUndelegate (or
commit-finalize-and-undelegate) intent accounts, and change the comment for
get_commit_finalize_intent_accounts to indicate it returns CommitFinalize (or
commit-finalize) intent accounts; ensure the wording matches the method names
get_commit_finalize_and_undelegate_intent_accounts and
get_commit_finalize_intent_accounts exactly.
- Around line 424-436: Update the six methods in MagicIntentBundle to consider
the new commit_finalize and commit_finalize_and_undelegate fields: in
is_empty(), include checks for commit_finalize and
commit_finalize_and_undelegate alongside commit and commit_and_undelegate; in
has_undelegate_intent() and has_committed_accounts(), treat
commit_finalize_and_undelegate and commit_finalize the same as their
non-finalize counterparts; in get_all_committed_accounts() and
get_all_committed_pubkeys(), append accounts/pubkeys from commit_finalize and
commit_finalize_and_undelegate just like commit and commit_and_undelegate; in
validate() and post_validation(), include index overlap and duplicate pubkey
checks for the finalize variants in the same pattern used for commit and
commit_and_undelegate so finalize-only bundles are correctly validated.
In `@test-integration/Cargo.toml`:
- Around line 39-41: The Cargo dependency entry for ephemeral-rollups-sdk
references a personal feature branch ("snawaz/create-commit-finalize") instead
of a stable upstream release; update the dependency in
test-integration/Cargo.toml (the ephemeral-rollups-sdk git/branch declaration)
to point to a merged upstream tag or published crate version, or replace the git
branch with the canonical crate version (or the repository's main branch) once
the feature branch is merged upstream so CI and other contributors do not rely
on a personal branch.
In
`@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs`:
- Around line 395-406: The match on commit_type has identical bodies for
ScheduleCommitType::Commit and ScheduleCommitType::CommitFinalize; collapse them
into a single arm using a multi-pattern (ScheduleCommitType::Commit |
ScheduleCommitType::CommitFinalize) that calls
assert_one_committee_account_was_not_undelegated_on_chain(&ctx), leaving the
other arm grouping (CommitAndUndelegate | CommitFinalizeAndUndelegate) unchanged
which calls assert_one_committee_account_was_undelegated_on_chain(&ctx).
- Around line 168-170: The debug message always prints "Commit and Undelegate
Transaction result" which is misleading for non-undelegate commits; update the
log to include the commit_type variable and tx_res so the message reflects the
actual commit variant (for example replace the fixed string with something like:
debug!("commit_order_book_account ({commit_type:?}) result: '{:?}'",
commit_type, tx_res)), ensuring you reference commit_type and tx_res in the
debug macro where the two debug! calls currently are.
In `@test-integration/test-committor-service/tests/test_ix_commit_local.rs`:
- Around line 896-898: The code references is_undelegate out of scope when
computing expected_owner (using program_id and dlp::id()) — remove the
dead/out-of-scope block that sets expected_owner using is_undelegate and instead
rely on the inner-loop logic where is_undelegate is defined (and the later
correct expected_owner computation around Lines 906–910); specifically delete or
revert the outer expected_owner declaration that uses is_undelegate so the
inner-loop expected_owner (and program_id/dlp::id() usage) remains the single
source of truth.
- Line 786: The call to ix_commit_local in execute_intent_bundle is missing the
new program_id: Pubkey fourth argument; update the call to pass the correct
Pubkey (e.g., the test/service PROGRAM_ID or service.program_id) as the fourth
parameter to ix_commit_local, and if execute_intent_bundle does not currently
have access to that Pubkey, add a program_id: Pubkey parameter to
execute_intent_bundle and thread it through to the ix_commit_local invocation so
the signatures match.
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Show resolved
Hide resolved
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
6 times, most recently
from
060f282 to
176da42
Compare
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
176da42 to
422570f
Compare
snawaz
force-pushed
the
graphite-base/847
branch
from
258e6ee to
3c10759
Compare
snawaz
changed the base branch from
graphite-base/847
to
snawaz/commit-finalize-api
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
422570f to
bfc6422
Compare
snawaz
force-pushed
the
snawaz/commit-finalize-api
branch
from
3c10759 to
da13a07
Compare
snawaz
changed the base branch from
snawaz/commit-finalize-api
to
graphite-base/847
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
bfc6422 to
33f3097
Compare
snawaz
force-pushed
the
graphite-base/847
branch
from
da13a07 to
f5a1892
Compare
snawaz
changed the base branch from
graphite-base/847
to
snawaz/commit-finalize-api
snawaz
force-pushed
the
snawaz/commit-finalize-api
branch
from
f5a1892 to
c4ec16d
Compare
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
33f3097 to
6478fcd
Compare
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
6478fcd to
2a10bab
Compare
snawaz
force-pushed
the
snawaz/schedule-commit-finalize
branch
from
c2d437f to
a281d23
Compare
1 participant
Add
ScheduleCommitFinalizethat eventually invokes:CommitFinalizeandCommitFinalizeFromBuffer. Unlike older commit instructions where we had separate instruction of full-state-states and diff-bytes,CommitFinalizehandles both, and so doesCommitFinalizeFromBuffer.