feat: Schedule undelegation from observed DLP requests#1357
Open
snawaz wants to merge 7 commits into
Open
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates the undelegation flow end to end. It adds a broadcasted undelegation observation event in Chainlink, threads subscription wiring through FetchCloner and InnerChainlink, and adds a scheduled-commits processor loop that consumes those observations. It also extends task fetching and undelegate task construction to carry request rent payer metadata, adds a schedulecommit CPI for requesting undelegation, and updates integration tests to exercise the new on-chain flow. Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/mod.rs (1)
627-635: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd serialization coverage for the
request_rent_payer: Some(..)path.Line 632 currently validates only the legacy
Nonebranch. Please add one test case that constructsUndelegateTask { request_rent_payer: Some(...) }and assertsinstruction(&validator)remains serializable.Proposed test extension
let undelegate_task: BaseTaskImpl = UndelegateTask { delegated_account: Pubkey::new_unique(), owner_program: Pubkey::new_unique(), rent_reimbursement: Pubkey::new_unique(), request_rent_payer: None, } .into(); assert_serializable(&undelegate_task.instruction(&validator)); + + let undelegate_with_request_task: BaseTaskImpl = UndelegateTask { + delegated_account: Pubkey::new_unique(), + owner_program: Pubkey::new_unique(), + rent_reimbursement: Pubkey::new_unique(), + request_rent_payer: Some(Pubkey::new_unique()), + } + .into(); + assert_serializable( + &undelegate_with_request_task.instruction(&validator), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-committor-service/src/tasks/mod.rs` around lines 627 - 635, The current test for UndelegateTask only covers the request_rent_payer: None case, leaving the Some variant untested. Add a second test case that constructs a UndelegateTask with request_rent_payer: Some(Pubkey::new_unique()) and calls assert_serializable on the resulting instruction to ensure the Some path is also covered and remains serializable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 117: The magicblock-delegation-program-api dependency in Cargo.toml uses
a relative path dependency pointing to ../delegation-program/dlp-api which will
not be available in CI or clean checkout environments. Replace this path
dependency with one of the following approaches: move the delegation program API
code into this repository and add it as a workspace member in Cargo.toml,
configure a git submodule for the delegation-program directory, or replace the
path dependency with a published crate version from crates.io if available.
Update the dependency declaration accordingly based on which approach is chosen.
In `@magicblock-accounts/src/scheduled_commits_processor.rs`:
- Around line 287-288: Replace the `.expect(POISONED_MUTEX_MSG)` calls on the
mutex locks (in observed_requests.lock(), manifest_ready_to_run.lock(),
manifest_finished.lock(), and other mutex lock calls throughout the
scheduled_commits_processor function) with proper error handling instead of
panicking. Reference the existing error recovery and logging pattern used in the
remove_intent_metas function and apply the same approach to gracefully handle
poisoned mutexes by logging the error and returning a result type, rather than
calling expect which terminates the processor on a poisoned mutex.
- Around line 119-128: The `undelegation_request_processor` task is being
spawned during `new()` initialization before the ledger replay and bank reset
that occur in `start()`. Move the tokio::spawn call for
`Self::undelegation_request_processor` from the `new()` method to the `start()`
method, placing it after the replay, bank reset, and scheduler mode transition
have completed. Gate the spawn behind the same post-replay phase condition used
for pending-intent recovery to ensure observed undelegation requests are not
scheduled against pre-replay state or cleared during startup replay handling.
- Around line 223-229: The code currently drops undelegation requests without
recovery when encountering transient failures across multiple paths: broadcast
lag errors (Err(broadcast::error::RecvError::Lagged) handler), chainlink
verification failures, and scheduler failures. Each path logs the issue, removes
the request from dedupe state with no retry mechanism. Additionally, the
`.expect(POISONED_MUTEX_MSG)` calls violate production error handling standards
by panicking instead of propagating or contextualizing errors. Replace all
`.expect(POISONED_MUTEX_MSG)` unwraps with proper error propagation using the ?
operator or explicit error handling, then implement either a bounded retry
mechanism with exponential backoff for transient failures or add a periodic
reconciliation scan that replays requests removed from dedupe state to ensure
transient failures do not permanently lose undelegation requests.
- Around line 53-58: The ObservedUndelegationRequestIdentity struct stores
delegation_nonce_at_request and expires_at_slot but these values are never
validated before constructing the UndelegateTask in the task builder. Add
validation logic in the task builder (around where UndelegateTask is constructed
with undelegate_with_request) to ensure the current request's delegation nonce
matches delegation_nonce_at_request and that the current slot has not exceeded
expires_at_slot. This validation must occur before the task is scheduled to
prevent stale or expired requests from being processed without verifying the
request state still matches what was originally observed.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 1704-1706: The issue is that the `send()` call at lines 1704–1706
in the `undelegation_request_sender.send()` discards the Result without handling
SendError, causing silently lost observations when no subscribers are active due
to a race condition between channel creation and subscriber installation. Fix
this by ensuring the broadcast channel receiver is created and subscribed to
before the `start_subscription_listener` is spawned at line 290, or by handling
the SendError explicitly instead of discarding it with `let _ =`, and consider
using a persistent queue (such as a VecDeque or persistent channel) to replay
observations for late subscribers rather than relying on the broadcast channel's
limited buffer.
In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs`:
- Around line 308-323: The get_multiple_accounts_with_config RPC call lacks
retry handling while other fetch paths in this file use NUM_FETCH_RETRIES for
transient error recovery. Wrap the get_multiple_accounts_with_config call with a
retry loop that retries up to NUM_FETCH_RETRIES times before ultimately mapping
the error using TaskInfoFetcherError::map_client_error. This ensures that
transient RPC failures don't abort finalize-task building for the entire bundle.
---
Outside diff comments:
In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 627-635: The current test for UndelegateTask only covers the
request_rent_payer: None case, leaving the Some variant untested. Add a second
test case that constructs a UndelegateTask with request_rent_payer:
Some(Pubkey::new_unique()) and calls assert_serializable on the resulting
instruction to ensure the Some path is also covered and remains serializable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1553e92d-3eff-4304-9d98-6ca9640be135
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlmagicblock-accounts/Cargo.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/test-committor-service/tests/test_transaction_preparator.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
531-537: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the mock honor the requested pubkeys.
Returning the full map lets tests pass even if
finalize_taskscallsfetch_undelegation_requestswith the wrong or empty account list. Filter bypubkeysto match theTaskInfoFetchercontract.Proposed mock fix
async fn fetch_undelegation_requests( &self, - _: &[Pubkey], + pubkeys: &[Pubkey], _: u64, ) -> TaskInfoFetcherResult<HashMap<Pubkey, UndelegationRequest>> { - Ok(self.undelegation_requests.clone()) + Ok(pubkeys + .iter() + .filter_map(|pubkey| { + self.undelegation_requests + .get(pubkey) + .cloned() + .map(|request| (*pubkey, request)) + }) + .collect()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-committor-service/src/tasks/task_strategist.rs` around lines 531 - 537, The fetch_undelegation_requests mock method ignores the pubkeys parameter (currently prefixed with underscore) and returns the entire undelegation_requests map, which allows tests to pass even with incorrect account lists. To match the TaskInfoFetcher contract, update the method to filter self.undelegation_requests by the requested pubkeys: remove the underscore prefix from the pubkeys parameter, then iterate through self.undelegation_requests and retain only the entries whose keys exist in the provided pubkeys slice before returning the filtered HashMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@magicblock-committor-service/src/tasks/task_strategist.rs`:
- Around line 531-537: The fetch_undelegation_requests mock method ignores the
pubkeys parameter (currently prefixed with underscore) and returns the entire
undelegation_requests map, which allows tests to pass even with incorrect
account lists. To match the TaskInfoFetcher contract, update the method to
filter self.undelegation_requests by the requested pubkeys: remove the
underscore prefix from the pubkeys parameter, then iterate through
self.undelegation_requests and retain only the entries whose keys exist in the
provided pubkeys slice before returning the filtered HashMap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9b399d8-0f9d-4c1d-8abf-69d1b2a2352d
📒 Files selected for processing (2)
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-integration/Cargo.toml`:
- Line 58: The `test-integration` manifest still uses an out-of-tree path
dependency for `magicblock-delegation-program-api`, which breaks fresh checkouts
just like the workspace dependency. Update the dependency declaration to use a
reproducible source instead of the relative path, such as a git revision or
published crate, or vendor the package so `cargo check` works without a sibling
checkout. Use the `magicblock-delegation-program-api` entry in
`test-integration/Cargo.toml` as the target to fix.
In `@test-integration/configs/schedulecommit-conf.devnet.toml`:
- Line 32: The ScheduleCommit devnet config is pointing to the wrong dlp.so
artifact path, which CI does not stage. Update the config entry used by
schedulecommit-conf.devnet.toml to reference the validator-staged dlp.so
produced by the test harness, or add the missing setup step that copies the
artifact into the current delegation-program path before startup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a19c6b2-99f6-43be-b7ac-225072fd2d3e
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
test-integration/Cargo.tomltest-integration/configs/schedulecommit-conf.devnet.tomltest-integration/programs/schedulecommit/src/api.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-committor-service/src/intent_executor/task_info_fetcher.rs`:
- Around line 58-65: The default implementation of
TaskInfoFetcher::fetch_undelegation_requests silently returns an empty map,
which can hide missing support and cause undelegation requests to be skipped.
Remove the default body from fetch_undelegation_requests in the TaskInfoFetcher
trait so implementers must provide an explicit implementation, and verify any
concrete fetcher types are updated accordingly to handle undelegation requests
consistently with the other required TaskInfoFetcher methods and the
task_builder.rs instruction selection flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb2cd7dc-d276-4eb3-8642-d559e2408b2b
⛔ Files ignored due to path filters (1)
test-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (1)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs
snawaz
force-pushed
the
snawaz/req-undel
branch
3 times, most recently
from
e1932fa to
a5fd718
Compare
snawaz
force-pushed
the
snawaz/req-undel
branch
5 times, most recently
from
95dcbe1 to
134332d
Compare
2 tasks
taco-paco
requested changes
Jul 6, 2026
Contributor
There was a problem hiding this comment.
This is a service that processes undelegation requests, it doesn't belong in magicblock-accounts crate. Let's move whether into magicblock-services or into magicblock-sommittor-service as a second service.
Contributor
There was a problem hiding this comment.
This crate seems so obsolete that its worth considering removing it all together I don't think we should revive it
Contributor
Author
There was a problem hiding this comment.
Moved
magicblock-accounts/src/scheduled_commits_processor.rs
to
magicblock-services/src/undelegation_request_service.rs
| ) -> TaskInfoFetcherResult<HashMap<Pubkey, DelegationMetadata>>; | ||
|
|
||
| /// Fetches request-undelegation accounts keyed by delegated account pubkey. | ||
| async fn fetch_undelegation_requests( |
Contributor
There was a problem hiding this comment.
Where is this used? This seems to be unused method as i can see, if so let's remove it.
Contributor
Author
There was a problem hiding this comment.
fetch_undelegation_requests is removed now.
| ) -> TaskInfoFetcherResult<Vec<Pubkey>>; | ||
|
|
||
| /// Fetches delegation metadata keyed by delegated account pubkey. | ||
| async fn fetch_delegation_metadata( |
Contributor
There was a problem hiding this comment.
From all the callsites this method is used to get rent_payer/rent_reimbursement address. We do have fetch_rent_reimbursements in TaskInfoFetcher. It seems like we don't need a new method for this, or did I miss something?
Contributor
Author
There was a problem hiding this comment.
fetch_rent_reimbursements is now replaced with fetch_delegation_metadata, as we need more fields from the metadata PDA.
snawaz
force-pushed
the
snawaz/req-undel
branch
2 times, most recently
from
c55461d to
9eee800
Compare
2 tasks
…ock-services/src/undelegation_request_service.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
request-undelegation flow:
observes request PDAs->ScheduleCommitAndUndelegate->request-aware undelegation to close the request PDA, in the committor service.schedule-commit-flow: now the regular path also sends the additional accounts to the finalize instruction, so that the DLP can detect "undelegation-request" and if so, finalize itself can undelegate the account.with retries and proper error loggings/handlings at each stage.