chore: fetch metrics more granular via origin and reason#1417
chore: fetch metrics more granular via origin and reason#1417thlorenz wants to merge 18 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughReplaces 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 |
|
@copilot resolve the merge conflicts in this pull request |
![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 (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (2)
3181-3195: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWrong
AccountFetchReasonintask_to_fetch_with_delegation_record: tags delegation-record companion fetches asProgramData.fn task_to_fetch_with_delegation_record(...) -> ... { let delegation_record_pubkey = delegation_record_pda_from_delegated_account(&pubkey); self.task_to_fetch_with_companion( pubkey, delegation_record_pubkey, slot, fetch_context.with_reason(AccountFetchReason::ProgramData), // wrong reason ) }This helper fetches an account together with its delegation-record companion, but tags the fetch with
AccountFetchReason::ProgramData. Compare withtask_to_fetch_with_program_data(lines 3197-3211) andresolve_programs_with_program_datainpipeline.rs(line 504), which correctly useAccountFetchReason::ProgramDatafor program+programdata pairs. This is the only caller oftask_to_fetch_with_delegation_record(viapipeline::resolve_delegated_accounts), so every delegated-account resolution done through the mainclone_accountspipeline is mislabeled as a program-data fetch instead of a delegation-record fetch — undermining the metric-attribution goal of this PR. This also contradicts the AI-generated line-range summary for this segment, which states this function "passesfetch_contextwith aDelegationRecordreason."🐛 Proposed fix
fn task_to_fetch_with_delegation_record( &self, pubkey: Pubkey, slot: u64, fetch_context: AccountFetchContext, ) -> task::JoinHandle<ChainlinkResult<AccountWithCompanion>> { let delegation_record_pubkey = delegation_record_pda_from_delegated_account(&pubkey); self.task_to_fetch_with_companion( pubkey, delegation_record_pubkey, slot, - fetch_context.with_reason(AccountFetchReason::ProgramData), + fetch_context.with_reason(AccountFetchReason::DelegationRecord), ) }🤖 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 3181 - 3195, Update task_to_fetch_with_delegation_record to pass the AccountFetchReason::DelegationRecord reason when calling task_to_fetch_with_companion, while preserving the existing delegation_record_pubkey derivation and fetch flow.
2352-2410: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the caller fetch reason in
fetch_and_clone_accounts
clone_accountsshould keep the originalfetch_contexthere; forcingDelegationRecordonly on the ATA path makes this branch report clone metrics differently fromfetch_and_clone_accounts_with_dedupand misattributes non-delegation clones.🤖 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 2352 - 2410, Preserve the caller-provided fetch context when invoking clone_accounts from fetch_and_clone_accounts. Remove the forced AccountFetchReason::DelegationRecord override and pass fetch_context unchanged, matching fetch_and_clone_accounts_with_dedup so non-delegation clone metrics retain their original attribution.
🤖 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/service.rs`:
- Line 383: Update the committor recovery path at AccountFetchContext
construction to use
AccountFetchContext::internal(AccountFetchReason::RequestedAccount) instead of
AccountFetchContext::rpc_get_account(). Keep the recovery behavior unchanged
while attributing the fetch to the internal entrypoint.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 3181-3195: Update task_to_fetch_with_delegation_record to pass the
AccountFetchReason::DelegationRecord reason when calling
task_to_fetch_with_companion, while preserving the existing
delegation_record_pubkey derivation and fetch flow.
- Around line 2352-2410: Preserve the caller-provided fetch context when
invoking clone_accounts from fetch_and_clone_accounts. Remove the forced
AccountFetchReason::DelegationRecord override and pass fetch_context unchanged,
matching fetch_and_clone_accounts_with_dedup so non-delegation clone metrics
retain their original attribution.
🪄 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: 8b2de71d-18af-49a3-a4fe-96280130e53a
📒 Files selected for processing (23)
.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-metrics.mdmagicblock-aperture/src/requests/http/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pending_operation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/lib.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/remote_account_provider/tests.rsmagicblock-chainlink/tests/01_ensure-accounts.rsmagicblock-chainlink/tests/03_deleg_after_sub.rsmagicblock-chainlink/tests/08_subupdate-ordering.rsmagicblock-chainlink/tests/09_waiter_reconciliation_race.rsmagicblock-chainlink/tests/basics.rsmagicblock-chainlink/tests/utils/test_context.rsmagicblock-committor-service/src/service.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-metrics/src/metrics/types.rs
…in-splits # Conflicts: # magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
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 (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (2)
2408-2443: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the caller’s fetch reason when entering the clone pipeline.
fetch_and_clone_accountsaccepts generic contexts such as RPC and greedy-discovery contexts, but Line 2442 relabels every clone asDelegationRecord. This misattributes normal account, ATA, and discovery clone/materialization metrics. Passfetch_contextunchanged; callers performing delegation-record work should set that reason themselves.Proposed fix
self.clone_accounts( pubkeys, accs, mark_empty_if_not_found, slot, - fetch_context.with_reason(AccountFetchReason::DelegationRecord), + fetch_context, )🤖 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 2408 - 2443, Update fetch_and_clone_accounts to pass the existing fetch_context unchanged into clone_accounts instead of applying AccountFetchReason::DelegationRecord. Preserve delegation-record attribution through callers that explicitly provide that reason.
3216-3230: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAttribute delegation-record companion fetches as
DelegationRecord.
task_to_fetch_with_delegation_recordderives a delegation-record PDA, but Line 3228 appliesAccountFetchReason::ProgramData. Pending-fetch and remote-fetch metrics for delegation-record RPCs will therefore be labeled as program-data fetches.Proposed fix
- fetch_context.with_reason(AccountFetchReason::ProgramData) + fetch_context.with_reason(AccountFetchReason::DelegationRecord)🤖 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 3216 - 3230, Update task_to_fetch_with_delegation_record so its fetch_context uses AccountFetchReason::DelegationRecord instead of AccountFetchReason::ProgramData, ensuring pending-fetch and remote-fetch metrics for delegation-record RPCs receive the correct attribution.
🤖 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 758-763: Remove the production expect call in the ownership-claim
loop around local_account_satisfies_clone_request. Handle a missing request
explicitly by returning the appropriate ClonerResult error, or enforce the
invariant before entering this path so the code no longer relies on a runtime
panic; preserve normal behavior when the request is present.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 2408-2443: Update fetch_and_clone_accounts to pass the existing
fetch_context unchanged into clone_accounts instead of applying
AccountFetchReason::DelegationRecord. Preserve delegation-record attribution
through callers that explicitly provide that reason.
- Around line 3216-3230: Update task_to_fetch_with_delegation_record so its
fetch_context uses AccountFetchReason::DelegationRecord instead of
AccountFetchReason::ProgramData, ensuring pending-fetch and remote-fetch metrics
for delegation-record RPCs receive the correct attribution.
🪄 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: e576b752-917c-4a70-ba77-20c9d5314625
📒 Files selected for processing (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (2)
2408-2443: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the caller’s fetch reason when entering the clone pipeline.
fetch_and_clone_accountsaccepts generic contexts such as RPC and greedy-discovery contexts, but Line 2442 relabels every clone asDelegationRecord. This misattributes normal account, ATA, and discovery clone/materialization metrics. Passfetch_contextunchanged; callers performing delegation-record work should set that reason themselves.Proposed fix
self.clone_accounts( pubkeys, accs, mark_empty_if_not_found, slot, - fetch_context.with_reason(AccountFetchReason::DelegationRecord), + fetch_context, )🤖 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 2408 - 2443, Update fetch_and_clone_accounts to pass the existing fetch_context unchanged into clone_accounts instead of applying AccountFetchReason::DelegationRecord. Preserve delegation-record attribution through callers that explicitly provide that reason.
3216-3230: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAttribute delegation-record companion fetches as
DelegationRecord.
task_to_fetch_with_delegation_recordderives a delegation-record PDA, but Line 3228 appliesAccountFetchReason::ProgramData. Pending-fetch and remote-fetch metrics for delegation-record RPCs will therefore be labeled as program-data fetches.Proposed fix
- fetch_context.with_reason(AccountFetchReason::ProgramData) + fetch_context.with_reason(AccountFetchReason::DelegationRecord)🤖 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 3216 - 3230, Update task_to_fetch_with_delegation_record so its fetch_context uses AccountFetchReason::DelegationRecord instead of AccountFetchReason::ProgramData, ensuring pending-fetch and remote-fetch metrics for delegation-record RPCs receive the correct attribution.
🤖 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 758-763: Remove the production expect call in the ownership-claim
loop around local_account_satisfies_clone_request. Handle a missing request
explicitly by returning the appropriate ClonerResult error, or enforce the
invariant before entering this path so the code no longer relies on a runtime
panic; preserve normal behavior when the request is present.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 2408-2443: Update fetch_and_clone_accounts to pass the existing
fetch_context unchanged into clone_accounts instead of applying
AccountFetchReason::DelegationRecord. Preserve delegation-record attribution
through callers that explicitly provide that reason.
- Around line 3216-3230: Update task_to_fetch_with_delegation_record so its
fetch_context uses AccountFetchReason::DelegationRecord instead of
AccountFetchReason::ProgramData, ensuring pending-fetch and remote-fetch metrics
for delegation-record RPCs receive the correct attribution.
🪄 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: e576b752-917c-4a70-ba77-20c9d5314625
📒 Files selected for processing (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rs
🛑 Comments failed to post (1)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
758-763: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the production
expectfrom the ownership path.Line 762 can panic the fetch-cloner task instead of returning a
ClonerResulterror if the request state is unexpectedly consumed. Replace it with explicit error handling, or document and enforce the invariant without relying on a production panic.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.”🤖 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 758 - 763, Remove the production expect call in the ownership-claim loop around local_account_satisfies_clone_request. Handle a missing request explicitly by returning the appropriate ClonerResult error, or enforce the invariant before entering this path so the code no longer relies on a runtime panic; preserve normal behavior when the request is present.Source: Path instructions
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-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
1790-1805: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the parent entrypoint for projected ATA fetches.
AccountFetchContext::project_ata()hard-codes theProjectAtaentrypoint, but this helper is called from subscription-update and greedy-discovery paths. That relabels internal fetches and clone metrics instead of preservingSubscriptionUpdatewhile changing only the reason toAtaProjection. Thread the caller context into this helper and usefetch_context.with_reason(AccountFetchReason::AtaProjection).Proposed fix
async fn clone_projected_ata_request( &self, request: AccountCloneRequest, + fetch_context: AccountFetchContext, ) -> ChainlinkResult<Signature> { self.clone_account_with_post_delegation_action_invariants( request, - AccountFetchContext::project_ata(), + fetch_context.with_reason(AccountFetchReason::AtaProjection), ) .await }Pass the appropriate subscription/discovery context at each caller.
🤖 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 1790 - 1805, Update clone_projected_ata_request to accept the caller’s AccountFetchContext, then derive the context with fetch_context.with_reason(AccountFetchReason::AtaProjection) instead of constructing AccountFetchContext::project_ata(). Update every caller, including subscription-update and greedy-discovery paths, to pass its existing context so the parent entrypoint and metrics are preserved.
🤖 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-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 1790-1805: Update clone_projected_ata_request to accept the
caller’s AccountFetchContext, then derive the context with
fetch_context.with_reason(AccountFetchReason::AtaProjection) instead of
constructing AccountFetchContext::project_ata(). Update every caller, including
subscription-update and greedy-discovery paths, to pass its existing context so
the parent entrypoint and metrics are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 654fdd35-807d-4764-b678-c73b02ff3b95
📒 Files selected for processing (12)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-committor-service/src/service.rstest-integration/test-chainlink/src/test_context.rstest-integration/test-chainlink/tests/ix_01_ensure-accounts.rstest-integration/test-chainlink/tests/ix_03_deleg_after_sub.rstest-integration/test-chainlink/tests/ix_06_redeleg_us_separate_slots.rstest-integration/test-chainlink/tests/ix_07_redeleg_us_same_slot.rstest-integration/test-chainlink/tests/ix_ata_eata_replace.rstest-integration/test-chainlink/tests/ix_exceed_capacity.rstest-integration/test-chainlink/tests/ix_full_scenarios.rstest-integration/test-chainlink/tests/ix_programs.rstest-integration/test-chainlink/tests/ix_remote_account_provider.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea7f2a62cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
* master: fix: distinguish ER account presence from delegation status (#1411) fix: refuse impossible sized intents (#1390) feat: Impl UndelegationRequest backfill loop (#1378) fix: increase RPC fetch retries to survive RPC lag behind pubsub tip (#1425) feat: Schedule undelegation from observed DLP requests (#1357) feat: add id for actions (#1409) release: 0.13.7 (#1423) refactor: move LatestBlockInner into core to be able to expose subscribe in LatestBlockProvider trait (#1405) Fix buffer preparation signature aliasing (#1420) feat: add ephemeral-system-program that handles ephemeral account lifecycle (#1415) fix: do not warn about the first false positive blockhash divergence (#1418)
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-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
481-487: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid fetching the System Program as a dummy companion.
Pubkey::default()resolves to the Solana System Program (11111111111111111111111111111111), which exists on-chain. Using it as a "dummy" companion results in a successful account fetch rather than returningNotFound, followed by an unnecessary attempt to fetch a delegation record for the System Program. This pollutes the cache and wastes RPC calls for every failed eATA derivation.Consider implementing a
task_to_fetch_single_atahelper onFetchClonerthat fetches the ATA alone without a companion to resolve this inefficiency.⚡ Proposed fix
First, implement the helper method in
FetchCloner(e.g., inmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs):pub(crate) fn task_to_fetch_single_ata( &self, pubkey: Pubkey, slot: u64, fetch_context: AccountFetchContext, ) -> task::JoinHandle<ChainlinkResult<AccountWithCompanion>> { let provider = self.remote_account_provider.clone(); let bank = self.accounts_bank.clone(); let fetch_count = self.fetch_count.clone(); task::spawn(async move { fetch_count.fetch_add(1, std::sync::atomic::Ordering::Relaxed); provider .try_get_multi(&[pubkey], None, fetch_context, Some(slot)) .await .map_err(ChainlinkError::from) .and_then(|mut accs| { let acc = accs.pop().ok_or_else(|| { ChainlinkError::UnexpectedAccountCount( "Expected exactly 1 account".to_string() ) })?; let account = acc.account.resolved_account_shared_data(&bank) .ok_or_else(|| ChainlinkError::ResolvedAccountCouldNoLongerBeFound(pubkey))?; Ok(AccountWithCompanion { pubkey, account, companion_pubkey: Pubkey::default(), companion_account: None, }) }) }) }Then, use it here in the fallback block:
- ata_join_set.spawn(FetchCloner::task_to_fetch_with_companion( - this, - *ata_pubkey, - Pubkey::default(), // Dummy companion - will be marked as NotFound - effective_slot, - ata_projection_context, - )); + ata_join_set.spawn(this.task_to_fetch_single_ata( + *ata_pubkey, + effective_slot, + ata_projection_context, + ));🤖 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/ata_projection.rs` around lines 481 - 487, The fallback in the ATA projection currently calls FetchCloner::task_to_fetch_with_companion with Pubkey::default(), causing an unnecessary System Program fetch and delegation lookup. Add a FetchCloner::task_to_fetch_single_ata helper that fetches and resolves only the ATA while leaving companion_account as None, then replace the fallback call in the ata_join_set block with this helper.
🤖 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-metrics/src/metrics/mod.rs`:
- Around line 501-537: Replace the `.unwrap()` calls on the metric registrations
`COMMITTOR_INTENT_TASK_PREPARATION_TIME`,
`COMMITTOR_INTENT_ALT_PREPARATION_TIME`, `COMMITTOR_INTENT_ALT_COUNT`, and
`COMMITTOR_FETCH_COMMIT_NONCES_WAIT_TIME` with the project’s established
production error-handling approach, or add an explicit invariant-based
justification if initialization failure is intentionally fatal. Do not leave
unexplained unwraps in these static metric definitions.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs`:
- Around line 481-487: The fallback in the ATA projection currently calls
FetchCloner::task_to_fetch_with_companion with Pubkey::default(), causing an
unnecessary System Program fetch and delegation lookup. Add a
FetchCloner::task_to_fetch_single_ata helper that fetches and resolves only the
ATA while leaving companion_account as None, then replace the fallback call in
the ata_join_set block with this helper.
🪄 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: 497a91d7-955b-4f08-836e-8f969a8b3159
📒 Files selected for processing (9)
.agents/context/crates/magicblock-chainlink.mdmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.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/mod.rsmagicblock-committor-service/src/service.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-services/src/undelegation_request_service.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: 1
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/fetch_cloner/ata_projection.rs (1)
481-487: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid fetching the System Program as a dummy companion.
Pubkey::default()resolves to the Solana System Program (11111111111111111111111111111111), which exists on-chain. Using it as a "dummy" companion results in a successful account fetch rather than returningNotFound, followed by an unnecessary attempt to fetch a delegation record for the System Program. This pollutes the cache and wastes RPC calls for every failed eATA derivation.Consider implementing a
task_to_fetch_single_atahelper onFetchClonerthat fetches the ATA alone without a companion to resolve this inefficiency.⚡ Proposed fix
First, implement the helper method in
FetchCloner(e.g., inmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs):pub(crate) fn task_to_fetch_single_ata( &self, pubkey: Pubkey, slot: u64, fetch_context: AccountFetchContext, ) -> task::JoinHandle<ChainlinkResult<AccountWithCompanion>> { let provider = self.remote_account_provider.clone(); let bank = self.accounts_bank.clone(); let fetch_count = self.fetch_count.clone(); task::spawn(async move { fetch_count.fetch_add(1, std::sync::atomic::Ordering::Relaxed); provider .try_get_multi(&[pubkey], None, fetch_context, Some(slot)) .await .map_err(ChainlinkError::from) .and_then(|mut accs| { let acc = accs.pop().ok_or_else(|| { ChainlinkError::UnexpectedAccountCount( "Expected exactly 1 account".to_string() ) })?; let account = acc.account.resolved_account_shared_data(&bank) .ok_or_else(|| ChainlinkError::ResolvedAccountCouldNoLongerBeFound(pubkey))?; Ok(AccountWithCompanion { pubkey, account, companion_pubkey: Pubkey::default(), companion_account: None, }) }) }) }Then, use it here in the fallback block:
- ata_join_set.spawn(FetchCloner::task_to_fetch_with_companion( - this, - *ata_pubkey, - Pubkey::default(), // Dummy companion - will be marked as NotFound - effective_slot, - ata_projection_context, - )); + ata_join_set.spawn(this.task_to_fetch_single_ata( + *ata_pubkey, + effective_slot, + ata_projection_context, + ));🤖 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/ata_projection.rs` around lines 481 - 487, The fallback in the ATA projection currently calls FetchCloner::task_to_fetch_with_companion with Pubkey::default(), causing an unnecessary System Program fetch and delegation lookup. Add a FetchCloner::task_to_fetch_single_ata helper that fetches and resolves only the ATA while leaving companion_account as None, then replace the fallback call in the ata_join_set block with this helper.
🤖 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-metrics/src/metrics/mod.rs`:
- Around line 501-537: Replace the `.unwrap()` calls on the metric registrations
`COMMITTOR_INTENT_TASK_PREPARATION_TIME`,
`COMMITTOR_INTENT_ALT_PREPARATION_TIME`, `COMMITTOR_INTENT_ALT_COUNT`, and
`COMMITTOR_FETCH_COMMIT_NONCES_WAIT_TIME` with the project’s established
production error-handling approach, or add an explicit invariant-based
justification if initialization failure is intentionally fatal. Do not leave
unexplained unwraps in these static metric definitions.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs`:
- Around line 481-487: The fallback in the ATA projection currently calls
FetchCloner::task_to_fetch_with_companion with Pubkey::default(), causing an
unnecessary System Program fetch and delegation lookup. Add a
FetchCloner::task_to_fetch_single_ata helper that fetches and resolves only the
ATA while leaving companion_account as None, then replace the fallback call in
the ata_join_set block with this helper.
🪄 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: 497a91d7-955b-4f08-836e-8f969a8b3159
📒 Files selected for processing (9)
.agents/context/crates/magicblock-chainlink.mdmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.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/mod.rsmagicblock-committor-service/src/service.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-services/src/undelegation_request_service.rs
🛑 Comments failed to post (1)
magicblock-metrics/src/metrics/mod.rs (1)
501-537: 📐 Maintainability & Code Quality | 🟠 Major | 💤 Low value
Avoid using
.unwrap()in production code.As per path instructions, any usage of
.unwrap()or.expect()in production Rust code must be treated as a major issue. Although these metric registrations appear to be relocated from another part of the file, please provide proper error handling or an explicit justification with invariants for these.unwrap()calls (e.g., explaining why metric registration failure is guaranteed not to happen or is an acceptable fatal initialization failure).🤖 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-metrics/src/metrics/mod.rs` around lines 501 - 537, Replace the `.unwrap()` calls on the metric registrations `COMMITTOR_INTENT_TASK_PREPARATION_TIME`, `COMMITTOR_INTENT_ALT_PREPARATION_TIME`, `COMMITTOR_INTENT_ALT_COUNT`, and `COMMITTOR_FETCH_COMMIT_NONCES_WAIT_TIME` with the project’s established production error-handling approach, or add an explicit invariant-based justification if initialization failure is intentionally fatal. Do not leave unexplained unwraps in these static metric definitions.Source: Path instructions
Summary
Split account-fetch metric attribution into separate
entrypointandfetch_reasonlabels so operator dashboards can distinguish direct RPC reads from internal follow-up fetches such as subscription updates, delegation records, program-data fetches, action dependencies, and undelegating refreshes.Partially addresses #1366
Details
magicblock-metrics
Adds bounded fetch attribution types for the top-level entrypoint and immediate fetch reason, and updates fetch-related metrics to emit both labels instead of the previous overloaded origin label.
This affects account fetch result metrics, Chainlink pending-fetch metrics, bank-precheck metrics, clone lifecycle/materialization metrics, empty-placeholder metrics, and subscription-registration metrics. Signatures remain available for tracing correlation only and are not metric labels.
A practical query to validate the new attribution split is:
For bank-precheck attribution, use:
magicblock-chainlink
Threads
AccountFetchContextthrough the fetch/clone pipeline, pending-operation tracking, remote account provider, subscription registration, and clone metrics.Top-level flows now report distinct entrypoints such as
rpc_get_account,rpc_get_multiple_accounts,send_transaction, andsubscription_update. Internal follow-up work preserves the parent entrypoint while changingfetch_reason, so work triggered by a subscription update no longer looks like directgetAccounttraffic.Specific reasons now cover delegation records, program data, action dependency missing/forced-refresh paths, undelegating refreshes, subscription-update clones, greedy discovery, ATA projection, program loading, and clock fetches.
magicblock-aperture and committor service
Updates RPC and service call sites to pass explicit fetch contexts at the boundary. Direct HTTP get-account/get-multiple-account requests use the corresponding RPC entrypoints, transaction submission uses
send_transaction, and committor recovery checks use direct get-account context.Summary by CodeRabbit
AccountFetchContext, separating entrypoint and reason across fetch, cloning, pending operations, and subscription-driven flows.AccountFetchContextinstead of the prior origin-based model.entrypoint/fetch_reason-scoped metrics correctly, with context-aware pending-fetch tracking.