Strip failed undelegations #640
Conversation
📝 WalkthroughWalkthroughAdded an UndelegationError variant to executor error enums and mapped undelegate instruction failures into it; implemented handle_undelegation_error to truncate a TransactionStrategy at the first Undelegate task and return a cleanup strategy; wired undelegation-aware recovery into single- and two-stage executors (returning cleanup Continue in finalize/patch paths, logging/breaking where appropriate); updated try_from_transaction_error to dispatch on (TaskType, InstructionError) and wrap indexed instruction errors; extended the flexi-counter test program and test utilities to simulate undelegation failures and added tests exercising parsing and recovery. Changes
Sequence Diagram(s)sequenceDiagram
participant Exec as IntentExecutor
participant Strat as TransactionStrategy
participant Map as ErrorMapper
participant Recover as handle_undelegation_error
Exec->>Strat: execute/send transaction
Strat-->>Exec: TransactionError (InstructionError @ index)
Exec->>Map: try_from_transaction_error(tx_err, index)
Map-->>Exec: TransactionStrategyExecutionError::UndelegationError(tx_err, signature)
alt Single-stage finalization
Exec->>Recover: handle_undelegation_error(strategy)
Recover-->>Exec: Cleanup strategy (tasks before Undelegate)
Exec->>Exec: Continue with cleanup strategy
else Two-stage commit path
Exec-->>Exec: Log unexpected undelegation error and Break
else Two-stage finalize path
Exec->>Recover: handle_undelegation_error(strategy)
Recover-->>Exec: Cleanup strategy
Exec->>Exec: Continue finalize with cleanup strategy
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Focus: correctness of index→TaskType mapping in Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/intent_executor/error.rs (1)
110-118: Consider adding a dedicated metrics label for undelegation failures
IntentExecutorError::UndelegationError(_, _)currently falls under the catch‑all"failed"label, while Actions, CPI limit, and CommitID have dedicated labels. For observability and alerting, it may be useful to distinguish undelegation failures as well, e.g.:fn value(&self) -> &str { match self { IntentExecutorError::ActionsError(_, _) => "actions_failed", IntentExecutorError::CpiLimitError(_, _) => "cpi_limit_failed", IntentExecutorError::CommitIDError(_, _) => "commit_nonce_failed", + IntentExecutorError::UndelegationError(_, _) => "undelegation_failed", _ => "failed", } }This keeps metrics granular as new error classes are introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
magicblock-committor-service/src/intent_executor/error.rs(4 hunks)magicblock-committor-service/src/intent_executor/mod.rs(3 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(1 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(2 hunks)test-integration/programs/flexi-counter/src/processor.rs(2 hunks)test-integration/programs/flexi-counter/src/state.rs(1 hunks)test-integration/test-committor-service/tests/test_intent_executor.rs(12 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(2 hunks)test-integration/test-committor-service/tests/utils/instructions.rs(1 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/programs/flexi-counter/src/state.rstest-integration/test-committor-service/tests/utils/instructions.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/test_intent_executor.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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-18T08:47:39.681Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.681Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rstest-integration/test-committor-service/tests/test_intent_executor.rs
🧬 Code graph analysis (5)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
test-integration/test-committor-service/tests/utils/instructions.rs (1)
init_account_and_delegate_ixs(20-51)
magicblock-committor-service/src/intent_executor/error.rs (2)
magicblock-rpc-client/src/lib.rs (2)
error(193-195)signature(82-90)magicblock-committor-service/src/tasks/args_task.rs (1)
task_type(146-153)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
test-integration/programs/flexi-counter/src/processor.rs (3)
programs/magicblock/src/mutate_accounts/account_mod_data.rs (1)
data(137-144)programs/magicblock/src/magic_context.rs (1)
deserialize(18-26)programs/magicblock/src/task_context.rs (1)
deserialize(82-90)
test-integration/test-committor-service/tests/test_intent_executor.rs (2)
test-integration/programs/flexi-counter/src/state.rs (2)
pda(34-37)new(16-22)test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
⏰ 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). (1)
- GitHub Check: run_make_ci_lint
🔇 Additional comments (15)
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (2)
178-184: LGTM! Consistent error handling pattern.The new
UndelegationErrorhandling follows the same pattern asActionsErrorandCommitIDError, correctly patching the strategy for retry and returning cleanup data.
12-13: LGTM! Test constants for undelegation failure simulation.The constants are appropriately scoped for test purposes and will enable undelegation error path testing.
test-integration/test-committor-service/tests/utils/instructions.rs (1)
20-30: LGTM! Clean parameter addition with safe default.The optional label parameter is properly handled with a sensible default value.
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
180-184: LGTM! Correct handling of unexpected undelegation in commit phase.Undelegation should not occur during the commit phase, so treating
UndelegationErroras unexpected and breaking the flow is architecturally correct.
222-228: LGTM! Proper recoverable error handling in finalize phase.The
UndelegationErrorhandling in the finalize phase correctly patches the strategy and prepares cleanup data for retry, consistent with howActionsErroris handled.test-integration/test-committor-service/tests/utils/transactions.rs (1)
127-146: LGTM! Clean parameter propagation.The optional label parameter is properly threaded through the test utility.
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
115-116: LGTM! Test updated for new API.Passing
Nonefor the label parameter is correct for this test case.
397-402: LGTM! Test updated for new API.Passing
Nonefor the label parameter is correct for this test case.magicblock-committor-service/src/intent_executor/mod.rs (3)
450-475: LGTM! Correct implementation of undelegation error recovery.The method correctly:
- Locates the first
Undelegatetask- Removes it and all subsequent tasks via
drain(position..)- Recomputes lookup table keys
- Returns an empty strategy if no
Undelegatetask exists
552-553: LGTM! Correct error status mapping.Treating
UndelegationErrorlikeCpiLimitErrorby mapping toNone(no status update) is correct since it's a recoverable error that will be retried after patching the strategy.
791-792: LGTM! Good safety note on cache invalidation.The comment correctly documents that cache reset occurs even when undelegation is removed from the strategy, ensuring safety for subsequent commits.
magicblock-committor-service/src/intent_executor/error.rs (1)
43-44: Undelegation error variants are wired consistently across executor layersAdding
UndelegationError(#[source] TransactionError, Option<Signature>)to bothIntentExecutorErrorandTransactionStrategyExecutionError, and mapping it throughfrom_strategy_execution_error, keeps the shape and provenance of undelegation failures consistent with existingActionsError/CommitIDErrorpaths. No changes needed here.Also applies to: 99-102, 126-127
test-integration/test-committor-service/tests/test_intent_executor.rs (2)
38-40: Flexi-counter imports match new undelegation test behaviorBringing in
FlexiCounterInstructionandFAIL_UNDELEGATION_LABELhere lines up with their later usage infailing_undelegate_actionand the new undelegation tests. The import structure remains clear and localized; no change needed.
121-121: Optionallabelis plumbed cleanly throughsetup_counterandinit_and_delegate_account_on_chainThe refactor to:
- Extend
setup_counterto acceptlabel: Option<String>and forward it toinit_and_delegate_account_on_chain, and- Update all call sites to pass
NoneorSome(FAIL_UNDELEGATION_LABEL.to_string())explicitly,is consistent and keeps the intent of each test clear (labeled vs. unlabeled counters). This gives you precise control over when undelegation is supposed to fail without impacting existing scenarios. No changes needed.
Also applies to: 222-222, 283-283, 340-341, 433-433, 483-483, 540-540, 627-627, 822-825, 828-829
test-integration/programs/flexi-counter/src/state.rs (1)
12-13: LGTM for undelegation failure constantsThe label and code are clearly named, typed appropriately, and fit well with the flexi-counter test program’s role in simulating undelegation failures. No issues from this file’s perspective.
| /// Handles actions error, stripping away actions | ||
| /// Returns [`TransactionStrategy`] to be cleaned up |
There was a problem hiding this comment.
Fix copy-paste error in documentation.
The comment says "Handles actions error, stripping away actions" but this method handles undelegation errors.
Apply this diff:
- /// Handles actions error, stripping away actions
+ /// Handles undelegation error, stripping away undelegation and subsequent tasks
/// Returns [`TransactionStrategy`] to be cleaned up📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Handles actions error, stripping away actions | |
| /// Returns [`TransactionStrategy`] to be cleaned up | |
| /// Handles undelegation error, stripping away undelegation and subsequent tasks | |
| /// Returns [`TransactionStrategy`] to be cleaned up |
🤖 Prompt for AI Agents
In magicblock-committor-service/src/intent_executor/mod.rs around lines 448 to
449, the doc comment incorrectly states "Handles actions error, stripping away
actions" though the function actually handles undelegation errors; update the
documentation comment to accurately describe that this method handles
undelegation errors (e.g., "Handles undelegation error, stripping away
undelegations") and ensure wording matches the function behavior and terminology
used elsewhere in the module.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test-integration/test-committor-service/tests/test_intent_executor.rs (1)
380-418: Extend undelegation error recovery test to assert commit-buffer cleanup as well
test_undelegation_error_recoverynicely validates that:
- An intent whose undelegation is guaranteed to fail (via
FAIL_UNDELEGATION_LABEL) still results inOkfromexecute.- The executor returns
ExecutionOutput::SingleStage(_), i.e., recovery happens without switching to a two-stage flow.- On-chain state and TableMania refcounts are checked via
verify.However, because
verifyis called with&HashMap::new()forcommit_ids_by_pk, theverify_buffers_cleaned_upstep becomes a no‑op here (there are no commit IDs to check against), so this test doesn’t currently assert that commit buffers are actually cleaned up in the undelegation‑failure recovery path.For symmetry with
test_commit_id_error_recoveryand the other recovery tests, you could also capture the commit ID for this account and pass it intoverify, e.g.:- let TestEnv { - fixture, - intent_executor, - task_info_fetcher: _, - pre_test_tablemania_state, - } = TestEnv::setup().await; + let TestEnv { + fixture, + intent_executor, + task_info_fetcher, + pre_test_tablemania_state, + } = TestEnv::setup().await; @@ - verify( - &fixture.table_mania, - fixture.rpc_client.get_inner(), - &HashMap::new(), - &pre_test_tablemania_state, - &[committed_account], - ) - .await; + let commit_ids_by_pk: HashMap<_, _> = [&committed_account] + .iter() + .map(|el| { + ( + el.pubkey, + task_info_fetcher.peek_commit_id(&el.pubkey).unwrap(), + ) + }) + .collect(); + + verify( + &fixture.table_mania, + fixture.rpc_client.get_inner(), + &commit_ids_by_pk, + &pre_test_tablemania_state, + &[committed_account], + ) + .await;This would ensure the undelegation‑error recovery path is also covered by the buffer‑cleanup assertions, not just the refcount and data checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-committor-service/src/intent_executor/error.rs(4 hunks)test-integration/test-committor-service/tests/test_intent_executor.rs(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.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:
test-integration/test-committor-service/tests/test_intent_executor.rs
🧬 Code graph analysis (2)
magicblock-committor-service/src/intent_executor/error.rs (4)
magicblock-rpc-client/src/lib.rs (2)
error(193-195)signature(82-90)magicblock-committor-service/src/tasks/buffer_task.rs (1)
task_type(122-126)magicblock-committor-service/src/tasks/args_task.rs (1)
task_type(146-153)magicblock-committor-service/src/tasks/mod.rs (1)
task_type(89-89)
test-integration/test-committor-service/tests/test_intent_executor.rs (5)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)magicblock-committor-service/src/tasks/args_task.rs (2)
instruction(45-98)new(36-41)test-integration/programs/flexi-counter/src/state.rs (2)
pda(34-37)new(16-22)test-integration/test-tools/src/conversions.rs (1)
err(8-8)test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
⏰ 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 (10)
magicblock-committor-service/src/intent_executor/error.rs (5)
43-44: LGTM! Variant structure is consistent with existing error patterns.The
UndelegationErrorvariant follows the same payload structure asActionsErrorandCommitIDError, maintaining consistency in the error hierarchy.
99-102: LGTM! Error mapping is correct.The passthrough mapping from
TransactionStrategyExecutionError::UndelegationErrortoIntentExecutorError::UndelegationErrorpreserves all context and is consistent with other variant mappings.
110-119: Verify whetherUndelegationErrorshould have a dedicated metrics label.The
LabelValueimplementation provides specific labels forActionsError,CpiLimitError, andCommitIDError, but the newUndelegationErrorvariant falls through to the catch-all case returning"failed".If undelegation failures should be tracked separately for observability, consider adding an explicit case like
IntentExecutorError::UndelegationError(_, _) => "undelegation_failed".
126-127: LGTM! Variant structure is consistent.The
UndelegationErrorvariant inTransactionStrategyExecutionErrormaintains consistency with the corresponding variant inIntentExecutorErrorand follows established patterns.
165-204: LGTM! Error mapping logic is sound and well-structured.The refactored error mapping correctly:
- Handles the instruction offset to account for budget instructions
- Guards against underflow and out-of-bounds access
- Maps task-specific failures to appropriate error variants (CommitIDError, ActionsError, UndelegationError)
- Falls back to returning the original error for unhandled cases
The pattern matching on
(task_type, instruction_err)tuples makes the logic clear and maintainable. The comment update on line 165 addresses the past review feedback.test-integration/test-committor-service/tests/test_intent_executor.rs (5)
37-40: ImportingFlexiCounterInstructionandFAIL_UNDELEGATION_LABELis appropriate for the new failure-path testsPulling in
FlexiCounterInstructionandFAIL_UNDELEGATION_LABELfrom the flexi-counter test program cleanly exposes the knobs you need to drive undelegation failures and base actions from the program itself, instead of hard-coding test-only constants here. This keeps the tests aligned with the on-chain test program and avoids duplication.Based on learnings
121-121: Consistent use ofsetup_counter(..., None)simplifies counter initialization across testsRefactoring the various parsing/recovery tests to go through
setup_counter(COUNTER_SIZE, None)centralizes the “plain” counter setup (including delegation and owner fixup) and keeps label-handling logic in one place. This reduces duplication and makes it easy to later tweak how counters are prepared without touching every test.Also applies to: 222-222, 283-283, 540-540, 627-627
161-208: Undelegation error parsing test mirrors existing patterns; be aware of string/position fragilityThe new
test_undelegation_error_parsingdoes a good job of:
- Using
FAIL_UNDELEGATION_LABELto deterministically trigger an undelegation failure.- Verifying that the executor surfaces
TransactionStrategyExecutionError::UndelegationError(_, _).- Asserting that the formatted error string contains the expected undelegation message.
This matches the existing commit‑ID / actions / CPI parsing tests and gives good coverage of the new error variant. Just note that pinning the full
EXPECTED_ERR_MSG(including instruction index and Solana’s error text) is intentionally brittle — any change in transaction layout or upstream error formatting will require updating this literal. If that trade-off is acceptable (as with the other parsing tests), this looks solid.
340-341: ExplicitNonelabel atinit_and_delegate_account_on_chaincall sites keeps intent clearUpdating the
init_and_delegate_account_on_chaincall sites to passNonefor the newlabel: Option<String>parameter makes it explicit that these scenarios are not using the failure label and should behave like the original “happy path” tests. This keeps the API usage uniform and avoids confusion with the new undelegation-failure scenarios.Also applies to: 433-433, 483-483
822-829:setup_counterhelper cleanly encapsulates counter auth + delegated account preparationThe new
setup_counter(counter_bytes, label)helper is a nice improvement:
- It reuses
init_and_delegate_account_on_chainso all counter setup (airdrop, init, realloc, delegate) stays in one place.- It returns both the
Keypairand the delegatedAccount, which fits existing test call sites that need to both operate on the account and fund/use the authority.- It centralizes the
account.owner = program_flexi_counter::id();fixup, avoiding scattered owner overrides.This should make future changes to how counters are initialized (or how labels are threaded through) much easier.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test-integration/test-committor-service/tests/test_intent_executor.rs (1)
382-421: Test validates undelegation recovery; consider verifying commit buffer cleanup.The test correctly validates that:
- An intent with a failing undelegation still returns
Okfromexecute- Recovery produces
ExecutionOutput::SingleStage- Final on-chain state and TableMania refcounts are verified
However,
verifyis called with an emptycommit_ids_by_pk, making theverify_buffers_cleaned_upstep a no-op. If undelegation fails after commit operations create buffers, you should capture and verify those buffers are cleaned up during recovery.To add buffer cleanup verification:
// Before verify, capture commit IDs if they exist let commit_ids_by_pk: HashMap<_, _> = [&committed_account] .iter() .filter_map(|el| { task_info_fetcher .peek_commit_id(&el.pubkey) .map(|id| (el.pubkey, id)) }) .collect(); tokio::time::sleep(Duration::from_secs(1)).await; verify( &fixture.table_mania, fixture.rpc_client.get_inner(), &commit_ids_by_pk, // Instead of HashMap::new() &pre_test_tablemania_state, &[committed_account], ) .await;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-integration/test-committor-service/tests/test_intent_executor.rs(17 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.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:
test-integration/test-committor-service/tests/test_intent_executor.rs
🧬 Code graph analysis (1)
test-integration/test-committor-service/tests/test_intent_executor.rs (2)
test-integration/programs/flexi-counter/src/state.rs (2)
pda(34-37)new(16-22)test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
🔇 Additional comments (4)
test-integration/test-committor-service/tests/test_intent_executor.rs (4)
38-39: LGTM! Test constant import is appropriate.The addition of
FAIL_UNDELEGATION_LABELfrom the flexi-counter test contract enables the new undelegation failure scenarios. This is a clean test-only addition.
121-121: LGTM! Call sites correctly updated for optional label parameter.All existing calls to
setup_counterhave been consistently updated to passNonefor the new optionallabelparameter, maintaining backward compatibility.Also applies to: 222-222, 283-283, 340-341, 436-436, 488-488, 547-547, 635-635
161-208: LGTM! Undelegation error parsing test correctly validates the new error path.The test properly:
- Forces an undelegation failure via
FAIL_UNDELEGATION_LABEL- Asserts the correct error variant
TransactionStrategyExecutionError::UndelegationError- Verifies the error message content
Note that
EXPECTED_ERR_MSGcouples the test to Solana's error formatting and instruction ordering. This is consistent with other tests in the file and acceptable if you're comfortable updating tests when those details change.
831-842: LGTM! Clean extension of test utility function.The addition of the optional
labelparameter tosetup_counteris well-designed:
- Uses
Option<String>for backward compatibility- Properly threads the label through to
init_and_delegate_account_on_chain- Enables test scenarios that require labeled counters
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test-integration/test-committor-service/tests/test_intent_executor.rs (1)
383-422: Test validates undelegation error recovery correctly.The test successfully verifies that when undelegation fails (via
FAIL_UNDELEGATION_LABEL), the executor recovers gracefully and produces aSingleStageoutput. The core behavior is properly tested.Note: A previous review suggested capturing commit IDs (via
task_info_fetcher.peek_commit_id) and passing them toverify()instead ofHashMap::new()at line 417, which would enable verification of commit buffer cleanup similar totest_commit_id_error_recovery. This remains an optional enhancement for test coverage symmetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-integration/test-committor-service/tests/test_intent_executor.rs(17 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.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_intent_executor.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:
test-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.rs
🧬 Code graph analysis (1)
test-integration/test-committor-service/tests/test_intent_executor.rs (5)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)magicblock-committor-service/src/tasks/args_task.rs (2)
instruction(45-98)new(36-41)test-integration/programs/flexi-counter/src/state.rs (2)
pda(34-37)new(16-22)test-integration/test-tools/src/conversions.rs (1)
err(8-8)test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
⏰ 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_test
- GitHub Check: Build Project
- GitHub Check: run_make_ci_lint
🔇 Additional comments (2)
test-integration/test-committor-service/tests/test_intent_executor.rs (2)
38-40: Import of test constant looks good.The
FAIL_UNDELEGATION_LABELimport is correctly added and used in the new undelegation error tests.
121-121: Label parameter propagation implemented correctly.The
setup_counterfunction signature has been properly updated to accept an optionallabelparameter, and all call sites have been consistently updated to passNonewhere the label is not needed. The new undelegation error tests correctly passSome(FAIL_UNDELEGATION_LABEL.to_string())to trigger the failure condition.Also applies to: 223-223, 284-284, 341-342, 437-437, 489-489, 548-548, 636-636, 832-843
test-integration/test-committor-service/tests/test_intent_executor.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-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-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:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧬 Code graph analysis (1)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
⏰ 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 (2)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
7-7: LGTM: Imports support the new serialized account data initialization.The
borsh::to_vecandFlexiCounterimports are correctly added to support serializing realistic account state for the tests.Also applies to: 22-22
117-128: Account initialization correctly serializes FlexiCounter state.The code change is correct and follows established patterns across the test suite. The function signature of
init_and_delegate_account_on_chainacceptslabel: Option<String>as its third parameter, so passingNoneis valid. The FlexiCounter serialization approach (create struct, serialize withto_vec(), resize to match byte length) is consistent with usage throughout all integration tests includingtest_intent_executor.rs,test_schedule_intent.rs, andtest_ledger_restore.rs. The test data withcount=101, updates=0, label="Counter"is properly initialized and follows the standard pattern.
| let (pda, mut pda_acc) = init_and_delegate_account_on_chain( | ||
| &counter_auth, | ||
| bytes as u64, | ||
| None, | ||
| ) | ||
| .await; | ||
|
|
||
| pda_acc.owner = program_flexi_counter::id(); | ||
| pda_acc.data = vec![0u8; bytes]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent account data initialization between single and multiple account paths.
In the multiple account path, account data is initialized as zeroed bytes (vec![0u8; bytes]), while the single account path creates a properly serialized FlexiCounter. This inconsistency could lead to different test behaviors:
- Single account tests (lines 120-128): Use serialized
FlexiCounterwith label="Counter", updates=0, count=101 - Multiple account tests (lines 414-415): Use zeroed bytes without any FlexiCounter structure
Consider aligning both paths to use the same initialization pattern for consistency:
+ let counter = FlexiCounter {
+ label: "Counter".to_string(),
+ updates: 0,
+ count: 0,
+ };
+ let mut data = to_vec(&counter).unwrap();
+ data.resize(bytes, 0);
+ pda_acc.data = data;
pda_acc.owner = program_flexi_counter::id();
- pda_acc.data = vec![0u8; bytes];If the zeroed bytes are intentional for testing different scenarios, consider adding a comment explaining the rationale.
🤖 Prompt for AI Agents
In test-integration/test-committor-service/tests/test_ix_commit_local.rs around
lines 407 to 415, the multiple-account path initializes pda_acc.data with zeroed
bytes while the single-account path uses a properly serialized FlexiCounter
(label="Counter", updates=0, count=101), causing inconsistent test setup; update
the multiple-account path to serialize and assign the same FlexiCounter
structure to pda_acc.data (or, if zeroed bytes are intentional, add a clear
comment explaining why this different initialization is required) so both paths
use consistent account data initialization.
taco-paco
force-pushed
the
feat/base-layer-ix/strip-failed-undelegation
branch
from
ff9d2a5 to
150554e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
magicblock-committor-service/src/intent_executor/error.rs (1)
186-187: Simplify the error reconstruction.The pattern match already binds
instruction_errtoInstructionError::Custom(NONCE_OUT_OF_ORDER), so reconstructing it is redundant. This mirrors theActionandUndelegatebranches and avoids unnecessary reconstruction.Apply this diff:
- tx_err_helper(InstructionError::Custom( - NONCE_OUT_OF_ORDER, - )), + tx_err_helper(instruction_err),magicblock-committor-service/src/intent_executor/mod.rs (1)
448-449: Fix copy-paste error in documentation.The comment says "Handles actions error, stripping away actions" but this method handles undelegation errors.
Apply this diff:
- /// Handles actions error, stripping away actions + /// Handles undelegation error, stripping away undelegation and subsequent tasks /// Returns [`TransactionStrategy`] to be cleaned uptest-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
407-415: Inconsistent account data initialization between paths persists.The multiple-account path still initializes account data with zeroed bytes (
vec![0u8; bytes]) while the single-account path now uses a properly serializedFlexiCounter(lines 120-128). This inconsistency could lead to different test behaviors and reduces test realism.Consider applying the same initialization pattern as the single-account path:
+ let counter = FlexiCounter { + label: "Counter".to_string(), + updates: 0, + count: 0, + }; + let mut data = to_vec(&counter).unwrap(); + data.resize(bytes, 0); + pda_acc.data = data; pda_acc.owner = program_flexi_counter::id(); - pda_acc.data = vec![0u8; bytes];If zeroed bytes are intentional for testing different scenarios, add a comment explaining the rationale.
test-integration/test-committor-service/tests/test_intent_executor.rs (2)
382-421: Recovery test passes empty commit IDs map, skipping buffer cleanup verification.The test correctly validates that undelegation error recovery succeeds and produces
SingleStageoutput. However, passing an emptyHashMaptoverify(line 416) causesverify_buffers_cleaned_upto skip assertions.For symmetry with other recovery tests, consider capturing commit IDs before verification:
+ let commit_ids_by_pk: HashMap<_, _> = [&committed_account] + .iter() + .map(|el| { + ( + el.pubkey, + task_info_fetcher.peek_commit_id(&el.pubkey).unwrap(), + ) + }) + .collect(); + tokio::time::sleep(Duration::from_secs(1)).await; verify( &fixture.table_mania, fixture.rpc_client.get_inner(), - &HashMap::new(), + &commit_ids_by_pk, &pre_test_tablemania_state, &[committed_account], )This would ensure commit buffers are verified as cleaned up after undelegation recovery.
371-371: Fixed delays acceptable for integration tests, but explicit polling would be more robust.The 1-second sleep delays allow blockchain state to stabilize before verification, which is a common pattern in blockchain integration tests. However, explicit polling for expected state would be more reliable.
Consider implementing a polling helper to wait for state stabilization rather than fixed delays:
async fn wait_for_state_stabilization( rpc_client: &Arc<RpcClient>, condition: impl Future<Output = bool>, max_attempts: u32, ) -> Result<(), String> { for _ in 0..max_attempts { tokio::time::sleep(Duration::from_millis(100)).await; if condition.await { return Ok(()); } } Err("State did not stabilize".to_string()) }This would improve test reliability by only waiting as long as necessary.
Also applies to: 412-412, 467-467, 526-526, 599-599, 712-712
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
magicblock-committor-service/src/intent_executor/error.rs(4 hunks)magicblock-committor-service/src/intent_executor/mod.rs(3 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(1 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(2 hunks)test-integration/programs/flexi-counter/src/processor.rs(2 hunks)test-integration/programs/flexi-counter/src/state.rs(1 hunks)test-integration/test-committor-service/tests/test_intent_executor.rs(17 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(4 hunks)test-integration/test-committor-service/tests/utils/instructions.rs(1 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 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-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/programs/flexi-counter/src/state.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/utils/instructions.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/error.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rstest-integration/test-committor-service/tests/test_intent_executor.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/utils/transactions.rstest-integration/test-committor-service/tests/utils/instructions.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/test_intent_executor.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:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/flexi-counter/src/processor.rs
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.rs
🧬 Code graph analysis (4)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
test-integration/test-committor-service/tests/utils/instructions.rs (1)
init_account_and_delegate_ixs(20-51)
magicblock-committor-service/src/intent_executor/error.rs (3)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
task_type(122-126)magicblock-committor-service/src/tasks/args_task.rs (1)
task_type(146-153)magicblock-committor-service/src/tasks/mod.rs (1)
task_type(89-89)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
test-integration/test-committor-service/tests/test_intent_executor.rs (5)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)magicblock-committor-service/src/tasks/args_task.rs (2)
instruction(45-98)new(36-41)test-integration/programs/flexi-counter/src/state.rs (2)
pda(34-37)new(16-22)test-integration/test-tools/src/conversions.rs (1)
err(8-8)test-integration/test-committor-service/tests/utils/transactions.rs (1)
init_and_delegate_account_on_chain(127-224)
🔇 Additional comments (15)
magicblock-committor-service/src/intent_executor/error.rs (1)
110-119: Verify metrics labeling for UndelegationError.The
LabelValueimplementation doesn't explicitly handleUndelegationError, so it falls through to the default"failed"label. While this may be intentional, it's worth confirming that undelegation errors shouldn't have a distinct metric label (e.g.,"undelegation_failed") for better observability.test-integration/programs/flexi-counter/src/state.rs (1)
12-13: LGTM!The undelegation failure constants are clearly named and provide a simple mechanism for testing undelegation error paths.
test-integration/test-committor-service/tests/utils/instructions.rs (1)
20-30: LGTM!The optional label parameter provides flexibility for test scenarios while maintaining backward compatibility through the default value.
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (1)
178-184: LGTM!The
UndelegationErrorhandling follows the established pattern for recoverable errors, invokinghandle_undelegation_errorto produce cleanup data and continuing with a retry.magicblock-committor-service/src/intent_executor/two_stage_executor.rs (2)
180-184: LGTM!Correctly treats
UndelegationErrorin the commit phase as unexpected and breaks, since undelegation should occur during finalization, not commit. The error message and handling are appropriate.
222-228: LGTM!The
UndelegationErrorhandling in the finalize phase correctly produces cleanup data viahandle_undelegation_errorand continues with a retry, consistent with the recoverable error pattern.test-integration/test-committor-service/tests/utils/transactions.rs (1)
127-146: LGTM!The label parameter is correctly threaded through the initialization flow, enabling test scenarios to customize counter labels for undelegation failure testing.
magicblock-committor-service/src/intent_executor/mod.rs (2)
450-475: LGTM!The implementation correctly locates the first
Undelegatetask, truncates the strategy from that point forward, and returns a cleanup strategy with the removed tasks. The logic handles the case where noUndelegatetask exists by returning an empty strategy.
552-553: LGTM!Correctly maps
UndelegationErrortoNone, preventing premature status updates since the error is recoverable and will be retried with cleanup.test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
7-7: LGTM: Necessary imports for FlexiCounter serialization.The new imports support the test changes that serialize
FlexiCounterdata structures using Borsh.Also applies to: 22-22
117-128: LGTM: Single-account path now uses properly serialized FlexiCounter data.This change improves test data realism by using a correctly serialized
FlexiCounterstructure instead of raw byte patterns, which better aligns with the undelegation test scenarios in this PR.test-integration/test-committor-service/tests/test_intent_executor.rs (4)
39-39: LGTM: Import needed for undelegation failure testing.The
FAIL_UNDELEGATION_LABELimport is used in the new undelegation error tests to trigger controlled failures.
161-208: LGTM: Undelegation error parsing test is well-structured.The test correctly:
- Forces an undelegation failure using
FAIL_UNDELEGATION_LABEL- Verifies the error is properly typed as
UndelegationError- Validates the error message content
This follows the established pattern of other error parsing tests in the file.
831-842: LGTM: setup_counter signature correctly extended to support labeled counters.The optional
labelparameter enables tests to create counters that trigger specific behavior (like forced undelegation failures), which aligns with the PR's testing objectives.
121-121: LGTM: Call sites consistently updated for new signature.All invocations of
setup_counterandinit_and_delegate_account_on_chainhave been updated to passNonefor the new optionallabelparameter, maintaining existing test behavior while enabling new undelegation failure tests.Also applies to: 222-222, 283-283, 340-341, 436-436, 488-488, 547-547, 635-635
| { | ||
| let data = delegated_account.data.borrow(); | ||
| if let Ok(counter) = FlexiCounter::deserialize(&mut data.as_ref()) { | ||
| if counter.label == FAIL_UNDELEGATION_LABEL { | ||
| return Err(ProgramError::Custom(FAIL_UNDELEGATION_CODE)); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using try_from_slice for consistency.
The code uses FlexiCounter::deserialize(&mut data.as_ref())? but the rest of this file consistently uses FlexiCounter::try_from_slice() (see lines 207, 237, 265, 368). While both work, maintaining consistency improves readability.
Apply this diff:
- {
- let data = delegated_account.data.borrow();
- if let Ok(counter) = FlexiCounter::deserialize(&mut data.as_ref()) {
- if counter.label == FAIL_UNDELEGATION_LABEL {
- return Err(ProgramError::Custom(FAIL_UNDELEGATION_CODE));
- }
- }
- };
+ let counter =
+ FlexiCounter::try_from_slice(&delegated_account.data.borrow())?;
+ if counter.label == FAIL_UNDELEGATION_LABEL {
+ return Err(ProgramError::Custom(FAIL_UNDELEGATION_CODE));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| let data = delegated_account.data.borrow(); | |
| if let Ok(counter) = FlexiCounter::deserialize(&mut data.as_ref()) { | |
| if counter.label == FAIL_UNDELEGATION_LABEL { | |
| return Err(ProgramError::Custom(FAIL_UNDELEGATION_CODE)); | |
| } | |
| } | |
| }; | |
| let counter = | |
| FlexiCounter::try_from_slice(&delegated_account.data.borrow())?; | |
| if counter.label == FAIL_UNDELEGATION_LABEL { | |
| return Err(ProgramError::Custom(FAIL_UNDELEGATION_CODE)); | |
| } |
🤖 Prompt for AI Agents
In test-integration/programs/flexi-counter/src/processor.rs around lines 399 to
406, replace the use of FlexiCounter::deserialize(&mut data.as_ref()) with the
crate-consistent FlexiCounter::try_from_slice(...) call so it matches other
usages in the file; call try_from_slice on the account data slice (e.g.,
data.as_ref() or &data[..]) and keep the same Ok/Err handling (if let
Ok(counter) = ... ) so the behavior remains unchanged but follows the
established pattern.
#636
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.