refactor: move commit-and-undelegate scheduling service from magicblock-accounts to magicblock-services#1402
refactor: move commit-and-undelegate scheduling service from magicblock-accounts to magicblock-services#1402snawaz wants to merge 1 commit into
Conversation
…ck-accounts to magicblock-services
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughWalkthroughThis PR removes the Sequence Diagram(s)sequenceDiagram
participant MagicValidator
participant UndelegationRequestService
participant Chainlink
participant TransactionScheduler
MagicValidator->>UndelegationRequestService: new(chainlink, scheduler, poll_interval, ...)
MagicValidator->>UndelegationRequestService: start()
UndelegationRequestService->>Chainlink: subscribe_undelegation_requests()
alt subscription succeeds
UndelegationRequestService->>UndelegationRequestService: spawn subscription_processor
UndelegationRequestService->>UndelegationRequestService: spawn poll_processor (if poll_interval > 0)
else subscription fails
UndelegationRequestService->>UndelegationRequestService: spawn poll_processor only
end
UndelegationRequestService->>Chainlink: observe DLP UndelegationRequest accounts
UndelegationRequestService->>TransactionScheduler: schedule ScheduleCommitAndUndelegate
MagicValidator->>UndelegationRequestService: stop()
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-services/src/undelegation_request_service.rs (1)
31-40: 🎯 Functional Correctness | 🔵 TrivialAll
Scheduleerrors treated as retryable regardless of underlyingTransactionErrorkind.
retryable()unconditionally returnstrueforSchedule(TransactionError). This means permanent failures (e.g. malformed instruction, insufficient funds, invalid program) get retried up toUNDELEGATION_REQUEST_MAX_ATTEMPTSjust like transient ones, wasting retry budget and delaying the terminal error log. Consider inspecting theTransactionErrorvariant to distinguish retryable (e.g. blockhash-not-found, node-busy) from permanent errors.🤖 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-services/src/undelegation_request_service.rs` around lines 31 - 40, The retryable() logic in ObservedUndelegationRequestError currently treats every Schedule(TransactionError) as retryable, which causes permanent transaction failures to be retried unnecessarily. Update retryable() to inspect the underlying TransactionError variant and return true only for genuinely transient cases (for example, blockhash-related or node-busy errors) while returning false for permanent failures. Use ObservedUndelegationRequestError and its retryable() method to locate the change.
🤖 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-services/src/undelegation_request_service.rs`:
- Around line 364-374: The fallback path in start() can become completely inert
when Chainlink subscription fails and poll_interval is zero, because the current
warn! is skipped and spawn_poll_processor() only emits debug output. Update the
start() logic in UndelegationRequestService to detect the specific combination
of subscribe_undelegation_requests() returning None and polling being disabled,
and emit a warn! or error! there before returning or attempting a no-op fallback
so operators can see that undelegation processing is effectively disabled.
---
Outside diff comments:
In `@magicblock-services/src/undelegation_request_service.rs`:
- Around line 31-40: The retryable() logic in ObservedUndelegationRequestError
currently treats every Schedule(TransactionError) as retryable, which causes
permanent transaction failures to be retried unnecessarily. Update retryable()
to inspect the underlying TransactionError variant and return true only for
genuinely transient cases (for example, blockhash-related or node-busy errors)
while returning false for permanent failures. Use
ObservedUndelegationRequestError and its retryable() method to locate the
change.
🪄 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: afa8b050-8fff-49b3-9e7f-31f5184b4eab
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.agents/context/architecture.md.agents/context/crate-map.md.agents/context/crates/magicblock-account-cloner.md.agents/context/crates/magicblock-accounts.md.agents/context/crates/magicblock-api.md.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-committor-program.md.agents/context/crates/magicblock-committor-service.md.agents/context/crates/magicblock-config.md.agents/context/crates/magicblock-core.md.agents/context/crates/magicblock-magic-program-api.md.agents/context/crates/magicblock-metrics.md.agents/context/crates/magicblock-services.md.agents/specs/validator-specification.mdCargo.tomldocs/architecture.mdmagicblock-accounts/Cargo.tomlmagicblock-accounts/README.mdmagicblock-accounts/src/config.rsmagicblock-accounts/src/errors.rsmagicblock-accounts/src/lib.rsmagicblock-accounts/src/traits.rsmagicblock-api/Cargo.tomlmagicblock-api/src/errors.rsmagicblock-api/src/magic_validator.rsmagicblock-committor-service/README.mdmagicblock-services/Cargo.tomlmagicblock-services/src/lib.rsmagicblock-services/src/undelegation_request_service.rstest-manual/Makefiletest-manual/helius-laser/sh/04_start-validator.sh
💤 Files with no reviewable changes (9)
- magicblock-accounts/README.md
- magicblock-accounts/Cargo.toml
- magicblock-accounts/src/traits.rs
- Cargo.toml
- magicblock-accounts/src/lib.rs
- magicblock-accounts/src/config.rs
- magicblock-accounts/src/errors.rs
- magicblock-api/Cargo.toml
- magicblock-api/src/errors.rs
| pub fn start(self: &Arc<Self>) { | ||
| let Some(requests) = self.chainlink.subscribe_undelegation_requests() | ||
| else { | ||
| if !self.undelegation_request_poll_interval.is_zero() { | ||
| if !self.poll_interval.is_zero() { | ||
| warn!( | ||
| "Cannot subscribe to DLP undelegation requests; falling back to polling only" | ||
| ); | ||
| } | ||
| self.spawn_undelegation_request_poll_processor(); | ||
| self.spawn_poll_processor(); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Silent full disablement when subscription fails and polling is disabled by config.
If subscribe_undelegation_requests() returns None and poll_interval is zero (explicitly disabled), the warn! is skipped (only fires if !self.poll_interval.is_zero()), and spawn_poll_processor only logs at debug!. The net effect: undelegation requests are never processed, with no operator-visible signal above debug level.
Consider logging a warn!/error! in this specific combination (subscribe failed AND polling disabled) since the service is effectively fully inert.
🔧 Proposed fix
pub fn start(self: &Arc<Self>) {
let Some(requests) = self.chainlink.subscribe_undelegation_requests()
else {
- if !self.poll_interval.is_zero() {
- warn!(
- "Cannot subscribe to DLP undelegation requests; falling back to polling only"
- );
- }
+ if self.poll_interval.is_zero() {
+ error!(
+ "Cannot subscribe to DLP undelegation requests and polling is disabled; undelegation requests will not be processed"
+ );
+ } else {
+ warn!(
+ "Cannot subscribe to DLP undelegation requests; falling back to polling only"
+ );
+ }
self.spawn_poll_processor();
return;
};📝 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.
| pub fn start(self: &Arc<Self>) { | |
| let Some(requests) = self.chainlink.subscribe_undelegation_requests() | |
| else { | |
| if !self.undelegation_request_poll_interval.is_zero() { | |
| if !self.poll_interval.is_zero() { | |
| warn!( | |
| "Cannot subscribe to DLP undelegation requests; falling back to polling only" | |
| ); | |
| } | |
| self.spawn_undelegation_request_poll_processor(); | |
| self.spawn_poll_processor(); | |
| return; | |
| }; | |
| pub fn start(self: &Arc<Self>) { | |
| let Some(requests) = self.chainlink.subscribe_undelegation_requests() | |
| else { | |
| if self.poll_interval.is_zero() { | |
| error!( | |
| "Cannot subscribe to DLP undelegation requests and polling is disabled; undelegation requests will not be processed" | |
| ); | |
| } else { | |
| warn!( | |
| "Cannot subscribe to DLP undelegation requests; falling back to polling only" | |
| ); | |
| } | |
| self.spawn_poll_processor(); | |
| return; | |
| }; |
🤖 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-services/src/undelegation_request_service.rs` around lines 364 -
374, The fallback path in start() can become completely inert when Chainlink
subscription fails and poll_interval is zero, because the current warn! is
skipped and spawn_poll_processor() only emits debug output. Update the start()
logic in UndelegationRequestService to detect the specific combination of
subscribe_undelegation_requests() returning None and polling being disabled, and
emit a warn! or error! there before returning or attempting a no-op fallback so
operators can see that undelegation processing is effectively disabled.
|
discarded, as it should be done differently. |
1 participant
Summary
Breaking Changes
Test Plan
Summary by CodeRabbit
New Features
Bug Fixes