feat: Execute CommitDiff as BufferTask #616
Conversation
📝 WalkthroughWalkthroughAdds a CommitDiff task path (new 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. |
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
4 times, most recently
from
dd95c9a to
4a09180
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
4ea5450 to
41493c5
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
17f396e to
3259f79
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
385e7a3 to
3999503
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
23a797e to
64d79cc
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
3999503 to
256ab3f
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
5838b75 to
e182eb1
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
59b7935 to
c07a177
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
18adf1e to
5b95cad
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
9da0edd to
cf6229d
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
2aada37 to
2eb5776
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
9be6a8a to
b94a549
Compare
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
Looks great! Would only ask to add some test that testifies to BufferDiff usage
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
e0844fd to
5652b48
Compare
snawaz
force-pushed
the
graphite-base/616
branch
from
8d88564 to
eb17e8a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
87-98: Update the error message to reflect CommitDiff support.The
unimplemented!()message states "Only commit task can be BufferTask currently", butCommitDiffis now also supported.🔎 Suggested fix
- _ => unimplemented!( - "Only commit task can be BufferTask currently. Fix your tests" - ), + _ => unimplemented!( + "Only Commit and CommitDiff tasks can be BufferTask currently. Fix your tests" + ),
🧹 Nitpick comments (4)
magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (2)
19-37: LGTM! Correct handling of CommitDiff for metadata extraction.The logic correctly extracts
CommitMetafrom bothCommitandCommitDiffvariants. The identical handling is appropriate since both represent commit operations with the same metadata requirements.💡 Optional: Reduce duplication using combined match patterns
fn visit_args_task(&mut self, task: &ArgsTask) { let Self::GetCommitMeta(commit_meta) = self; - match &task.task_type { - ArgsTaskType::Commit(task) => { - *commit_meta = Some(CommitMeta { - committed_pubkey: task.committed_account.pubkey, - commit_id: task.commit_id, - }) - } - ArgsTaskType::CommitDiff(task) => { - *commit_meta = Some(CommitMeta { - committed_pubkey: task.committed_account.pubkey, - commit_id: task.commit_id, - }) - } + *commit_meta = match &task.task_type { + ArgsTaskType::Commit(task) | ArgsTaskType::CommitDiff(task) => { + Some(CommitMeta { + committed_pubkey: task.committed_account.pubkey, + commit_id: task.commit_id, + }) + } - _ => *commit_meta = None, + _ => None, - } + }; }
39-56: LGTM! Consistent handling across BufferTask variants.The metadata extraction logic for
BufferTaskcorrectly mirrors theArgsTaskhandling, treating bothCommitandCommitDiffidentically.💡 Optional: Reduce duplication using combined match patterns
fn visit_buffer_task(&mut self, task: &BufferTask) { let Self::GetCommitMeta(commit_meta) = self; - match &task.task_type { - BufferTaskType::Commit(task) => { - *commit_meta = Some(CommitMeta { - committed_pubkey: task.committed_account.pubkey, - commit_id: task.commit_id, - }) - } - BufferTaskType::CommitDiff(task) => { - *commit_meta = Some(CommitMeta { - committed_pubkey: task.committed_account.pubkey, - commit_id: task.commit_id, - }) - } - } + *commit_meta = match &task.task_type { + BufferTaskType::Commit(task) | BufferTaskType::CommitDiff(task) => { + Some(CommitMeta { + committed_pubkey: task.committed_account.pubkey, + commit_id: task.commit_id, + }) + } + }; }magicblock-committor-service/src/tasks/buffer_task.rs (2)
52-84: LGTM! Preparation logic correctly handles both commit types.The preparation logic appropriately:
- For
Commit: uses full account data- For
CommitDiff: computes diff from base to committed state viacompute_diffBoth derive chunks from the respective data length, ensuring buffer chunking matches the instruction expectations.
The two branches follow a similar pattern (compute data → derive chunks → build PreparationTask). While this duplication is acceptable for clarity, a helper function could reduce repetition if preferred.
100-146: LGTM! Instruction generation correctly differentiates commit types.The instruction generation appropriately:
- For
Commit: callscommit_state_from_bufferwith full state data in buffer- For
CommitDiff: callscommit_diff_from_bufferwith diff data in bufferBoth use
CommitStateFromBufferArgscorrectly since the actual data/diff is already in the buffer; args only contain commit metadata (nonce, lamports, allow_undelegation).The buffer PDA derivation logic is duplicated across both branches. Consider extracting it to reduce repetition:
💡 Optional refactor to reduce duplication
impl BaseTask for BufferTask { fn instruction(&self, validator: &Pubkey) -> Instruction { + let (commit_id, committed_account, allow_undelegation) = match &self.task_type { + BufferTaskType::Commit(task) => ( + task.commit_id, + &task.committed_account, + task.allow_undelegation, + ), + BufferTaskType::CommitDiff(task) => ( + task.commit_id, + &task.committed_account, + task.allow_undelegation, + ), + }; + + let commit_id_slice = commit_id.to_le_bytes(); + let (commit_buffer_pubkey, _) = + magicblock_committor_program::pdas::buffer_pda( + validator, + &committed_account.pubkey, + &commit_id_slice, + ); + + let args = CommitStateFromBufferArgs { + nonce: commit_id, + lamports: committed_account.account.lamports, + allow_undelegation, + }; + match &self.task_type { - BufferTaskType::Commit(task) => { - let commit_id_slice = task.commit_id.to_le_bytes(); - let (commit_buffer_pubkey, _) = - magicblock_committor_program::pdas::buffer_pda( - validator, - &task.committed_account.pubkey, - &commit_id_slice, - ); - + BufferTaskType::Commit(_) => { dlp::instruction_builder::commit_state_from_buffer( *validator, - task.committed_account.pubkey, - task.committed_account.account.owner, + committed_account.pubkey, + committed_account.account.owner, commit_buffer_pubkey, - CommitStateFromBufferArgs { - nonce: task.commit_id, - lamports: task.committed_account.account.lamports, - allow_undelegation: task.allow_undelegation, - }, + args, ) } - BufferTaskType::CommitDiff(task) => { - let commit_id_slice = task.commit_id.to_le_bytes(); - let (commit_buffer_pubkey, _) = - magicblock_committor_program::pdas::buffer_pda( - validator, - &task.committed_account.pubkey, - &commit_id_slice, - ); - + BufferTaskType::CommitDiff(_) => { dlp::instruction_builder::commit_diff_from_buffer( *validator, - task.committed_account.pubkey, - task.committed_account.account.owner, + committed_account.pubkey, + committed_account.account.owner, commit_buffer_pubkey, - CommitStateFromBufferArgs { - nonce: task.commit_id, - lamports: task.committed_account.account.lamports, - allow_undelegation: task.allow_undelegation, - }, + args, ) } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
magicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rsmagicblock-committor-service/src/tasks/task_visitors/utility_visitor.rstest-integration/Cargo.tomltest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
💤 Files with no reviewable changes (1)
- magicblock-committor-service/src/tasks/mod.rs
✅ Files skipped from review due to trivial changes (1)
- test-integration/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
- magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs
- test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
🧠 Learnings (1)
📚 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/buffer_task.rs
🧬 Code graph analysis (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
value(210-215)
magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs (2)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
visit_buffer_task(68-107)magicblock-committor-service/src/tasks/visitor.rs (1)
visit_buffer_task(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run_make_ci_format
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (6)
magicblock-committor-service/src/tasks/args_task.rs (1)
134-138: LGTM! CommitDiff now optimizes to BufferTask directly.The change correctly maps
ArgsTaskType::CommitDifftoBufferTaskType::CommitDiff, preventing degradation toCommitStateand enabling direct execution as a BufferTask. This aligns with the PR objective.magicblock-committor-service/src/tasks/buffer_task.rs (5)
20-26: LGTM! BufferTaskType now supports CommitDiff.The addition of
CommitDiff(CommitDiffTask)variant enables direct execution of CommitDiff operations as BufferTask, addressing the PR's core objective.
171-176: LGTM! Compute units appropriately set for CommitDiff.Using the same compute unit budget (70,000) for both
CommitandCommitDiffis reasonable and safe. Based on learnings, these units represent the per-task budget and will be summed correctly when multiple tasks are batched in transactions.
183-188: LGTM! Correct task type mapping for CommitDiff.Mapping
CommitDifftoTaskType::Commitis appropriate since both represent commit operations from a task categorization perspective.
195-206: LGTM! Commit ID reset correctly handles both variants.The
reset_commit_idmethod now properly updates the commit ID for bothCommitandCommitDifftasks and re-triggers preparation, ensuring buffer data is recomputed with the new commit ID.
209-216: LGTM! Consistent label for CommitDiff metrics.The label "buffer_commit_diff" follows the established naming pattern and enables proper differentiation in metrics collection.
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
I trust @taco-paco with the detailed review and agree that test should be added.
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 (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
113-121: CommitStrategy::FromBuffer not renamed to StateBuffer.Lines 114 and 119 still use
CommitStrategy::FromBufferwhich should beCommitStrategy::StateBufferbased on the enum variant rename. This will cause compilation errors.🔎 Suggested fix
// We cannot use 680 as changed_len because that both 679 and 680 produce encoded tx // of size 1644 (which is the max limit), but while the size of raw bytes for 679 is within // 1232 limit, the size for 680 execeds by 1 (1233). That is why we used // 681 as changed_len where CommitStrategy goes from Args to FromBuffer. - commit_book_order_account(681, CommitStrategy::FromBuffer, false).await; + commit_book_order_account(681, CommitStrategy::StateBuffer, false).await; } #[tokio::test] async fn test_ix_commit_order_book_change_10k_bytes() { - commit_book_order_account(10 * 1024, CommitStrategy::FromBuffer, false) + commit_book_order_account(10 * 1024, CommitStrategy::StateBuffer, false) .await; }
🧹 Nitpick comments (1)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
92-95: Update panic message to reflect CommitDiff support.The panic message mentions "Only commit task" but should now include CommitDiff since it's a valid BufferTaskType.
🔎 Suggested fix
ArgsTaskType::CommitDiff(task) => BufferTaskType::CommitDiff(task), _ => unimplemented!( - "Only commit task can be BufferTask currently. Fix your tests" + "Only Commit and CommitDiff tasks can be BufferTask currently. Fix your tests" ),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
magicblock-committor-service/src/persist/commit_persister.rsmagicblock-committor-service/src/persist/db.rsmagicblock-committor-service/src/persist/types/commit_strategy.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs
- magicblock-committor-service/src/tasks/args_task.rs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/persist/db.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/persist/commit_persister.rsmagicblock-committor-service/src/persist/types/commit_strategy.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/persist/commit_persister.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/mod.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/buffer_task.rs
🧬 Code graph analysis (3)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
try_optimize_tx_size(125-143)magicblock-committor-service/src/tasks/buffer_task.rs (1)
try_optimize_tx_size(149-153)
magicblock-committor-service/src/tasks/buffer_task.rs (4)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
args(17-23)magicblock-committor-service/src/tasks/args_task.rs (3)
task_type(177-185)instruction(51-123)try_optimize_tx_size(125-143)magicblock-committor-service/src/tasks/mod.rs (6)
task_type(93-93)instruction(67-67)instruction(259-265)buffer_pda(235-242)buffer_pda(286-293)try_optimize_tx_size(71-73)magicblock-committor-program/src/state/chunks.rs (1)
from_data_length(48-51)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
magicblock-committor-service/src/persist/types/commit_status.rs (1)
as_str(125-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
🔇 Additional comments (12)
magicblock-committor-service/src/tasks/mod.rs (2)
49-53: LGTM - TaskStrategy is now public as intended.The enum is correctly made public to support external visibility requirements in task strategy handling.
69-73: LGTM - Method rename improves clarity.The
try_optimize_tx_sizename better communicates the intent compared to the previous genericoptimize, and the documentation accurately describes the return semantics.magicblock-committor-service/src/tasks/buffer_task.rs (3)
67-82: LGTM - CommitDiff preparation correctly computes diff and creates preparation task.The diff computation order
(base_account.data, committed_account.account.data)is consistent with args_task.rs, and the chunking is correctly derived from the diff length. Past review concern about argument order has been addressed.
124-144: LGTM - CommitDiff instruction generation follows the same pattern as Commit.The use of
CommitStateFromBufferArgsforcommit_diff_from_bufferis consistent with the buffer-based approach where the actual diff data is read from the buffer account, and only metadata (nonce, lamports, allow_undelegation) is passed as instruction arguments.
171-176: LGTM - Consistent compute units for CommitDiff.Using the same 70,000 compute units as Commit is reasonable since both operations follow similar execution paths on-chain.
magicblock-committor-service/src/persist/commit_persister.rs (1)
587-596: LGTM - Test correctly updated for renamed CommitStrategy variant.The test now uses
CommitStrategy::StateArgsconsistent with the enum variant rename.magicblock-committor-service/src/persist/db.rs (2)
775-776: LGTM - Test helper updated for renamed variant.
910-911: LGTM - Test updated for renamed variant.magicblock-committor-service/src/tasks/task_strategist.rs (2)
272-278: LGTM - Method rename with accurate documentation.The renamed
try_optimize_tx_size_if_neededclearly communicates intent, and the documentation correctly explains that the returned size may still exceed the limit.
331-331: LGTM - Consistent with BaseTask trait method rename.test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
267-306: LGTM - Multi-account test expectations correctly updated.Tests now use
CommitStrategy::StateArgsconsistent with the enum variant rename.
340-427: LGTM - Bundle test expectations correctly updated to State-prefixed variants.Tests correctly use
StateArgs,StateBuffer, andStateBufferWithLookupTablevariants.
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
2ba11d8 to
7926711
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
25-58: Serialization round-trip is broken due to inconsistent string mappings.There's a critical mismatch between
as_str()output andTryFrom<&str>input:
Variant as_str()returnsTryFromexpectsStateArgsWithLookupTable"StageArgsWithLookupTable""StateArgsWithLookupTable"StateBuffer"StageBuffer""StageBuffer"✓StateBufferWithLookupTable"StageBufferWithLookupTable""StageBufferWithLookupTable"✓The
StateArgsWithLookupTablevariant has a mismatch:as_str()returns"StageArgsWithLookupTable"butTryFromexpects"StateArgsWithLookupTable". This breaks database persistence/retrieval for this strategy.🔎 Proposed fix - use consistent "State" prefix
pub fn as_str(&self) -> &str { use CommitStrategy::*; match self { StateArgs => "StateArgs", - StateArgsWithLookupTable => "StageArgsWithLookupTable", - StateBuffer => "StageBuffer", - StateBufferWithLookupTable => "StageBufferWithLookupTable", + StateArgsWithLookupTable => "StateArgsWithLookupTable", + StateBuffer => "StateBuffer", + StateBufferWithLookupTable => "StateBufferWithLookupTable", } }And update
TryFromaccordingly:fn try_from(value: &str) -> Result<Self, CommitPersistError> { match value { "StateArgs" => Ok(Self::StateArgs), "StateArgsWithLookupTable" => Ok(Self::StateArgsWithLookupTable), - "StageBuffer" => Ok(Self::StateBuffer), - "StageBufferWithLookupTable" => { + "StateBuffer" => Ok(Self::StateBuffer), + "StateBufferWithLookupTable" => { Ok(Self::StateBufferWithLookupTable) } _ => Err(CommitPersistError::InvalidCommitStrategy( value.to_string(), )), } }
🧹 Nitpick comments (1)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
77-104: TheBufferTaskTypematch statement (lines 77-104) is currently exhaustive and does not cause a compile error, as both existing variants (CommitandCommitDiff) are explicitly handled. Adding a catch-all arm is optional defensive programming to prevent future compile errors if new variants are added to the enum, consistent with the style used invisit_args_task. Consider adding_ => {}at the end of the match for consistency and future-proofing, especially given the enum comment indicating "Action in the future."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
magicblock-committor-service/src/persist/commit_persister.rsmagicblock-committor-service/src/persist/db.rsmagicblock-committor-service/src/persist/types/commit_strategy.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- magicblock-committor-service/src/persist/commit_persister.rs
🧰 Additional context used
🧠 Learnings (4)
📚 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:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/persist/db.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rsmagicblock-committor-service/src/persist/types/commit_strategy.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
as_str(25-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (6)
magicblock-committor-service/src/persist/db.rs (2)
775-778: LGTM!Test helper correctly updated to use the renamed
CommitStrategy::StateArgsvariant, aligning with the broader enum rename in this PR.
910-916: LGTM!Test correctly updated to use
CommitStrategy::StateBufferfor strategy update verification, consistent with the renamed enum variants.magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
26-65: LGTM!The strategy variant updates to
StateArgsWithLookupTableandStateArgsare correct. The explicit pattern matching onArgsTaskType::CommitandArgsTaskType::CommitDiffwith a catch-all_ => {}properly handles new CommitDiff support while remaining forward-compatible with any future task types.test-integration/test-committor-service/tests/test_ix_commit_local.rs (3)
70-121: LGTM!Single-account test expectations correctly updated to use
StateArgsfor smaller payloads (≤679 bytes) andStateBufferfor larger payloads, consistent with the renamed enum variants.
267-327: LGTM!Multi-account test expectations correctly updated to use
StateArgsfor the various bundle configurations, aligning with the renamed variants.
339-427: LGTM!Test expectations for bundle scenarios with lookup tables correctly updated to use
StateBufferWithLookupTableandStateArgsas appropriate.
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
7926711 to
a8cb56a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
29-43: Consider simplifying the nested conditionals.The closure logic is correct but the nested
if/elsecan be flattened for readability.🔎 Suggested simplification
let commit_strategy = |is_diff: bool| { - if is_diff { - if uses_lookup_tables { - CommitStrategy::DiffArgsWithLookupTable - } else { - CommitStrategy::DiffArgs - } - } else { - if uses_lookup_tables { - CommitStrategy::StateArgsWithLookupTable - } else { - CommitStrategy::StateArgs - } - } + match (is_diff, uses_lookup_tables) { + (true, true) => CommitStrategy::DiffArgsWithLookupTable, + (true, false) => CommitStrategy::DiffArgs, + (false, true) => CommitStrategy::StateArgsWithLookupTable, + (false, false) => CommitStrategy::StateArgs, + } };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
magicblock-committor-service/src/persist/commit_persister.rsmagicblock-committor-service/src/persist/db.rsmagicblock-committor-service/src/persist/types/commit_strategy.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rsmagicblock-committor-service/src/tasks/task_visitors/utility_visitor.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- magicblock-committor-service/src/persist/commit_persister.rs
- magicblock-committor-service/src/tasks/task_strategist.rs
- magicblock-committor-service/src/tasks/task_visitors/utility_visitor.rs
- magicblock-committor-service/src/tasks/mod.rs
- magicblock-committor-service/src/persist/db.rs
- test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rsmagicblock-committor-service/src/persist/types/commit_strategy.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.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/buffer_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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
try_optimize_tx_size(149-153)value(210-215)magicblock-committor-service/src/tasks/mod.rs (1)
try_optimize_tx_size(71-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (11)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
57-77: Good backward compatibility support in TryFrom.The implementation correctly maps legacy string values (e.g.,
"Args","FromBuffer") to the renamed variants, enabling backward compatibility with persisted data.magicblock-committor-service/src/tasks/args_task.rs (2)
125-143: LGTM! CommitDiff now correctly routes to BufferTaskType::CommitDiff.The
try_optimize_tx_sizemethod correctly handles bothCommitandCommitDiffvariants by wrapping them in the appropriateBufferTaskType. This aligns with the PR objective of executing CommitDiff directly as a BufferTask.
67-85: LGTM! CommitDiff instruction generation is correct.The
compute_diffcall correctly uses(base_account.data(), committed_account.account.data())order, which computes the diff from the base state to the committed state.magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)
45-70: LGTM! Strategy persistence for ArgsTask handles both Commit and CommitDiff correctly.The match expression correctly extracts
commit_idandpubkeyfrom both variants and applies the appropriate strategy based on the task type.
75-119: LGTM! visit_buffer_task mirrors visit_args_task correctly.The BufferTask visitor correctly uses
StateBuffer*andDiffBuffer*variants, maintaining consistency with the Args visitor's pattern.test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
68-121: Test expectations correctly updated for new strategy variants.The single-account and order-book commit tests now properly expect:
StateArgsfor small unchanged data (100 bytes)DiffArgsfor larger data with diffsDiffBufferwhen data exceeds Args size limitsThis aligns with the PR objective of executing CommitDiff directly as BufferTask.
260-452: Multiple account test expectations correctly reflect Diff strategy usage.The tests properly expect
DiffArgsfor smaller bundles andDiffBufferWithLookupTablefor larger bundles, consistent with the new CommitDiff-as-BufferTask implementation.magicblock-committor-service/src/tasks/buffer_task.rs (4)
67-83: LGTM! CommitDiff preparation correctly computes and stores the diff.The preparation logic for
CommitDiffcorrectly:
- Computes the diff with proper argument order
(base, committed)- Derives chunks from diff length
- Stores the diff as
committed_datafor buffer writesThis was previously flagged and has been fixed.
195-206: LGTM!reset_commit_idcorrectly handles both variants.The method properly updates the
commit_idfor bothCommitandCommitDiffvariants and re-computes the preparation state, which is essential since the buffer PDA depends on the commit ID.
87-98: LGTM! Test utility conversion handles CommitDiff correctly.The
From<ArgsTaskType>implementation for test contexts properly mapsCommitDifftoBufferTaskType::CommitDiff.
124-144: Verify the arg type forcommit_diff_from_bufferagainst the delegation program API.The
CommitDiffvariant at line 138 usesCommitStateFromBufferArgs, while the args-basedcommit_diff(in args_task.rs) usesCommitDiffArgs. This naming pattern suggests there may be a separateCommitDiffFromBufferArgstype that should be used instead. Confirm with the delegation program's public API thatCommitStateFromBufferArgsis the correct type for both buffer-based commit variants, or if a dedicated args type exists forcommit_diff_from_buffer.
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
a8cb56a to
7dfba55
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
110-114: Update the stale comment to reflect new strategy variant names.The comment references old variant names "Args" and "FromBuffer", but the code now uses "DiffArgs" and "DiffBuffer". Update the comment for consistency.
🔎 Proposed fix
// We cannot use 680 as changed_len because that both 679 and 680 produce encoded tx // of size 1644 (which is the max limit), but while the size of raw bytes for 679 is within // 1232 limit, the size for 680 execeds by 1 (1233). That is why we used - // 681 as changed_len where CommitStrategy goes from Args to FromBuffer. + // 681 as changed_len where CommitStrategy goes from DiffArgs to DiffBuffer. commit_book_order_account(681, CommitStrategy::DiffBuffer, false).await;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
magicblock-committor-service/src/persist/types/commit_strategy.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/persist/types/commit_strategy.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
magicblock-committor-service/src/persist/types/commit_status.rs (1)
as_str(125-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
🔇 Additional comments (8)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
70-95: LGTM!The strategy variant updates correctly distinguish between state-based commits (StateArgs for small 100-byte accounts) and diff-based commits (DiffArgs for larger accounts). This aligns with the PR's goal of supporting CommitDiff as a BufferTask.
267-448: LGTM!All multi-account test expectations have been consistently updated to use the new strategy variant names (DiffArgs, DiffBuffer, DiffBufferWithLookupTable). The mappings appropriately reflect when arguments, buffers, or address lookup tables are required based on account sizes and bundle configurations.
magicblock-committor-service/src/persist/types/commit_strategy.rs (4)
7-23: LGTM! Enum structure is well-organized.The renamed State* variants and new Diff* variants follow a consistent pattern, with clear separation between state-based and diff-based commit strategies, each offering with/without lookup table options.
26-38: LGTM! String mappings are consistent.All variant-to-string mappings correctly use the "State" prefix (not "Stage"), resolving the previous inconsistency flagged in earlier reviews.
40-48: LGTM! Diff variants correctly included.The method now includes
DiffArgsWithLookupTableandDiffBufferWithLookupTable, addressing the concern raised in the previous review.
51-72: LGTM! Backward compatibility handled well.The alias support (e.g., accepting both "Args" and "StateArgs") ensures a smooth migration path for existing persisted data while introducing the new Diff* variants.
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)
29-43: Closure approach eliminates strategy duplication.The closure pattern effectively consolidates the strategy selection logic. The implementation correctly maps
is_diffto the appropriate Diff* or State* variants based onuses_lookup_tables.
94-106: The match statement is exhaustive and requires no changes.BufferTaskTypehas only two variants—CommitandCommitDiff—both of which are explicitly handled in the match. A catch-all arm is unnecessary and would actually weaken the code by silently ignoring future variants instead of forcing them to be handled explicitly, which is Rust's intended behavior.Likely an incorrect or invalid review comment.
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
7dfba55 to
bd29c17
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
108-121: Stale comment references old variant names.The comment on lines 110-113 mentions "CommitStrategy goes from Args to FromBuffer" but the code now uses
DiffBuffer. Update the comment to reflect the new variant names.🔎 Suggested fix
// We cannot use 680 as changed_len because that both 679 and 680 produce encoded tx // of size 1644 (which is the max limit), but while the size of raw bytes for 679 is within // 1232 limit, the size for 680 execeds by 1 (1233). That is why we used - // 681 as changed_len where CommitStrategy goes from Args to FromBuffer. + // 681 as changed_len where CommitStrategy goes from DiffArgs to DiffBuffer. commit_book_order_account(681, CommitStrategy::DiffBuffer, false).await;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
magicblock-committor-service/src/persist/types/commit_strategy.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
🧠 Learnings (3)
📚 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:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
🔇 Additional comments (7)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)
26-70: LGTM! Clean refactoring addresses previous duplication concerns.The tuple extraction pattern
(commit_id, pubkey, commit_strategy)consolidates the error handling into a single block, eliminating the duplication flagged in prior reviews. The closure-based strategy selection is clear and maintainable.
73-117: LGTM! Consistent pattern applied to BufferTask handling.Same clean refactoring pattern applied here. The match is exhaustive for
BufferTaskType::CommitandBufferTaskType::CommitDiffvariants.test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
68-96: LGTM! Test expectations correctly updated for new strategy variants.The pattern is clear and intentional:
- Small accounts (100 bytes) →
StateArgs(full state fits in args)- Larger accounts (800+ bytes) →
DiffArgs(diff-based commit)This aligns with the PR objective of supporting CommitDiff directly as a BufferTask.
260-426: LGTM! Bundle test expectations consistently updated.All multi-account tests correctly use the new Diff* variants:
DiffArgsfor smaller bundles that fit in transaction argsDiffBufferWithLookupTablefor larger bundles requiring buffers and lookup tablesThe test coverage adequately exercises the new CommitDiff-as-BufferTask functionality.
magicblock-committor-service/src/persist/types/commit_strategy.rs (3)
3-23: LGTM! Well-structured enum extension.The new Diff* variants mirror the existing State* variants with consistent naming and documentation. The enum covers all 8 combinations (State/Diff × Args/Buffer × With/Without LookupTable).
25-48: LGTM! Previous review concerns addressed.Both past issues are resolved:
- String consistency:
as_str()now returns consistent "State*" and "Diff*" prefixesuses_lookup()correctly includes all four lookup-table variants
51-71: LGTM! Backward-compatible parsing with legacy alias support.The
TryFromimplementation correctly maps legacy names (Args,FromBuffer, etc.) to the newState*variants while supporting the newDiff*variants directly. This ensures existing persisted data remains parseable after the variant rename.
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
bd29c17 to
815b0b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
120-123: Consider updating the comment to reference new variant names.The comment mentions "Args to FromBuffer" but the code now uses "DiffArgs to DiffBuffer". While the explanation remains accurate, updating the comment to reference the new variant names would improve consistency.
🔎 Proposed update
-// 681 as changed_len where CommitStrategy goes from Args to FromBuffer. +// 681 as changed_len where CommitStrategy goes from DiffArgs to DiffBuffer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
magicblock-committor-service/src/persist/types/commit_strategy.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
🔇 Additional comments (7)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)
29-68: LGTM! Clean refactor with correct logic.The closure-based approach for computing
CommitStrategycorrectly distinguishes between Diff* and State* variants based on theis_diffflag, and appropriately handles lookup table usage. The extraction ofcommit_id,pubkey, andcommit_strategyfrom bothCommitandCommitDifftask types is well-structured and aligns with the PR's goal of supporting CommitDiff as a BufferTask.
76-114: LGTM! Consistent with args task implementation.The buffer task visitor follows the same clean pattern as the args task visitor, correctly computing CommitStrategy variants based on the
is_diffflag and handling bothCommitandCommitDifftask types appropriately.test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
70-131: LGTM! Test expectations correctly updated.The test expectations have been properly updated to use the new CommitStrategy variants:
StateArgsfor small accounts (<= 256 bytes)DiffArgsfor larger accounts and diff-based commitsDiffBufferfor very large changesThese expectations align well with the PR's goal of supporting CommitDiff tasks without degrading to CommitState.
277-434: LGTM! Multi-account test expectations are consistent.The multi-account test expectations have been systematically updated to use
DiffArgsandDiffBufferWithLookupTablevariants based on bundle size and lookup table usage. All updates are consistent with the new CommitStrategy naming scheme.magicblock-committor-service/src/persist/types/commit_strategy.rs (3)
7-23: LGTM! Clean enum structure with parallel State and Diff variants.**The renamed State* variants and newly added Diff* variants follow a consistent naming convention with clear documentation. The parallel structure between State and Diff variants makes the enum easy to understand and maintain.
26-48: LGTM! Helper methods are correct and complete.Both
as_str()anduses_lookup()are properly implemented:
as_str()consistently uses "State" and "Diff" prefixes (no "Stage" typo)uses_lookup()correctly includes all four WithLookupTable variants (both State and Diff*)The past review concerns have been addressed.
51-72: LGTM! TryFrom provides excellent backward compatibility.The
TryFromimplementation is well-designed:
- Accepts both old variant names ("Args", "FromBuffer", etc.) and new names ("StateArgs", "StateBuffer", etc.) for backward compatibility
- Properly handles all new Diff* variants
- Appropriate error handling for invalid strategy strings
This ensures that old persisted data will continue to work while new code can use the updated variant names.
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
815b0b3 to
543381e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
43-68: LGTM!The tuple extraction pattern effectively consolidates the
set_commit_strategycall to a single location (lines 58-68), eliminating the duplication that existed in the previous implementation. The early return for non-Commit/CommitDiff variants is appropriate since other task types don't require commit strategy persistence.Note: The past review comment suggested extracting error handling into a helper function. While that would reduce a few lines of code, the current approach already achieves the primary goal of avoiding duplicated
set_commit_strategycalls.magicblock-committor-service/src/persist/types/commit_strategy.rs (2)
26-38: LGTM!The
as_str()method correctly returns consistent string representations for all variants, matching the variant names exactly. The past review comment about "State" vs "Stage" prefix inconsistency has been fully addressed.
40-48: LGTM!The
uses_lookup()method correctly includes all four*WithLookupTablevariants (both State* and Diff*). The past review comment about missing Diff variants has been fully addressed.
🧹 Nitpick comments (1)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
90-102: Consider adding a catch-all clause for future-proofing.The
BufferTaskTypeenum currently has only two variants (CommitandCommitDiff), making this pattern match exhaustive. However, since the enum includes a comment indicating future variants may be added, adding a_ => returnclause would be defensive programming that prevents potential compilation errors if new variants are introduced later. Unlike theArgsTaskTypeenum (which has five variants and requires the catch-all),BufferTaskTypeis optional but recommended for consistency with defensive patterns in the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
magicblock-committor-service/src/persist/types/commit_strategy.rsmagicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rstest-integration/test-committor-service/tests/test_ix_commit_local.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
🔇 Additional comments (6)
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
29-41: LGTM!The
commit_strategyclosure correctly maps theis_diffanduses_lookup_tablesflags to the appropriateCommitStrategyvariants. The logic is clear and covers all four combinations for Args-based strategies.test-integration/test-committor-service/tests/test_ix_commit_local.rs (3)
68-116: LGTM!The test expectations correctly reflect the strategy transition:
StateArgsfor accounts ≤256 bytes (full state fits in transaction args)DiffArgsfor accounts >256 bytes (diff-based commit still fits in args)This aligns with the PR objective of supporting CommitDiff without degrading to CommitState.
118-141: LGTM!The order book tests correctly use
DiffArgsfor smaller diffs and transition toDiffBufferat 681 bytes. The inline comment (lines 130-133) clearly explains the threshold logic based on transaction size limits (1644 bytes encoded, 1232 bytes raw).
281-471: LGTM!The bundle tests consistently use
Diff*variants (DiffArgs,DiffBuffer,DiffBufferWithLookupTable) across various bundle sizes and account configurations. The strategy selection appropriately considers:
- Transaction size constraints (Args vs Buffer)
- Lookup table usage (based on bundle size optimization)
This correctly validates the new CommitDiff-as-BufferTask functionality.
magicblock-committor-service/src/persist/types/commit_strategy.rs (2)
3-23: LGTM!The enum structure is well-organized with clear separation between:
- State-based variants (StateArgs, StateBuffer) for full state commits
- Diff-based variants (DiffArgs, DiffBuffer) for differential commits
- Lookup table variants for transaction optimization
The naming is consistent and the comments accurately describe each variant's purpose.
51-72: LGTM!The
TryFrom<&str>implementation provides appropriate backward compatibility by aliasing old variant names to their State* equivalents:
- "Args" → StateArgs
- "FromBuffer" → StateBuffer
This ensures existing persisted commit strategies can be parsed correctly. The new Diff* variants don't require aliases since they're introduced in this PR.
Yes. I made the following additions:
One important change I’m not 100% confident aboutSince
I’d really appreciate extra scrutiny on this part during review, since it affects persisted data and possibly long-term compatibility (?). |
Problem
The previous PR #575 in this stack implements support for
CommitDiffthat executes asArgsTask. When the transaction size grows beyond the limit,CommitDifffirst degrades toCommitStateand then gets executed asBufferTaskto reduce the transaction size.Degration of
CommitDifftoCommitStateis not a desirable thing.Solution
So this PR implements support for
CommitDiffasBufferTask. Means degradation ofCommitDifftoCommitStateis not required anymore.Related
Another PR magicblock-labs/delegation-program#118 that implements
CommitDiffFromBufferinstruction, is created in the delegation-program.Screenshot
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.