feat: Impl UndelegationRequest backfill loop#1378
Conversation
|
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. |
|
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:
📝 WalkthroughWalkthroughThis PR adds a configurable polling backfill path for discovering owner-program 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
600-621: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake polling a real fallback, or remove the dead branch
spawn_undelegation_request_poll_processor()currently runs unconditionally whenever polling is enabled, so validators keep scanning the full DLP program even when subscriptions are healthy.- The warning also disappears if polling is disabled, so a subscription failure can go unlogged.
🤖 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-accounts/src/scheduled_commits_processor.rs` around lines 600 - 621, The polling path in spawn_undelegation_request_processor is not a true fallback because spawn_undelegation_request_poll_processor runs even after a successful subscribe_undelegation_requests call. Update the logic so polling is only started when the subscription is unavailable or fails, and keep the warning behavior tied to that fallback path. If polling is intended to always run, remove the dead fallback branch and its warning from scheduled_commits_processor::spawn_undelegation_request_processor.
🤖 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-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 338-393: The new fallback scan in fetch_undelegation_requests
lacks tracing, unlike fetch_remote_accounts and fetch_accounts, so add
#[instrument] to this method to capture span context for the RPC-heavy
get_program_accounts_with_config call. Make sure the instrumentation includes
useful fields for debugging the scan, such as account count/result size or
request context, and preserves failure visibility through the existing ? error
paths.
In `@magicblock-chainlink/src/remote_account_provider/chain_rpc_client.rs`:
- Around line 107-140: The `get_program_accounts_with_config` scan currently
uses `collect()` over `Result`, so a single bad account decode or owner parse
aborts the whole batch. Update the mapping in `chain_rpc_client.rs` to handle
per-account failures locally inside the iterator, skip malformed entries, and
keep returning the successfully decoded `(Pubkey, Account)` pairs. Use the
existing `client_error::ErrorKind::Custom` paths around `account.data.decode()`
and `Pubkey::from_str(...)` to log or record the failure, but do not let one bad
record turn the entire RPC response into an error.
In `@magicblock-chainlink/src/remote_account_provider/mod.rs`:
- Around line 911-936: The program-account scan in
get_program_accounts_with_config is using the same short RPC_FETCH_TIMEOUT as
small fetches, which can cause repeated timeouts under larger
UndelegationRequest account scans. Update this path to use a separate, longer
timeout (preferably configurable) or add a small retry with backoff before
returning RemoteAccountProviderError::AccountResolutionsFailed, while keeping
the existing commitment and error handling flow intact.
---
Outside diff comments:
In `@magicblock-accounts/src/scheduled_commits_processor.rs`:
- Around line 600-621: The polling path in spawn_undelegation_request_processor
is not a true fallback because spawn_undelegation_request_poll_processor runs
even after a successful subscribe_undelegation_requests call. Update the logic
so polling is only started when the subscription is unavailable or fails, and
keep the warning behavior tied to that fallback path. If polling is intended to
always run, remove the dead fallback branch and its warning from
scheduled_commits_processor::spawn_undelegation_request_processor.
🪄 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: 6e40b066-afa7-4928-a1d2-d02db5309be2
📒 Files selected for processing (17)
.agents/context/crates/magicblock-accounts.md.agents/context/crates/magicblock-api.md.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-config.md.agents/specs/validator-specification.mdconfig.example.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_rpc_client.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/testing/rpc_client_mock.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rs
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
2095658 to
df12e87
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-chainlink/src/chainlink/mod.rs (1)
234-272: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
try_newshould reuse theFetchClonersender when one is provided.
InnerChainlink::try_newalways creates a fresh broadcast channel, whileFetchCloner::new()creates a separate one. Any caller that builds a cloner with the plain constructor and then passes it intoInnerChainlink::try_newends up with two disconnected channels, sosubscribe_undelegation_requests()won’t receive the cloner’s live events.try_new_from_endpointsalready wires a shared sender correctly; this helper should do the same or expose the sender fromFetchCloner.🤖 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-chainlink/src/chainlink/mod.rs` around lines 234 - 272, InnerChainlink::try_new is creating a new broadcast channel instead of reusing the one already owned by FetchCloner, which disconnects undelegation events. Update try_new (or try_new_with_undelegation_request_sender) to take the sender from the provided FetchCloner when present, matching the wiring used by try_new_from_endpoints. If needed, expose the sender from FetchCloner and ensure subscribe_undelegation_requests sees the same live channel.
🤖 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 @.agents/context/crates/magicblock-accounts.md:
- Around line 234-238: This change updates `.agents/context`, so `AGENTS.md`
must be kept in sync to keep the entrypoint accurate. Mirror the new
owner-program undelegation request details from the updated context into
`AGENTS.md`, ensuring the
`ScheduledCommitsProcessorImpl::spawn_undelegation_request_processor` behavior,
polling/backfill settings, dedupe pruning behavior, and
`DelegationMetadata`/`ScheduleCommitAndUndelegate` flow are reflected
consistently.
In `@magicblock-accounts/src/scheduled_commits_processor.rs`:
- Around line 217-238: The pruning logic in
scheduled_commits_processor::process_observed_undelegation_request_with_retries/retain_observed_requests
is using a stale live_request_pdas snapshot while the subscription worker can
add newer requests concurrently. Update the scan/prune flow to record a
per-request generation or last-seen marker during the poll scan, and have
retain_observed_requests skip deleting entries that were inserted or refreshed
after that scan. Make sure the concurrency between the subscription and poll
processors in the main scheduler loop is preserved, but pruning only removes
requests that are truly older than the current scan.
- Around line 71-74: The retry policy in
ObservedUndelegationRequestError::retryable is too broad because it marks
Schedule(TransactionError) as retryable alongside transient failures. Update
this method to only return true for the transient error variants you actually
want to back off on, and exclude deterministic schedule/transaction errors so
scheduled commit retries stop consuming all attempts on permanent failures.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 339-393: The `ObservedUndelegationRequest` construction is
duplicated between `fetch_undelegation_requests` and
`resolve_account_to_clone_from_forwarded_sub_with_unsubscribe`, so the mapping
can drift over time. Add a shared constructor or `From`-style helper on
`ObservedUndelegationRequest` that takes the `request_pda`,
`UndelegationRequest`, and `observed_slot`, then use it in both call sites. Keep
the field mapping centralized so any future `UndelegationRequest` changes only
need one update.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/mod.rs`:
- Around line 234-272: InnerChainlink::try_new is creating a new broadcast
channel instead of reusing the one already owned by FetchCloner, which
disconnects undelegation events. Update try_new (or
try_new_with_undelegation_request_sender) to take the sender from the provided
FetchCloner when present, matching the wiring used by try_new_from_endpoints. If
needed, expose the sender from FetchCloner and ensure
subscribe_undelegation_requests sees the same live channel.
🪄 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: dfc2591e-3b7d-4841-bc1a-cc6ce53c77d0
📒 Files selected for processing (17)
.agents/context/crates/magicblock-accounts.md.agents/context/crates/magicblock-api.md.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-config.md.agents/specs/validator-specification.mdconfig.example.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_rpc_client.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/testing/rpc_client_mock.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-chainlink/src/chainlink/mod.rs (1)
234-272: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
try_newshould reuse theFetchClonersender when one is provided.
InnerChainlink::try_newalways creates a fresh broadcast channel, whileFetchCloner::new()creates a separate one. Any caller that builds a cloner with the plain constructor and then passes it intoInnerChainlink::try_newends up with two disconnected channels, sosubscribe_undelegation_requests()won’t receive the cloner’s live events.try_new_from_endpointsalready wires a shared sender correctly; this helper should do the same or expose the sender fromFetchCloner.🤖 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-chainlink/src/chainlink/mod.rs` around lines 234 - 272, InnerChainlink::try_new is creating a new broadcast channel instead of reusing the one already owned by FetchCloner, which disconnects undelegation events. Update try_new (or try_new_with_undelegation_request_sender) to take the sender from the provided FetchCloner when present, matching the wiring used by try_new_from_endpoints. If needed, expose the sender from FetchCloner and ensure subscribe_undelegation_requests sees the same live channel.
🤖 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 @.agents/context/crates/magicblock-accounts.md:
- Around line 234-238: This change updates `.agents/context`, so `AGENTS.md`
must be kept in sync to keep the entrypoint accurate. Mirror the new
owner-program undelegation request details from the updated context into
`AGENTS.md`, ensuring the
`ScheduledCommitsProcessorImpl::spawn_undelegation_request_processor` behavior,
polling/backfill settings, dedupe pruning behavior, and
`DelegationMetadata`/`ScheduleCommitAndUndelegate` flow are reflected
consistently.
In `@magicblock-accounts/src/scheduled_commits_processor.rs`:
- Around line 217-238: The pruning logic in
scheduled_commits_processor::process_observed_undelegation_request_with_retries/retain_observed_requests
is using a stale live_request_pdas snapshot while the subscription worker can
add newer requests concurrently. Update the scan/prune flow to record a
per-request generation or last-seen marker during the poll scan, and have
retain_observed_requests skip deleting entries that were inserted or refreshed
after that scan. Make sure the concurrency between the subscription and poll
processors in the main scheduler loop is preserved, but pruning only removes
requests that are truly older than the current scan.
- Around line 71-74: The retry policy in
ObservedUndelegationRequestError::retryable is too broad because it marks
Schedule(TransactionError) as retryable alongside transient failures. Update
this method to only return true for the transient error variants you actually
want to back off on, and exclude deterministic schedule/transaction errors so
scheduled commit retries stop consuming all attempts on permanent failures.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 339-393: The `ObservedUndelegationRequest` construction is
duplicated between `fetch_undelegation_requests` and
`resolve_account_to_clone_from_forwarded_sub_with_unsubscribe`, so the mapping
can drift over time. Add a shared constructor or `From`-style helper on
`ObservedUndelegationRequest` that takes the `request_pda`,
`UndelegationRequest`, and `observed_slot`, then use it in both call sites. Keep
the field mapping centralized so any future `UndelegationRequest` changes only
need one update.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/mod.rs`:
- Around line 234-272: InnerChainlink::try_new is creating a new broadcast
channel instead of reusing the one already owned by FetchCloner, which
disconnects undelegation events. Update try_new (or
try_new_with_undelegation_request_sender) to take the sender from the provided
FetchCloner when present, matching the wiring used by try_new_from_endpoints. If
needed, expose the sender from FetchCloner and ensure
subscribe_undelegation_requests sees the same live channel.
🪄 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: dfc2591e-3b7d-4841-bc1a-cc6ce53c77d0
📒 Files selected for processing (17)
.agents/context/crates/magicblock-accounts.md.agents/context/crates/magicblock-api.md.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-config.md.agents/specs/validator-specification.mdconfig.example.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_rpc_client.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/testing/rpc_client_mock.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rs
🛑 Comments failed to post (4)
.agents/context/crates/magicblock-accounts.md (1)
234-238: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep
AGENTS.mdin sync.These
.agents/contextedits should be mirrored inAGENTS.mdso the entrypoint stays accurate. As per coding guidelines, "If anything is added to, removed from, renamed, or reorganized inside./.agents/, updateAGENTS.mdin the same change so the entrypoint remains accurate."🤖 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 @.agents/context/crates/magicblock-accounts.md around lines 234 - 238, This change updates `.agents/context`, so `AGENTS.md` must be kept in sync to keep the entrypoint accurate. Mirror the new owner-program undelegation request details from the updated context into `AGENTS.md`, ensuring the `ScheduledCommitsProcessorImpl::spawn_undelegation_request_processor` behavior, polling/backfill settings, dedupe pruning behavior, and `DelegationMetadata`/`ScheduleCommitAndUndelegate` flow are reflected consistently.Source: Coding guidelines
magicblock-accounts/src/scheduled_commits_processor.rs (2)
71-74: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Current retry policy ==" sed -n '65,80p' magicblock-accounts/src/scheduled_commits_processor.rs echo echo "== Locked Solana transaction-error dependency ==" rg -n --glob 'Cargo.toml' --glob 'Cargo.lock' 'solana-transaction-error|solana-transaction' . echo echo "== Existing retryable() usage in the repo ==" rg -n --type rust '\bretryable\s*\(' .Repository: magicblock-labs/magicblock-validator
Length of output: 19007
🌐 Web query:
solana-transaction-error TransactionError retryable method 3.1.0 source💡 Result:
In the context of the Solana blockchain, the
TransactionErrortype is defined in thesolana-transaction-errorcrate, which reached version 3.1.0 on February 18, 2026 [1][2][3]. This crate provides the standardized enumeration of errors that can occur during the various stages of transaction processing, such as scheduling, fee validation, account loading, and execution [4][5][6]. There is no specific "retryable" property or method directly associated with theTransactionErrorenum in the 3.1.0 release [5][2]. Instead, Solana developers determine whether a transaction should be retried based on the specific error code returned by the network and the state of the transaction's blockhash [7][8]. Key points regarding transaction retries in Solana: 1. Handling Retryability: Developers are responsible for implementing logic to evaluate whether an error is retryable [7][8]. For example, errors likeBlockhashNotFoundor expired transactions (often indicated by block height expiration) are generally considered retryable, provided the developer rebuilds the transaction with a fresh, valid blockhash [8]. 2. Non-Retryable Errors: Errors that indicate fundamental issues—such asInsufficientFundsForFeeorSignatureFailure—are typically not retryable because they relate to the validity of the transaction itself rather than temporary network conditions [8]. 3. RPC Retry Mechanism: ThesendTransactionJSON-RPC method accepts amaxRetriesparameter, which allows the RPC node to attempt rebroadcasting a transaction to the leader [7]. IfmaxRetriesis not specified, the node may continue to retry until the blockhash expires [7]. 4. Developer Responsibility: Official guidance strongly recommends that developers implement their own rebroadcasting and confirmation logic [7]. This includes monitoring thelastValidBlockHeightof the transaction's blockhash and handling scenarios where the transaction must be re-signed or rebuilt with a newer blockhash [7][8][9]. For implementation, you should consult theTransactionErrorenum variants to identify the cause of failure and map them to your application's retry logic [8][6]. References to "3.1.0" in your query likely refer to the version of thesolana-transaction-errorcrate, which tracks these error variants but does not manage the retry lifecycle itself [1][2].Citations:
- 1: https://docs.rs/crate/solana-transaction-error/3.1.0
- 2: https://crates.io/crates/solana-transaction-error/3.1.0
- 3: https://github.com/anza-xyz/solana-sdk/releases/tag/transaction-error@v3.1.0
- 4: https://github.com/solana-labs/solana/blob/c0c60386544ec9a9ec7119229f37386d9f070523/sdk/src/transaction/error.rs
- 5: https://docs.rs/solana-transaction/latest/solana_transaction/enum.TransactionError.html
- 6: https://solana.com/docs/core/transactions/transaction-pipeline
- 7: https://solana.com/developers/guides/advanced/retry
- 8: https://solana.com/docs/payments/production-readiness
- 9: https://github.com/solana-foundation/developer-content/blob/main/docs/advanced/confirmation.md
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== scheduled_commits_processor around retry usage ==" sed -n '240,285p' magicblock-accounts/src/scheduled_commits_processor.rs echo echo "== TransactionError references in this crate ==" rg -n "TransactionError" magicblock-accounts/src/scheduled_commits_processor.rs magicblock-accounts/src -g '!**/target/**'Repository: magicblock-labs/magicblock-validator
Length of output: 2751
Don't retry every scheduler failure.
Schedule(TransactionError)is treated as retryable here, so deterministic failures can consume all attempts; narrow retries to the transient cases you actually want to back off on.🤖 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-accounts/src/scheduled_commits_processor.rs` around lines 71 - 74, The retry policy in ObservedUndelegationRequestError::retryable is too broad because it marks Schedule(TransactionError) as retryable alongside transient failures. Update this method to only return true for the transient error variants you actually want to back off on, and exclude deterministic schedule/transaction errors so scheduled commit retries stop consuming all attempts on permanent failures.
217-238: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Pruning races with the live subscription worker.
Lines 490-499 run the subscription and poll processors concurrently, but Lines 235-238 prune against a stale
live_request_pdassnapshot from the poll scan. Any request inserted by the subscription path after that scan and before pruning runs gets deleted again, so the next update/scan can schedule the same undelegation twice. Track a per-request scan generation / last-seen marker and only prune entries that were already older than the current scan.Also applies to: 479-500
🤖 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-accounts/src/scheduled_commits_processor.rs` around lines 217 - 238, The pruning logic in scheduled_commits_processor::process_observed_undelegation_request_with_retries/retain_observed_requests is using a stale live_request_pdas snapshot while the subscription worker can add newer requests concurrently. Update the scan/prune flow to record a per-request generation or last-seen marker during the poll scan, and have retain_observed_requests skip deleting entries that were inserted or refreshed after that scan. Make sure the concurrency between the subscription and poll processors in the main scheduler loop is preserved, but pruning only removes requests that are truly older than the current scan.magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
339-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract a shared constructor for
ObservedUndelegationRequest.The same 8-field mapping from
UndelegationRequest(+ pda + observed_slot) toObservedUndelegationRequestis duplicated here and inresolve_account_to_clone_from_forwarded_sub_with_unsubscribe(poll path vs. subscription path). A sharedFrom/constructor would prevent the two call sites from drifting when fields are added/renamed.♻️ Suggested helper
+impl ObservedUndelegationRequest { + fn from_request( + request_pda: Pubkey, + request: &UndelegationRequest, + observed_slot: u64, + ) -> Self { + Self { + request_pda, + delegated_account: request.delegated_account, + owner_program: request.owner_program, + rent_payer: request.rent_payer, + created_slot: request.created_slot, + expires_at_slot: request.expires_at_slot, + last_commit_id_at_request: request.last_commit_id_at_request, + observed_slot, + } + } +}Then both call sites become
ObservedUndelegationRequest::from_request(request_pda, &request, observed_slot).Also applies to: 1901-1940
🤖 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-chainlink/src/chainlink/fetch_cloner/mod.rs` around lines 339 - 393, The `ObservedUndelegationRequest` construction is duplicated between `fetch_undelegation_requests` and `resolve_account_to_clone_from_forwarded_sub_with_unsubscribe`, so the mapping can drift over time. Add a shared constructor or `From`-style helper on `ObservedUndelegationRequest` that takes the `request_pda`, `UndelegationRequest`, and `observed_slot`, then use it in both call sites. Keep the field mapping centralized so any future `UndelegationRequest` changes only need one update.
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
df12e87 to
9483333
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-accounts/src/scheduled_commits_processor.rs`:
- Around line 178-245: The pruning in undelegation_request_poll_processor is too
aggressive and can delete freshly observed dedupe entries before the live
subscription catches up, causing duplicate re-processing. Update the pruning
flow around retain_observed_requests and the observed_requests map so an entry
is only removed after it has been missing across more than one consecutive poll
(for example with a miss counter), or make the getProgramAccounts scan use the
same commitment as undelegation_request_processor before pruning. Keep the fix
localized to undelegation_request_poll_processor and the shared
observed_undelegation_requests handling.
- Around line 479-489: The warning in spawn_undelegation_request_processor is
inverted: when subscribe_undelegation_requests() returns None, emit a warn! for
the no-fallback case where undelegation_request_poll_interval.is_zero() and
requests will never be observed, and keep the existing warning for the
polling-fallback case if desired. Update the conditional around the warn! in
scheduled_commits_processor::spawn_undelegation_request_processor so the
severity reflects the actual risk, rather than relying on the debug! in
spawn_undelegation_request_poll_processor.
In `@magicblock-chainlink/src/chainlink/mod.rs`:
- Around line 645-652: Add a #[instrument(skip(self))] attribute to
fetch_undelegation_requests in InnerChainlink so it matches the tracing behavior
of the sibling public async methods like ensure_accounts, fetch_accounts,
accounts_delegated_on_base_and_er, and undelegation_requested. Keep the method
logic unchanged; just apply the same instrumentation convention to preserve
visibility on the polling fallback path.
🪄 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: e17099fe-c944-436e-a604-b7ec8195d1c5
📒 Files selected for processing (17)
.agents/context/crates/magicblock-accounts.md.agents/context/crates/magicblock-api.md.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-config.md.agents/specs/validator-specification.mdconfig.example.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_rpc_client.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/testing/rpc_client_mock.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rs
snawaz
force-pushed
the
snawaz/backfill-req
branch
4 times, most recently
from
e9365f6 to
0e52050
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
magicblock-chainlink/src/chainlink/mod.rs (1)
645-652: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
#[instrument]for consistency with sibling async methods.Every other public async method on
InnerChainlink(ensure_accounts,fetch_accounts,accounts_delegated_on_base_and_er,undelegation_requested) carries#[instrument(skip(self, ...))]. This method still lacks it, reducing trace visibility for the polling fallback path.♻️ Suggested addition
+ #[instrument(skip(self))] pub async fn fetch_undelegation_requests( &self, ) -> ChainlinkResult<Vec<ObservedUndelegationRequest>> {🤖 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-chainlink/src/chainlink/mod.rs` around lines 645 - 652, The public async method fetch_undelegation_requests on InnerChainlink is missing the same tracing instrumentation used by its sibling async methods. Add a #[instrument(skip(self, ...))] attribute to fetch_undelegation_requests, matching the pattern already present on ensure_accounts, fetch_accounts, accounts_delegated_on_base_and_er, and undelegation_requested so the polling fallback path is visible in traces.magicblock-accounts/src/scheduled_commits_processor.rs (2)
178-245: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPoll-based pruning can still race with the live subscription and cause duplicate re-processing.
undelegation_request_processor(subscription) andundelegation_request_poll_processor(polling) run concurrently against the sameobserved_undelegation_requestsmap wheneverundelegation_request_poll_intervalis non-zero — the poll loop is spawned even after a successful subscription (seespawn_undelegation_request_processor, line 499). Each poll tick computeslive_request_pdasfrom a singlegetProgramAccountssnapshot and then unconditionally prunes any dedupe entry not present in it viaretain_observed_requests.A request PDA freshly observed/inserted via the live subscription can be pruned before the concurrent scan catches up (different commitment/timing), causing it to look "new" again on the next scan and resubmit
ScheduleCommitAndUndelegatefor a request already in flight.Consider only pruning entries missing across more than one consecutive scan, or aligning the scan's commitment with the subscription's commitment before removing dedupe state.
Also applies to: 455-462
🤖 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-accounts/src/scheduled_commits_processor.rs` around lines 178 - 245, The polling path in undelegation_request_poll_processor is pruning observed_undelegation_requests too aggressively, which can race with the live subscription and make in-flight requests look new again. Update the dedupe removal logic in undelegation_request_poll_processor and retain_observed_requests so entries are only removed after being absent for more than one consecutive scan, or otherwise ensure the polling commitment/timing matches the subscription before pruning. Keep the fix localized to the polling/pruning flow used alongside spawn_undelegation_request_processor and process_observed_undelegation_request_with_retries.
479-489: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWarning severity is still inverted for the "no fallback available" case.
When
subscribe_undelegation_requests()returnsNone,warn!only fires ifundelegation_request_poll_intervalis non-zero (i.e., when polling will compensate). When polling is also disabled, the strictly worse outcome (undelegation requests never observed) is not warned here — only adebug!("Skipping DLP undelegation request poll processor")fires elsewhere, easy to miss at default log levels.🩹 Suggested fix
- if !self.undelegation_request_poll_interval.is_zero() { - warn!( - "Cannot subscribe to DLP undelegation requests; falling back to polling only" - ); - } + if self.undelegation_request_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" + ); + }🤖 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-accounts/src/scheduled_commits_processor.rs` around lines 479 - 489, The warning in spawn_undelegation_request_processor is inverted: the no-subscription path should warn most strongly when undelegation_request_poll_interval is zero and there is no fallback, not only when polling is available. Update the early None branch from subscribe_undelegation_requests() so it emits warn! for the no-fallback case and keep the existing fallback-to-polling behavior, using the spawn_undelegation_request_processor and spawn_undelegation_request_poll_processor flow to guide the fix.
🤖 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-accounts/src/scheduled_commits_processor.rs`:
- Around line 527-563: Add tests around the critical paths in
ScheduledCommitsProcessorImpl beyond retain_observed_requests: cover
process_observed_undelegation_request deduplication, retry/backoff behavior,
mutex-poisoning handling, and the poll/subscription pruning race that affects
ScheduleCommitAndUndelegate submissions. Use the existing
ScheduledCommitsProcessorImpl methods and
observed_requests/ObservedUndelegationRequests setup as anchors, and assert the
expected request retention, retry decisions, and safe error handling under
failure conditions.
---
Duplicate comments:
In `@magicblock-accounts/src/scheduled_commits_processor.rs`:
- Around line 178-245: The polling path in undelegation_request_poll_processor
is pruning observed_undelegation_requests too aggressively, which can race with
the live subscription and make in-flight requests look new again. Update the
dedupe removal logic in undelegation_request_poll_processor and
retain_observed_requests so entries are only removed after being absent for more
than one consecutive scan, or otherwise ensure the polling commitment/timing
matches the subscription before pruning. Keep the fix localized to the
polling/pruning flow used alongside spawn_undelegation_request_processor and
process_observed_undelegation_request_with_retries.
- Around line 479-489: The warning in spawn_undelegation_request_processor is
inverted: the no-subscription path should warn most strongly when
undelegation_request_poll_interval is zero and there is no fallback, not only
when polling is available. Update the early None branch from
subscribe_undelegation_requests() so it emits warn! for the no-fallback case and
keep the existing fallback-to-polling behavior, using the
spawn_undelegation_request_processor and
spawn_undelegation_request_poll_processor flow to guide the fix.
In `@magicblock-chainlink/src/chainlink/mod.rs`:
- Around line 645-652: The public async method fetch_undelegation_requests on
InnerChainlink is missing the same tracing instrumentation used by its sibling
async methods. Add a #[instrument(skip(self, ...))] attribute to
fetch_undelegation_requests, matching the pattern already present on
ensure_accounts, fetch_accounts, accounts_delegated_on_base_and_er, and
undelegation_requested so the polling fallback path is visible in traces.
🪄 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: 4e2fa5f7-4d00-4b67-938e-458bbffd54e4
📒 Files selected for processing (17)
.agents/context/crates/magicblock-accounts.md.agents/context/crates/magicblock-api.md.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-config.md.agents/specs/validator-specification.mdconfig.example.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_rpc_client.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/testing/rpc_client_mock.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rs
snawaz
force-pushed
the
snawaz/backfill-req
branch
2 times, most recently
from
2281257 to
d0e8ac6
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
d0e8ac6 to
01af044
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
981-1005: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winProgram-account scan still shares the 5s single-fetch timeout with no retry.
RPC_FETCH_TIMEOUT(5s) is sized for small single/multi-account fetches. Scanning all DLP-ownedUndelegationRequestaccounts can return substantially more data and grow with the pending-request backlog, yet this path reuses the same fixed budget and has no retry (unlikefetch()). This risks the polling backfill fallback perpetually timing out exactly when subscriptions are unavailable and it's needed most. This was flagged previously (noted as addressed), but the current code still uses the sharedRPC_FETCH_TIMEOUTwith no adjustment.Consider a separate, longer (ideally configurable) timeout for program-account scans, or a short retry with backoff before surfacing the error.
🤖 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-chainlink/src/remote_account_provider/mod.rs` around lines 981 - 1005, The get_program_accounts_with_config path is still using the shared RPC_FETCH_TIMEOUT intended for small fetches, so program-account scans can time out under larger backfills. Update get_program_accounts_with_config in remote_account_provider::mod to use a separate longer timeout for program-account scans, ideally configurable, or add a small retry/backoff before returning AccountResolutionsFailed. Keep the existing fetch() behavior unchanged and make the timeout choice explicit in this method so the scan path does not inherit the single-fetch budget.magicblock-chainlink/src/remote_account_provider/chain_rpc_client.rs (1)
106-140: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSingle malformed account still aborts the entire program-account scan.
.collect()overResultshort-circuits on the first decode/owner-parse failure, discarding every other account already fetched in that response and turning the whole scan into anErr. For the undelegation-request polling backfill, one persistently malformed/undecodable account would fail every poll cycle instead of just skipping that record — inconsistent with the graceful per-record skip already done one layer up infetch_undelegation_requests. This was flagged in a previous review round (noted as addressed), but the current code still shows the unfixed.collect()-over-Resultpattern.🛡️ Proposed fix: skip malformed accounts instead of failing the whole scan
async fn get_program_accounts_with_config( &self, pubkey: &Pubkey, config: RpcProgramAccountsConfig, ) -> client_error::Result<Vec<(Pubkey, Account)>> { - self.rpc_client + let ui_accounts = self + .rpc_client .get_program_ui_accounts_with_config(pubkey, config) - .await? - .into_iter() - .map(|(pubkey, account)| { - let data = account.data.decode().ok_or_else(|| { - client_error::ErrorKind::Custom(format!( - "failed to decode program account {pubkey} data" - )) - })?; - let owner = - Pubkey::from_str(&account.owner).map_err(|err| { - client_error::ErrorKind::Custom(format!( - "failed to decode program account {pubkey} owner: {err}" - )) - })?; - Ok(( - pubkey, - Account { - lamports: account.lamports, - data, - owner, - executable: account.executable, - rent_epoch: account.rent_epoch, - }, - )) - }) - .collect() + .await?; + let mut accounts = Vec::with_capacity(ui_accounts.len()); + for (pubkey, account) in ui_accounts { + let Some(data) = account.data.decode() else { + warn!(pubkey = %pubkey, "failed to decode program account data; skipping"); + continue; + }; + let Ok(owner) = Pubkey::from_str(&account.owner) else { + warn!(pubkey = %pubkey, owner = %account.owner, "failed to parse program account owner; skipping"); + continue; + }; + accounts.push(( + pubkey, + Account { + lamports: account.lamports, + data, + owner, + executable: account.executable, + rent_epoch: account.rent_epoch, + }, + )); + } + Ok(accounts) }🤖 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-chainlink/src/remote_account_provider/chain_rpc_client.rs` around lines 106 - 140, The `get_program_accounts_with_config` scan still aborts on the first malformed account because the `map(...).collect()` over `Result` short-circuits on decode/owner parse errors. Update the iteration in `chain_rpc_client::get_program_accounts_with_config` to skip invalid entries instead of returning an error, so one bad account does not fail the entire response. Keep the existing decode/owner parsing logic for each item, but handle failures per-record and continue collecting the remaining valid `(Pubkey, Account)` pairs.
🤖 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-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 2249-2287: Add a direct test for the live-subscription
undelegation-request branch in fetch_cloner’s update-handling path, since only
the polling path is currently covered. Exercise the
`UndelegationRequest::try_from_bytes_with_discriminator` decode from a forwarded
account update, verify the `ObservedUndelegationRequest` is built correctly, and
assert it is sent through `undelegation_request_sender` when the
delegation-record companion lookup misses. Use the surrounding control flow in
`fetch_cloner`/`DelegationActions` and the `ObservedUndelegationRequest` fields
to locate and validate this branch.
In `@magicblock-chainlink/src/testing/rpc_client_mock.rs`:
- Around line 438-463: The `get_program_accounts_with_config` mock uses
`self.accounts.lock().unwrap()`, which is the only new unwrap/expect concern in
this test-only RPC mock. Either replace it with proper poisoning/error handling
that propagates through the existing `client_error::Result`, or add an explicit
invariant comment in the `rpc_client_mock` implementation near `self.accounts`
explaining that this in-memory `Mutex` is only used in the test double, is never
held across `await`, and intentionally follows the same established pattern used
elsewhere in this file.
---
Duplicate comments:
In `@magicblock-chainlink/src/remote_account_provider/chain_rpc_client.rs`:
- Around line 106-140: The `get_program_accounts_with_config` scan still aborts
on the first malformed account because the `map(...).collect()` over `Result`
short-circuits on decode/owner parse errors. Update the iteration in
`chain_rpc_client::get_program_accounts_with_config` to skip invalid entries
instead of returning an error, so one bad account does not fail the entire
response. Keep the existing decode/owner parsing logic for each item, but handle
failures per-record and continue collecting the remaining valid `(Pubkey,
Account)` pairs.
In `@magicblock-chainlink/src/remote_account_provider/mod.rs`:
- Around line 981-1005: The get_program_accounts_with_config path is still using
the shared RPC_FETCH_TIMEOUT intended for small fetches, so program-account
scans can time out under larger backfills. Update
get_program_accounts_with_config in remote_account_provider::mod to use a
separate longer timeout for program-account scans, ideally configurable, or add
a small retry/backoff before returning AccountResolutionsFailed. Keep the
existing fetch() behavior unchanged and make the timeout choice explicit in this
method so the scan path does not inherit the single-fetch budget.
🪄 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: 7628ee7f-2451-4e22-ad4a-3feea37564d4
📒 Files selected for processing (17)
.agents/context/crates/magicblock-accounts.md.agents/context/crates/magicblock-api.md.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-config.md.agents/specs/validator-specification.mdconfig.example.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_rpc_client.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/testing/rpc_client_mock.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (2)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
981-1005: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winProgram-account scan still shares the 5s single-fetch timeout with no retry.
RPC_FETCH_TIMEOUT(5s) is sized for small single/multi-account fetches. Scanning all DLP-ownedUndelegationRequestaccounts can return substantially more data and grow with the pending-request backlog, yet this path reuses the same fixed budget and has no retry (unlikefetch()). This risks the polling backfill fallback perpetually timing out exactly when subscriptions are unavailable and it's needed most. This was flagged previously (noted as addressed), but the current code still uses the sharedRPC_FETCH_TIMEOUTwith no adjustment.Consider a separate, longer (ideally configurable) timeout for program-account scans, or a short retry with backoff before surfacing the error.
🤖 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-chainlink/src/remote_account_provider/mod.rs` around lines 981 - 1005, The get_program_accounts_with_config path is still using the shared RPC_FETCH_TIMEOUT intended for small fetches, so program-account scans can time out under larger backfills. Update get_program_accounts_with_config in remote_account_provider::mod to use a separate longer timeout for program-account scans, ideally configurable, or add a small retry/backoff before returning AccountResolutionsFailed. Keep the existing fetch() behavior unchanged and make the timeout choice explicit in this method so the scan path does not inherit the single-fetch budget.magicblock-chainlink/src/remote_account_provider/chain_rpc_client.rs (1)
106-140: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSingle malformed account still aborts the entire program-account scan.
.collect()overResultshort-circuits on the first decode/owner-parse failure, discarding every other account already fetched in that response and turning the whole scan into anErr. For the undelegation-request polling backfill, one persistently malformed/undecodable account would fail every poll cycle instead of just skipping that record — inconsistent with the graceful per-record skip already done one layer up infetch_undelegation_requests. This was flagged in a previous review round (noted as addressed), but the current code still shows the unfixed.collect()-over-Resultpattern.🛡️ Proposed fix: skip malformed accounts instead of failing the whole scan
async fn get_program_accounts_with_config( &self, pubkey: &Pubkey, config: RpcProgramAccountsConfig, ) -> client_error::Result<Vec<(Pubkey, Account)>> { - self.rpc_client + let ui_accounts = self + .rpc_client .get_program_ui_accounts_with_config(pubkey, config) - .await? - .into_iter() - .map(|(pubkey, account)| { - let data = account.data.decode().ok_or_else(|| { - client_error::ErrorKind::Custom(format!( - "failed to decode program account {pubkey} data" - )) - })?; - let owner = - Pubkey::from_str(&account.owner).map_err(|err| { - client_error::ErrorKind::Custom(format!( - "failed to decode program account {pubkey} owner: {err}" - )) - })?; - Ok(( - pubkey, - Account { - lamports: account.lamports, - data, - owner, - executable: account.executable, - rent_epoch: account.rent_epoch, - }, - )) - }) - .collect() + .await?; + let mut accounts = Vec::with_capacity(ui_accounts.len()); + for (pubkey, account) in ui_accounts { + let Some(data) = account.data.decode() else { + warn!(pubkey = %pubkey, "failed to decode program account data; skipping"); + continue; + }; + let Ok(owner) = Pubkey::from_str(&account.owner) else { + warn!(pubkey = %pubkey, owner = %account.owner, "failed to parse program account owner; skipping"); + continue; + }; + accounts.push(( + pubkey, + Account { + lamports: account.lamports, + data, + owner, + executable: account.executable, + rent_epoch: account.rent_epoch, + }, + )); + } + Ok(accounts) }🤖 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-chainlink/src/remote_account_provider/chain_rpc_client.rs` around lines 106 - 140, The `get_program_accounts_with_config` scan still aborts on the first malformed account because the `map(...).collect()` over `Result` short-circuits on decode/owner parse errors. Update the iteration in `chain_rpc_client::get_program_accounts_with_config` to skip invalid entries instead of returning an error, so one bad account does not fail the entire response. Keep the existing decode/owner parsing logic for each item, but handle failures per-record and continue collecting the remaining valid `(Pubkey, Account)` pairs.
🤖 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-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 2249-2287: Add a direct test for the live-subscription
undelegation-request branch in fetch_cloner’s update-handling path, since only
the polling path is currently covered. Exercise the
`UndelegationRequest::try_from_bytes_with_discriminator` decode from a forwarded
account update, verify the `ObservedUndelegationRequest` is built correctly, and
assert it is sent through `undelegation_request_sender` when the
delegation-record companion lookup misses. Use the surrounding control flow in
`fetch_cloner`/`DelegationActions` and the `ObservedUndelegationRequest` fields
to locate and validate this branch.
In `@magicblock-chainlink/src/testing/rpc_client_mock.rs`:
- Around line 438-463: The `get_program_accounts_with_config` mock uses
`self.accounts.lock().unwrap()`, which is the only new unwrap/expect concern in
this test-only RPC mock. Either replace it with proper poisoning/error handling
that propagates through the existing `client_error::Result`, or add an explicit
invariant comment in the `rpc_client_mock` implementation near `self.accounts`
explaining that this in-memory `Mutex` is only used in the test double, is never
held across `await`, and intentionally follows the same established pattern used
elsewhere in this file.
---
Duplicate comments:
In `@magicblock-chainlink/src/remote_account_provider/chain_rpc_client.rs`:
- Around line 106-140: The `get_program_accounts_with_config` scan still aborts
on the first malformed account because the `map(...).collect()` over `Result`
short-circuits on decode/owner parse errors. Update the iteration in
`chain_rpc_client::get_program_accounts_with_config` to skip invalid entries
instead of returning an error, so one bad account does not fail the entire
response. Keep the existing decode/owner parsing logic for each item, but handle
failures per-record and continue collecting the remaining valid `(Pubkey,
Account)` pairs.
In `@magicblock-chainlink/src/remote_account_provider/mod.rs`:
- Around line 981-1005: The get_program_accounts_with_config path is still using
the shared RPC_FETCH_TIMEOUT intended for small fetches, so program-account
scans can time out under larger backfills. Update
get_program_accounts_with_config in remote_account_provider::mod to use a
separate longer timeout for program-account scans, ideally configurable, or add
a small retry/backoff before returning AccountResolutionsFailed. Keep the
existing fetch() behavior unchanged and make the timeout choice explicit in this
method so the scan path does not inherit the single-fetch budget.
🪄 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: 7628ee7f-2451-4e22-ad4a-3feea37564d4
📒 Files selected for processing (17)
.agents/context/crates/magicblock-accounts.md.agents/context/crates/magicblock-api.md.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-config.md.agents/specs/validator-specification.mdconfig.example.tomlmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/remote_account_provider/chain_rpc_client.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/testing/rpc_client_mock.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rs
🛑 Comments failed to post (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
2249-2287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No test coverage for the live-subscription undelegation-request decode/broadcast path.
The polling path (
fetch_undelegation_requests) has a dedicated test, but this branch — decoding anUndelegationRequestfrom a forwarded subscription update and broadcasting it viaundelegation_request_sender— has no direct test. Given it's a new, non-trivial control-flow branch (only reached when the speculative delegation-record companion lookup misses), a regression here would silently break the low-latency discovery path while the polling backstop masks it.🤖 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-chainlink/src/chainlink/fetch_cloner/mod.rs` around lines 2249 - 2287, Add a direct test for the live-subscription undelegation-request branch in fetch_cloner’s update-handling path, since only the polling path is currently covered. Exercise the `UndelegationRequest::try_from_bytes_with_discriminator` decode from a forwarded account update, verify the `ObservedUndelegationRequest` is built correctly, and assert it is sent through `undelegation_request_sender` when the delegation-record companion lookup misses. Use the surrounding control flow in `fetch_cloner`/`DelegationActions` and the `ObservedUndelegationRequest` fields to locate and validate this branch.magicblock-chainlink/src/testing/rpc_client_mock.rs (1)
438-463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
.lock().unwrap()in new mock code.As per path instructions,
.unwrap()/.expect()inmagicblock-*/**production Rust should be flagged as major with justification. This is test-only mock infrastructure (src/testing/rpc_client_mock.rs), consistent with the same.lock().unwrap()pattern already used elsewhere in this file for the same non-poisonable, non-.await-spanningMutex. No behavior change or new risk introduced; flagging for visibility per policy rather than as a functional concern.As per path instructions, "Treat any usage of
.unwrap()or.expect()in production Rust code as a MAJOR issue... Request proper error handling or explicit justification with invariants." Justification: thisMutexguards an in-memory test double, is never held across.await, and mirrors the pre-existing convention used throughout this same file.🤖 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-chainlink/src/testing/rpc_client_mock.rs` around lines 438 - 463, The `get_program_accounts_with_config` mock uses `self.accounts.lock().unwrap()`, which is the only new unwrap/expect concern in this test-only RPC mock. Either replace it with proper poisoning/error handling that propagates through the existing `client_error::Result`, or add an explicit invariant comment in the `rpc_client_mock` implementation near `self.accounts` explaining that this in-memory `Mutex` is only used in the test double, is never held across `await`, and intentionally follows the same established pattern used elsewhere in this file.Source: Path instructions
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
01af044 to
297031f
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
297031f to
ddad8a9
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
ddad8a9 to
bce382f
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
bce382f to
9f35077
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
9f35077 to
350f1cc
Compare
snawaz
force-pushed
the
graphite-base/1378
branch
from
c55461d to
9eee800
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
350f1cc to
bcab82e
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
bcab82e to
955e518
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
b7ce6f8 to
e8006e0
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
e8006e0 to
30a4265
Compare
snawaz
force-pushed
the
snawaz/backfill-req
branch
from
30a4265 to
fa04fb0
Compare
snawaz
force-pushed
the
graphite-base/1378
branch
from
b0f37e5 to
dd1e180
Compare
1 participant
5m) fetches UndelegationRequest PDAs and processes them.