refactor(cloner): make post-delegation clone mode exclusive - #1504
refactor(cloner): make post-delegation clone mode exclusive#1504snawaz wants to merge 4 commits into
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
98e5168 to
325057f
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
| 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
There was a problem hiding this comment.
Not sure if I understood.
the field .needs_undelegation does not exist anymore, so wherever it was set to true, this PR has already replaced that with:
request.post_delegation_mode = ClonePostDelegationMode::RescueUndelegate;Hope that is what you meant.
There was a problem hiding this comment.
Sorry it's unclear I didn't know where to put the comment, but this PR only applies the logic to build_small_account_tx not build_large_account_txs
There was a problem hiding this comment.
ah, I see.
Github's diffview is confusing to me as well. It hides the function name, which is a critical piece of information/context when reviewing changes.
Anyway for build_large_account_txs, the changes are on line 339-356.
Github is weird! 😆
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-account-cloner/src/lib.rs (1)
653-654: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle rescue mode in the size fallback.
RescueUndelegateaddsschedule_undelegation_ixon Line 279-280.has_actions()does not cover this mode. If the small transaction exceedsMAX_INLINE_TRANSACTION_SIZE, this condition remains false andsend_txsubmits the oversized transaction instead of usingbuild_large_account_txs.Use a predicate that returns true for every mode that adds post-delegation instructions. Add a boundary test for an oversized small rescue transaction.
Proposed fix
if tx_size > MAX_INLINE_TRANSACTION_SIZE - && request.post_delegation_mode.has_actions() + && request.post_delegation_mode.requires_post_delegation()
requires_post_delegation()should return true forRescueUndelegateand non-emptyExecuteActions.🤖 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-account-cloner/src/lib.rs` around lines 653 - 654, Update the size-fallback condition in the transaction-building flow to use a predicate covering every mode that adds post-delegation instructions, including RescueUndelegate and non-empty ExecuteActions, instead of request.post_delegation_mode.has_actions(). Ensure oversized small rescue transactions use build_large_account_txs rather than send_tx, and add a boundary test covering that case.
🤖 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-account-cloner/src/lib.rs`:
- Around line 653-654: Update the size-fallback condition in the
transaction-building flow to use a predicate covering every mode that adds
post-delegation instructions, including RescueUndelegate and non-empty
ExecuteActions, instead of request.post_delegation_mode.has_actions(). Ensure
oversized small rescue transactions use build_large_account_txs rather than
send_tx, and add a boundary test covering that case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea06c94f-59c5-441d-b8af-94c7a4370259
📒 Files selected for processing (2)
magicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.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 (1)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
981-1000: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
active_delegation_satisfies_requestexpression.
local_account_satisfies_clone_request(Line 981-988) andclone_account_with_ownership(Line 1179-1184) compute the identical boolean expression frompost_delegation_mode.has_actions(),delegated_to_other, andpost_delegation_mode.is_rescue_undelegate(). Extract a shared helper. This removes the risk that a future edit updates one copy and not the other.♻️ Proposed helper extraction
+ fn active_delegation_satisfies_request( + request: &AccountCloneRequest, + ) -> bool { + !request.post_delegation_mode.has_actions() + && request.delegated_to_other.is_none() + && !request.post_delegation_mode.is_rescue_undelegate() + } + fn local_account_satisfies_clone_request( &self, request: &AccountCloneRequest, ) -> bool { - 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 = + Self::active_delegation_satisfies_request(request);- 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 = + Self::active_delegation_satisfies_request( + &owned_request, + );Also applies to: 1179-1184
🤖 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 981 - 1000, Extract the shared boolean logic for `active_delegation_satisfies_request` from `local_account_satisfies_clone_request` and `clone_account_with_ownership` into a helper that accepts the relevant request state. Replace both inline expressions with calls to that helper, preserving the existing behavior and avoiding duplicate calculations.
🤖 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-account-cloner/src/lib.rs`:
- Around line 270-283: Extract the duplicated post-delegation handling from
build_small_account_tx and build_large_account_txs into a shared helper that
matches request.post_delegation_mode once and returns the derived actions,
needs_undelegation flag, and sibling instruction. Update both callers to consume
the helper’s results while preserving their existing transaction behavior,
including empty ExecuteActions and None cases.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 981-1000: Extract the shared boolean logic for
`active_delegation_satisfies_request` from
`local_account_satisfies_clone_request` and `clone_account_with_ownership` into
a helper that accepts the relevant request state. Replace both inline
expressions with calls to that helper, preserving the existing behavior and
avoiding duplicate calculations.
🪄 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 Plus
Run ID: d7bbcc2b-a2e1-4eed-838a-5baeb06e438e
📒 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
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.Summary by CodeRabbit
New Features
Bug Fixes
Tests