refactor(cloner): make post-delegation clone mode exclusive - #1504
Conversation
📝 WalkthroughWalkthroughIntroduces 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.
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/cloner/mod.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "plain mode" check derived from two negated predicates instead of a direct accessor.
ClonePostDelegationModeexposeshas_actions()andis_rescue_undelegate()but no direct check for theNonevariant, so both consumer sites reconstruct "is plain/None mode" as!has_actions() && !is_rescue_undelegate(). This duplicates the same 3-term boolean twice verbatim and implicitly relies on the invariant thatExecuteActionsnever wraps empty actions (only guaranteed by theFrom<DelegationActions>constructor, not by the type itself, since the variant is public).
magicblock-chainlink/src/cloner/mod.rs#L61-80: addpub fn is_none(&self) -> bool { matches!(self, Self::None) }toClonePostDelegationModefor a direct, invariant-independent check.magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs#L863-865: replace!request.post_delegation_mode.has_actions() && ... && !request.post_delegation_mode.is_rescue_undelegate()withrequest.post_delegation_mode.is_none() && request.delegated_to_other.is_none().magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs#L1057-1061: apply the same simplification toowned_request.♻️ Proposed fix
impl ClonePostDelegationMode { + pub fn is_none(&self) -> bool { + matches!(self, Self::None) + } + pub fn execute_actions(&self) -> Option<&DelegationActions> {- let active_delegation_satisfies_request = - !request.post_delegation_mode.has_actions() - && request.delegated_to_other.is_none() - && !request.post_delegation_mode.is_rescue_undelegate(); + let active_delegation_satisfies_request = request + .post_delegation_mode + .is_none() + && request.delegated_to_other.is_none();- let active_delegation_satisfies_request = - !owned_request.post_delegation_mode.has_actions() - && owned_request.delegated_to_other.is_none() - && !owned_request - .post_delegation_mode - .is_rescue_undelegate(); + let active_delegation_satisfies_request = owned_request + .post_delegation_mode + .is_none() + && owned_request.delegated_to_other.is_none();🤖 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/cloner/mod.rs` at line 1, Add an `is_none()` accessor to `ClonePostDelegationMode` using a direct `None` variant match, then update both plain-mode checks in the fetch-cloner request and owned-request paths to use `post_delegation_mode.is_none()` alongside `delegated_to_other.is_none()`.
🤖 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/cloner/mod.rs`:
- Line 1: Add an `is_none()` accessor to `ClonePostDelegationMode` using a
direct `None` variant match, then update both plain-mode checks in the
fetch-cloner request and owned-request paths to use
`post_delegation_mode.is_none()` alongside `delegated_to_other.is_none()`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76b6b7d0-1316-4ec5-9db7-d9fae0c956b2
📒 Files selected for processing (9)
magicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/cloner/mod.rsmagicblock-chainlink/src/testing/cloner_stub.rstest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/tests/ix_aml_undelegation.rs
8c0cf5e to
98e5168
Compare
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)
890-909: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated
active_delegation_satisfies_requestpredicate.The exact same three-condition check (
!post_delegation_mode.has_actions() && delegated_to_other.is_none() && !post_delegation_mode.is_rescue_undelegate()) is computed independently inlocal_account_satisfies_clone_requestandclone_account_with_ownership. Both migrations were done correctly, but keeping the logic in one place avoids a future edit silently diverging between the pre-check and the post-failure reconciliation path.♻️ Suggested consolidation
+impl AccountCloneRequest { + /// True when an actively delegated local copy already satisfies this + /// clone request (no pending actions, not delegated elsewhere, no + /// rescue-undelegate pending). + fn active_delegation_satisfies_request(&self) -> bool { + !self.post_delegation_mode.has_actions() + && self.delegated_to_other.is_none() + && !self.post_delegation_mode.is_rescue_undelegate() + } +}Then in
local_account_satisfies_clone_request:- let active_delegation_satisfies_request = - !request.post_delegation_mode.has_actions() - && request.delegated_to_other.is_none() - && !request.post_delegation_mode.is_rescue_undelegate(); + let active_delegation_satisfies_request = + request.active_delegation_satisfies_request();And in
clone_account_with_ownership:- let active_delegation_satisfies_request = - !owned_request.post_delegation_mode.has_actions() - && owned_request.delegated_to_other.is_none() - && !owned_request - .post_delegation_mode - .is_rescue_undelegate(); + let active_delegation_satisfies_request = + owned_request.active_delegation_satisfies_request();Also applies to: 1088-1093
🤖 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 890 - 909, Extract the duplicated three-condition active-delegation predicate into a shared helper near the existing clone-request logic, then update both local_account_satisfies_clone_request and clone_account_with_ownership to call it. Preserve the current boolean behavior in both pre-check and post-failure reconciliation paths.
🤖 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 890-909: Extract the duplicated three-condition active-delegation
predicate into a shared helper near the existing clone-request logic, then
update both local_account_satisfies_clone_request and
clone_account_with_ownership to call it. Preserve the current boolean behavior
in both pre-check and post-failure reconciliation paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab43e07f-a018-417f-98e9-08eb8a7742f2
📒 Files selected for processing (9)
magicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/cloner/mod.rsmagicblock-chainlink/src/testing/cloner_stub.rstest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/tests/ix_aml_undelegation.rs
| vec![], | ||
| )]); | ||
| request.post_delegation_mode = | ||
| ClonePostDelegationMode::from(DelegationActions::from(vec
There was a problem hiding this comment.
nit: there could be a impl From<Vec<Instruction>> for ClonePostDelegationMode
| ixs.push(Self::schedule_undelegation_ix(request.pubkey)); | ||
| } else if !actions.is_empty() { | ||
| ixs.push(Self::post_delegation_action_ix(request.pubkey, actions)); | ||
| match &request.post_delegation_mode { |
There was a problem hiding this comment.
nit: this and the change above could be merged in a single match
| fn small_undelegation_clone_schedules_in_same_tx_without_actions() { | ||
| let pubkey = Pubkey::new_unique(); | ||
| let mut request = request(pubkey, vec![1, 2, 3], vec![action()]); | ||
| request.needs_undelegation = true; |
There was a problem hiding this comment.
These should also be applied to large accounts
2 participants
This PR replaces product-state with sum-type (enum) to make the exclusivity semantic obvious. See
enum ClonePostDelegationModethat replaces the two-fields with one field.