chore: refactor chainlink #803
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
|
Warning Rate limit exceeded@GabrielePicco has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 27 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive fetch-and-clone system for the Solana chainlink component. It adds new modules implementing account fetching, delegation record handling, ATA/eATA projection, and account cloning with subscription management. Core additions include the FetchCloner struct orchestrating the workflow, pipeline functions classifying and resolving accounts, delegation resolution logic, program loading utilities, subscription cancellation strategies, supporting data types, and extensive test coverage. A configuration file is updated to point to new remote endpoints. 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: 4
Fix all issues with AI Agents 🤖
In @magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs:
- Around line 1064-1070: The closure passed to .map(|mut accs| { ... })
currently uses accs.pop().unwrap() twice which must be replaced with explicit
error handling: check accs.len() or pattern-match accs.as_slice() (e.g., match
accs.as_slice() { [first, last] | [first, .., last] => (first.clone(),
last.clone()), _ => return Err(...) }) and propagate a meaningful error (or
convert to Result) instead of panicking; update the closure signature to return
Result<(RemoteAccount, RemoteAccount), YourErrorType> (or map_err accordingly)
so callers can handle the failure, and include clear error text mentioning the
missing accounts when constructing the Err.
In @magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs:
- Around line 1207-1284: Rename the test function
test_fetch_with_some_acounts_marked_as_empty_if_not_found to correct the typo
(test_fetch_with_some_accounts_marked_as_empty_if_not_found); update the fn
declaration and any references to that symbol (e.g., in the #[tokio::test]
annotated function name) so the test name matches the corrected spelling and
compiles/runs cleanly.
In @magicblock-chainlink/src/chainlink/fetch_cloner/types.rs:
- Around line 26-33: The ExistingSubs struct contains delegation_records and
program_data_accounts that are computed in build_existing_subs but never used;
either remove these two fields from the ExistingSubs definition and stop
computing/populating them in build_existing_subs (and remove any temporary
variables/queries that only existed to build them), or if they are intended for
future use, add a clear doc comment on the delegation_records and
program_data_accounts fields explaining their planned purpose and why the
current call site intentionally discards them, and keep build_existing_subs
unchanged; locate the struct by the name ExistingSubs and the builder function
build_existing_subs to apply the change.
♻️ Duplicate comments (1)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
967-983: Same.expect()issue as noted above.Line 971 has the same mutex
.expect()pattern that should be addressed consistently with line 916.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
magicblock-chainlink/src/chainlink/fetch_cloner.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/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rsmagicblock-chainlink/src/chainlink/fetch_cloner/subscription.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rstest-integration/configs/cloning-conf.ephem.toml
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/fetch_cloner/subscription.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/configs/cloning-conf.ephem.toml
🧠 Learnings (11)
📓 Common learnings
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rsmagicblock-chainlink/src/chainlink/fetch_cloner/subscription.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/fetch_cloner/subscription.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rsmagicblock-chainlink/src/chainlink/fetch_cloner/subscription.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs
📚 Learning: 2025-12-17T12:46:36.207Z
Learnt from: GabrielePicco
Repo: magicblock-labs/magicblock-validator PR: 738
File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196
Timestamp: 2025-12-17T12:46:36.207Z
Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/fetch_cloner/subscription.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/types.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs
📚 Learning: 2025-11-19T11:31:24.218Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/subscription.rs
📚 Learning: 2025-11-25T11:07:20.001Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/programs/flexi-counter/src/processor.rs:643-680
Timestamp: 2025-11-25T11:07:20.001Z
Learning: In the compressed delegation architecture, ownership reassignment during undelegation is handled by the delegation program itself, not by the owner program's external undelegate handler. The external undelegate handler (e.g., `process_external_undelegate_compressed` in flexi-counter) is a callback for program-specific cleanup (restoring lamports and data) after the delegation program has already reassigned ownership back to the original owner stored in the CompressedDelegationRecord.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner/delegation.rs
🧬 Code graph analysis (4)
magicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rs (1)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
handle_executable_sub_update(339-410)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (2)
magicblock-core/src/token_programs.rs (1)
try_derive_eata_address_and_bump(81-89)magicblock-chainlink/src/chainlink/fetch_cloner/delegation.rs (2)
fetch_and_parse_delegation_record(83-154)get_delegated_to_other(66-81)
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs (4)
magicblock-chainlink/src/testing/utils.rs (1)
create_test_lru_cache(106-116)magicblock-chainlink/src/remote_account_provider/remote_account.rs (5)
set_remote_slot(129-136)remote_slot(162-168)delegated(95-101)executable(87-93)confined(112-118)magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (2)
new(98-127)fetch_count(130-132)magicblock-chainlink/src/chainlink/fetch_cloner/types.rs (1)
is_ok(77-80)
magicblock-chainlink/src/chainlink/fetch_cloner/types.rs (2)
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs (2)
std(247-247)std(1406-1406)magicblock-chainlink/src/chainlink/fetch_cloner/subscription.rs (1)
fmt(50-101)
🔇 Additional comments (50)
test-integration/configs/cloning-conf.ephem.toml (2)
3-3: Verify that the listen port change doesn't cause conflicts.The listen port changed from
8899to7799. Previously, the remotes configuration pointed to0.0.0.0:7799, and now the service itself listens on7799. Please verify that this port change aligns with the new architecture and doesn't cause port conflicts or break existing integration test infrastructure.
5-6: Remove the review comment.The external devnet endpoints in
cloning-conf.ephem.tomlare the correct configuration for this test. The cloning tests specifically validate the ephemeral validator's ability to clone accounts from a remote cluster—a core feature requiring connection to actual remote infrastructure. This is not a reliability concern but a necessary architectural choice for testing account cloning functionality.Likely an incorrect or invalid review comment.
magicblock-chainlink/src/chainlink/fetch_cloner/program_loader.rs (4)
1-14: LGTM!Imports are well-organized and appropriate for the module's functionality.
16-39: LGTM!Good early-exit guards for:
- Unauthorized programs (with debug logging)
- LoaderV1 programs that shouldn't receive subscription updates (with error logging)
The defensive checks align with the existing implementation pattern in
fetch_cloner.rs.
41-75: LGTM!LoaderV3 program data fetching with comprehensive error handling. Both
Ok(Err(...))andErr(...)cases are properly logged and cause early returns. The fallback for non-LoaderV3 programs correctly passes through the original account withNonefor program data.
77-94: LGTM!Program resolution and cloning with proper error handling:
ProgramAccountResolver::try_newfailure is logged and returns early- Clone failure is logged appropriately
All error paths are handled without panics.
magicblock-chainlink/src/chainlink/fetch_cloner/subscription.rs (5)
1-9: LGTM!Imports are appropriate for the module's functionality.
11-26: LGTM!Well-structured enum representing the three cancellation strategies. The doc comments clearly explain each variant's purpose.
28-47: Verifyis_emptysemantics forNewandHybridvariants.The
is_emptyimplementation forCancelStrategy::Newreturnstrueonly when bothnew_subsandexisting_subsare empty. However, looking at the usage incancel_subs(lines 117-120), the actual subscriptions to cancel are computed asnew_subs.difference(&existing_subs), meaningexisting_subsis used as an exclusion filter, not as a source of subscriptions to cancel.If
new_subsis empty butexisting_subsis non-empty,is_empty()returnsfalse, but the function would have no subscriptions to cancel (sincedifferenceof empty set with anything is empty).Consider whether
is_emptyshould reflect "will any subscriptions be cancelled" rather than "are all fields empty":// For New variant, only new_subs matters since existing_subs is exclusion filter CancelStrategy::New { new_subs, .. } => new_subs.is_empty(),The same consideration applies to the
Hybridvariant.
49-102: LGTM!The
Displayimplementation provides clear debugging output for each strategy variant.
104-161: LGTM!The
cancel_subsfunction demonstrates good practices:
- Early exit when strategy is empty
- Race condition prevention by checking
is_pendingbefore unsubscribing- Concurrent unsubscription via
JoinSet- Non-fatal error handling (warning logs instead of propagating errors)
This aligns with the retrieved learning that unsubscribe operations resolve to
()and shouldn't be inspected for results.magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs (8)
1-26: LGTM!Test imports are comprehensive and appropriate.
40-85: LGTM!Well-designed assertion macros that encapsulate common verification patterns for cloned accounts, checking lamports, data, owner, delegation status, and remote_slot.
87-164: LGTM!Good test context structure and setup helper. The
#[allow(unused)]annotations onforward_rxandsubscription_txare appropriate - these are kept alive to prevent channel errors, as per the retrieved learning about keeping receivers alive.
166-188: LGTM!Clean helper function for initializing FetchCloner in tests.
193-234: LGTM!Good basic test for non-delegated account fetch and clone.
273-349: LGTM!Thorough test for delegated accounts with valid delegation records, verifying:
- Account is cloned with correct delegation properties
- Delegation record itself is not cloned
- Delegated accounts are not subscribed (since we control them)
869-952: LGTM!Excellent test for race condition prevention during concurrent operations on the same account. This validates the
is_pendingcheck incancel_subs.
1596-1661: LGTM!Good edge case test verifying that an empty
allowed_programslist is treated as unrestricted (allows all programs).magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (5)
1-15: LGTM!Imports are well-organized for the module's ATA/eATA projection functionality.
17-40: LGTM!Clear function signature with appropriate generic bounds and well-documented purpose.
42-70: Subscriptions are kept active intentionally.The code subscribes to both the ATA and derived eATA but doesn't cancel these subscriptions even on errors. This aligns with the retrieved learning that eATA subscriptions are intentionally NOT cancelled to watch for future creation or delegation. Based on learnings, this is the correct behavior.
72-90: LGTM!Proper error handling for join results - logs warnings and continues processing remaining items rather than failing the entire batch.
92-135: LGTM!Delegation-aware ATA projection logic:
- Defaults to cloning ATA as-is
- If eATA exists with delegation record, applies projection when delegated to us
- Correctly sets delegated flag on projected ATA
- Always returns an
AccountCloneRequestfor each processed ATAmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rs (5)
1-18: LGTM!Imports are appropriate for delegation record handling.
20-32: LGTM!Clean delegation record parsing with proper error mapping to
ChainlinkError::InvalidDelegationRecord.
34-64: LGTM!Delegation record application logic correctly:
- Determines confinement status (authority is default pubkey)
- Sets owner and confined flags unconditionally
- Sets delegated flag based on authority matching and owner not being EATA_PROGRAM_ID
- Returns commit_frequency_ms only when delegated to us
66-81: LGTM!Clean helper to determine if account is delegated to another validator.
83-154: LGTM!Well-structured async delegation record fetch with:
- Proper slot matching constraints
- Conditional unsubscription only when:
- Was not already watching the delegation record
- Account doesn't exist in bank (handles edge case of concurrent clone)
- Error logging for unsubscription failures (non-fatal)
The comment on lines 133-135 acknowledges the small possibility of a concurrent fetch+clone, which is acceptable.
magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs (8)
1-30: LGTM!Imports are comprehensive and appropriate for the pipeline orchestration module.
32-63: LGTM!Clean helper to build existing subscription sets for accounts, delegation records, and program data accounts.
65-151: LGTM!Well-structured account classification using fold pattern. The classification logic correctly:
- Separates not-found from found accounts
- Identifies delegation program owned accounts
- Filters native loader programs with appropriate warning
- Detects ATAs for special handling
- Falls back to plain accounts for everything else
The warning at line 114-116 about native loader programs is good defensive coding.
153-166: LGTM!Simple and correct partition logic for not-found accounts.
168-320: LGTM!Comprehensive delegation resolution with proper error handling:
- Spawns concurrent fetch tasks for delegation records
- Collects errors and aborts with subscription cleanup on failure
- Parses delegation records and applies to accounts
- Tracks missing delegation records for caller awareness
- Uses
CancelStrategy::Newfor cleanup on error paths
322-504: LGTM!Well-structured program resolution:
- Partitions LoaderV3 programs from single-account programs
- Batch fetches program + program_data pairs with slot matching
- Validates expected vs actual account counts
- Uses
ProgramAccountResolverwith proper error propagation via?- Cleans up subscriptions on error via
CancelStrategy::New
506-571: LGTM!Good cancellation strategy computation with a safety assertion in test builds to verify the new approach matches the old approach. The
#[cfg(test)]block ensures this validation runs during testing without impacting production performance.
573-621: LGTM!Clean concurrent cloning implementation:
- Spawns account clone tasks
- Filters programs by allowed_programs before cloning
- Collects all results and propagates first error via
?Good use of
JoinSetfor concurrent operations.magicblock-chainlink/src/chainlink/fetch_cloner/types.rs (6)
1-11: LGTM!Imports are appropriate for the type definitions.
13-18: LGTM!Clean struct for pairing an account with its optional companion (delegation record or program data).
20-24: LGTM!Clear enum representing refresh decision options.
35-57: LGTM!Well-structured intermediate types for the pipeline stages:
ClassifiedAccountsfor categorized fetch resultsResolvedDelegatedAccountsfor delegation resolution outputResolvedProgramsfor program resolution output
59-81: LGTM!Clean result type with convenient accessor methods and
is_ok()check.
83-111: LGTM!Good
Displayimplementation providing human-readable output for debugging and logging.magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (7)
61-88: LGTM!The struct design is well-organized with appropriate generic bounds. Using
Arc<Mutex<HashMap<...>>>forpending_requestsis suitable for coordinating concurrent fetch deduplication.
98-127: LGTM!The constructor properly initializes all fields and starts the subscription listener on a cloned Arc reference, avoiding lifetime issues.
291-515: LGTM!The method correctly handles multiple account resolution scenarios (delegation-owned, ATA with eATA projection, plain accounts) with appropriate error logging and subscription management. The nested control flow is complex but necessary given the business logic requirements.
1084-1146: LGTM!The
resolve_account_with_companionmethod uses comprehensive pattern matching to handle all account/companion combinations with proper error propagation. This is a good example of idiomatic Rust error handling.
1148-1188: LGTM!The utility methods are concise delegators with appropriate error mapping. The
is_watching,subscribe_to_account, and other accessors provide a clean public API.
1189-1221: LGTM!The
airdrop_account_if_emptymethod has appropriate guard conditions and delegates to the cloner for the actual account creation. The discarded signature is acceptable for a best-effort airdrop operation.
163-165: TODO: Consider parallelizing delegation record fetches for high subscription volumes.The comment indicates a potential scalability concern when handling many subscription updates. The current sequential processing may become a bottleneck.
Would you like me to open an issue to track implementing parallel delegation record fetching, or help design a solution using a bounded task pool?
⛔ Skipped due to learnings
Learnt from: thlorenz Repo: magicblock-labs/magicblock-validator PR: 621 File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495 Timestamp: 2025-11-07T14:20:31.457Z Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.Learnt from: bmuddha Repo: magicblock-labs/magicblock-validator PR: 578 File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27 Timestamp: 2025-10-21T14:00:54.642Z Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579Learnt from: thlorenz Repo: magicblock-labs/magicblock-validator PR: 703 File: magicblock-chainlink/src/submux/mod.rs:652-654 Timestamp: 2025-12-01T16:02:05.367Z Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.Learnt from: GabrielePicco Repo: magicblock-labs/magicblock-validator PR: 738 File: magicblock-chainlink/src/chainlink/fetch_cloner.rs:1109-1196 Timestamp: 2025-12-17T12:46:36.207Z Learning: In magicblock-chainlink/src/chainlink/fetch_cloner.rs, eATA account subscriptions are intentionally NOT cancelled even when the eATA doesn't exist or delegation conversion fails. The subscriptions are kept active to watch for future creation or delegation of eATA accounts (per maintainer GabrielePicco).
GabrielePicco
changed the title
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Summary
Refactor cloning pipeline
Compatibility
Testing
Checklist