refactor(cloner): make post-delegation clone mode exclusive - #1504
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:
📝 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
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
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)
test-integration/test-chainlink/src/ixtest_context.rs (1)
180-195: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEnforce the deadline around the balance wait.
get_balancereturns before the loop body checks the condition, so the 30s assertion can be skipped if the first balance ≥ 777 SOL arrives afterdeadline. Wrap the poll intokio::time::timeoutusing the remaining duration, and ensure the client timeout does not leave this request unbounded for this setup path.🤖 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 `@test-integration/test-chainlink/src/ixtest_context.rs` around lines 180 - 195, Update the balance-wait logic in the setup path around the existing get_balance loop so the 30s limit is enforced by tokio::time::timeout instead of only the loop-body assertion. Use the remaining duration from the deadline to bound the poll around rpc_client.get_balance for counter_auth.pubkey(), and preserve the same success condition once the balance reaches 777 * LAMPORTS_PER_SOL. Ensure the wait cannot run unbounded if the first visible balance arrives after the deadline.
🤖 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 `@test-integration/test-chainlink/src/ixtest_context.rs`:
- Around line 180-195: Update the balance-wait logic in the setup path around
the existing get_balance loop so the 30s limit is enforced by
tokio::time::timeout instead of only the loop-body assertion. Use the remaining
duration from the deadline to bound the poll around rpc_client.get_balance for
counter_auth.pubkey(), and preserve the same success condition once the balance
reaches 777 * LAMPORTS_PER_SOL. Ensure the wait cannot run unbounded if the
first visible balance arrives after the deadline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4c82689-95e5-444e-80bb-16da10e77344
📒 Files selected for processing (1)
test-integration/test-chainlink/src/ixtest_context.rs
3 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