Skip to content

explore/refactor: explore and make minor improvements#1103

Draft
snawaz wants to merge 1 commit intomasterfrom
snawaz/explore
Draft

explore/refactor: explore and make minor improvements#1103
snawaz wants to merge 1 commit intomasterfrom
snawaz/explore

Conversation

@snawaz
Copy link
Copy Markdown
Contributor

@snawaz snawaz commented Mar 27, 2026

Summary

Compatibility

  • No breaking changes
  • Config change (describe):
  • Migration needed (describe):

Testing

  • tests (or explain)

Checklist

  • docs updated (if needed)
  • closes #

Summary by CodeRabbit

Release Notes

  • Documentation

    • Enhanced code documentation with clarifying comments for action dependency resolution and companion-fetch semantics.
  • Refactor

    • Improved internal naming conventions for better code clarity and maintainability.

@github-actions
Copy link
Copy Markdown

Manual Deploy Available

You can trigger a manual deploy of this PR branch to testnet:

Deploy to Testnet 🚀

Alternative: Comment /deploy on this PR to trigger deployment directly.

⚠️ Note: Manual deploy requires authorization. Only authorized users can trigger deployments.

Comment updated automatically when the PR is synchronized.

Copy link
Copy Markdown
Contributor Author

snawaz commented Mar 27, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

The changes update the fetch-cloner module with clarifications and documentation improvements. The modifications add checkpoint comments for better code clarity, refactor companion-fetch plumbing to use a generic "companion" naming convention (replacing "deleg"), and tighten failure behavior enforcement during delegated dependency resolution. Error handling for missing delegation action accounts is explicitly highlighted. No changes to exported public entity signatures or core logic flow were made.

Suggested reviewers

  • thlorenz
  • bmuddha
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch snawaz/explore

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

1330-1340: ⚠️ Potential issue | 🟠 Major

Cancel subscriptions before returning this action-dependency error.

This exit happens before the shared cancel_subs(...) call at Line 1388. By then the fetch has already created subscriptions for the original request, action_dependencies_to_fetch, and the action_dep_record_subs returned from resolve_delegated_accounts(), but those record subs are not yet merged into new_subs. On error we keep watching accounts we never clone. Please run the same cleanup here, and in the sibling not_found return above, before returning.

Suggested fix
             if !action_dep_missing_delegation_record.is_empty() {
+                let new_subs_to_cancel = new_subs
+                    .iter()
+                    .copied()
+                    .chain(action_dep_record_subs.iter().copied())
+                    .collect::<HashSet<_>>();
+                cancel_subs(
+                    &self.remote_account_provider,
+                    CancelStrategy::New {
+                        new_subs: new_subs_to_cancel,
+                        existing_subs: existing_subs.clone(),
+                    },
+                )
+                .await;
                 return Err(ChainlinkError::MissingDelegationActionAccounts(
                     action_dep_missing_delegation_record
                         .iter()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs` around lines 1330 -
1340, On error paths that return ChainlinkError::MissingDelegationActionAccounts
(when action_dep_missing_delegation_record is non-empty) and the sibling
not_found return, ensure you cancel any subscriptions created earlier before
returning: call the same cleanup used later (cancel_subs(...)) with the current
subscriptions built from action_dependencies_to_fetch and the
action_dep_record_subs returned by resolve_delegated_accounts(), merging them
into new_subs if necessary so all active subs are cancelled; place this
cancellation right before returning the error to avoid leaving orphaned watches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 1330-1340: On error paths that return
ChainlinkError::MissingDelegationActionAccounts (when
action_dep_missing_delegation_record is non-empty) and the sibling not_found
return, ensure you cancel any subscriptions created earlier before returning:
call the same cleanup used later (cancel_subs(...)) with the current
subscriptions built from action_dependencies_to_fetch and the
action_dep_record_subs returned by resolve_delegated_accounts(), merging them
into new_subs if necessary so all active subs are cancelled; place this
cancellation right before returning the error to avoid leaving orphaned watches.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e6359a36-7fa7-4903-9d1e-59c4967b0d1f

📥 Commits

Reviewing files that changed from the base of the PR and between 0f18d68 and 7476d1a.

📒 Files selected for processing (2)
  • magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs
  • magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant