feat: add an instruction which allows to undelegate confined accounts #122
Conversation
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/discriminator.rs(1 hunks)src/instruction_builder/mod.rs(2 hunks)src/instruction_builder/undelegate_confined_account.rs(1 hunks)src/lib.rs(1 hunks)src/processor/fast/mod.rs(2 hunks)src/processor/fast/undelegate.rs(1 hunks)src/processor/fast/undelegate_confined_account.rs(1 hunks)tests/test_undelegate_confined_account.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T11:45:25.802Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.
Applied to files:
src/processor/fast/mod.rs
🧬 Code graph analysis (5)
src/lib.rs (1)
src/processor/fast/undelegate_confined_account.rs (1)
process_undelegate_confined_account(32-171)
src/instruction_builder/mod.rs (1)
src/instruction_builder/undelegate_confined_account.rs (1)
undelegate_confined_account(14-40)
src/processor/fast/mod.rs (1)
src/instruction_builder/undelegate_confined_account.rs (1)
undelegate_confined_account(14-40)
tests/test_undelegate_confined_account.rs (3)
src/pda.rs (2)
delegation_metadata_pda_from_delegated_account(106-112)delegation_record_pda_from_delegated_account(98-104)tests/fixtures/accounts.rs (2)
create_delegation_record_data(67-84)get_delegation_metadata_data(99-105)src/instruction_builder/undelegate_confined_account.rs (1)
undelegate_confined_account(14-40)
src/instruction_builder/undelegate_confined_account.rs (1)
src/pda.rs (3)
delegation_metadata_pda_from_delegated_account(106-112)delegation_record_pda_from_delegated_account(98-104)undelegate_buffer_pda_from_delegated_account(141-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (10)
src/processor/fast/mod.rs (1)
8-8: LGTM!The new module declaration and re-export follow the established pattern for processor modules.
Also applies to: 18-18
src/lib.rs (1)
120-122: LGTM!The new discriminator case correctly routes to the fast processor handler, consistent with other fast-path instructions.
src/instruction_builder/mod.rs (1)
16-16: LGTM!Module declaration and re-export follow the established pattern and maintain alphabetical ordering.
Also applies to: 35-35
src/discriminator.rs (1)
42-43: LGTM!The new discriminator variant follows the established pattern with unique value and consistent documentation style.
src/processor/fast/undelegate.rs (1)
232-232: LGTM!Appropriate visibility change to
pub(crate)enables code reuse by the newprocess_undelegate_confined_accounthandler while keeping the function internal to the crate.src/instruction_builder/undelegate_confined_account.rs (1)
14-39: LGTM!The instruction builder correctly:
- Derives all required PDAs from the delegated account
- Sets appropriate mutability flags (writable for accounts being modified/closed)
- Maintains correct account ordering matching the processor expectation
- Uses the new discriminator for serialization
src/processor/fast/undelegate_confined_account.rs (4)
22-41: LGTM!The function signature and account unpacking follow standard patterns. Documentation clearly describes the required accounts and their roles.
43-44: LGTM!Admin signer validation is correctly implemented.
82-109: LGTM!The validation logic correctly enforces that:
- The delegated account is a valid PDA owned by the delegation program
- Delegation record and metadata are initialized
- The account is confined (authority == system program)
- The owner matches the stored owner in the delegation record
146-170: LGTM!The CPI processing and cleanup logic is correctly implemented:
- Reuses the existing
process_undelegation_with_cpihelper for consistency- Properly drops borrowed data references before closing PDAs to avoid memory safety issues
- Closes all PDAs in the correct order, returning lamports to the admin
The error handling is safe due to Solana's transaction atomicity - if any step fails, all state changes are reverted.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/processor/fast/utils/requires.rs:
- Around line 577-649: Update the SAFETY comment inside require_authorization to
remove the incorrect "presale program" wording and replace it with accurate
delegation program terminology and expected lifespan/context (e.g., "this
delegation program" and its actual deployment expectations), while keeping the
technical warning about reading raw ProgramData bytes and that this approach is
unsafe for long‑lived programs; ensure the comment still documents why the
unsafe bytes read is acceptable here and references Upgradeable Loader v3 layout
and pinocchio lacking UpgradeableLoaderState.
♻️ Duplicate comments (2)
src/processor/fast/undelegate_confined_account.rs (2)
79-88: Add safety comment for unsafe ownership reassignment.The
unsafeblock reassigns account ownership without documentation. As noted in a previous review, safety-criticalunsafeblocks require explicit justification.🔎 Suggested fix
// If the delegated account has no data, simply assign back to the original owner and clean up PDAs if delegated_account.data_is_empty() { + // SAFETY: Reassigning ownership is safe because: + // 1. Account is empty (no data to corrupt) - verified by data_is_empty() check above + // 2. Account is owned by this program (verified at line 51 via require_owned_pda) + // 3. Owner program is validated against delegation record (verified at lines 75-77) + // 4. Admin authorization ensures this is a privileged operation (verified at line 48) unsafe { delegated_account.assign(owner_program.key()); }
90-112: Consider adding size validation before data copy (defensive programming).The code copies delegated account data without explicit size validation. While Solana enforces a 10MB maximum account size at the system level and this is an admin-only operation, adding an explicit reasonable size limit would make the constraint clear and fail fast if a misconfigured account is encountered. This remains a minor defensive programming improvement as noted in the previous review.
🔎 Optional defensive validation
+ // Validate account size to prevent unexpected large allocations + const MAX_DELEGATED_ACCOUNT_SIZE: usize = 10_485_760; // 10MB (Solana's max) + if delegated_account.data_len() > MAX_DELEGATED_ACCOUNT_SIZE { + return Err(ProgramError::InvalidAccountData); + } + // Initialize undelegation buffer PDA and copy state let undelegate_buffer_bump: u8 = require_uninitialized_pda(
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/lib.rssrc/processor/fast/undelegate.rssrc/processor/fast/undelegate_confined_account.rssrc/processor/fast/utils/requires.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T11:45:25.802Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.
Applied to files:
Cargo.toml
🧬 Code graph analysis (3)
src/lib.rs (1)
src/processor/fast/undelegate_confined_account.rs (1)
process_undelegate_confined_account(33-139)
src/processor/fast/utils/requires.rs (2)
src/state/utils/discriminator.rs (1)
to_bytes(15-18)src/entrypoint.rs (1)
pinocchio(19-19)
src/processor/fast/undelegate_confined_account.rs (5)
src/entrypoint.rs (1)
pinocchio(19-19)src/state/delegation_metadata.rs (1)
seeds(35-35)src/processor/fast/utils/requires.rs (1)
require_authorization(577-649)src/processor/fast/undelegate.rs (1)
process_undelegation_with_cpi(234-290)src/processor/fast/mod.rs (1)
to_pinocchio_program_error(20-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (10)
src/processor/fast/utils/requires.rs (1)
12-19: LGTM! Constants properly gated for unit testing.The BPF loader and program data constants are correctly derived at compile time and gated with
cfg(not(feature = "unit_test_config")), allowing test configurations to bypass these checks.src/processor/fast/undelegate_confined_account.rs (3)
38-53: LGTM! Account validation and authorization checks are comprehensive.The function properly:
- Validates the expected 8 accounts
- Requires admin signature
- Calls
require_authorizationto verify admin is the program upgrade authority (which internally validatesdelegation_program_datamatches the expected ProgramData PDA)- Validates delegated account ownership and PDA derivations
56-77: LGTM! Delegation record loading and confined account validation are correct.The logic properly:
- Loads and deserializes delegation record and metadata
- Validates the authority is the system program (the "confined" constraint)
- Verifies the owner program matches the delegation record
114-138: LGTM! CPI delegation and cleanup logic are correct.The function properly:
- Reuses
process_undelegation_with_cpi(nowpub(crate)) with the same safety guarantees as the standard undelegate flow- Correctly drops borrowed data before closing PDAs
- Closes all three accounts (buffer, delegation record, metadata) to the admin
Cargo.toml (4)
21-29: LGTM! Dependencies properly added to support new authorization logic.The
programfeature now includes:
const-cryptofor compile-time PDA derivation (used inPROGRAM_DATA_ID)- Additional
pinocchiocrates for the fast path implementationThese additions align with the new
require_authorizationfunction and confined account undelegation feature.
41-53: LGTM! Optional dependencies correctly configured.The optional dependencies are properly aligned with the
programfeature requirements. The addition ofthiserrorprovides better error handling capabilities.
56-56: LGTM! Self dev-dependency enables unit test configuration.Adding
magicblock-delegation-programwithunit_test_configas a dev-dependency allows tests to use the relaxed authorization checks (DEFAULT_VALIDATOR_IDENTITY) instead of requiring actual program upgrade authority validation.
62-70: LGTM! Release profile optimized for Solana deployment.The profile settings are standard best practices for Solana programs:
opt-level = "z"andlto = "fat"minimize binary sizecodegen-units = 1enables maximum optimizationstrip = "symbols"removes debug symbolsoverflow-checks = falsereduces compute unitssrc/lib.rs (1)
118-120: LGTM! Discriminator routing correctly added.The new
UndelegateConfinedAccountcase is properly routed toprocessor::fast::process_undelegate_confined_account, consistent with the existing fast path instruction handling pattern.src/processor/fast/undelegate.rs (1)
234-234: LGTM! Visibility change enables code reuse.Changing
process_undelegation_with_cpitopub(crate)allows the newundelegate_confined_accountmodule to reuse this CPI logic while keeping it internal to the crate. This is a good example of reducing code duplication.
|
Warning Rate limit exceeded@GabrielePicco has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 21 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 (8)
WalkthroughAdds an admin-only UndelegateConfinedAccount instruction: new discriminator, instruction builder, fast-path processor implementation, authorization helper, visibility change for an undelegate helper, integration test, module re-exports, and Cargo manifest updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Co-authored-by: Sarfaraz Nawaz <sir_nawaz959@yahoo.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/processor/fast/undelegate_confined_account.rs:
- Around line 61-67: The code fails rustfmt due to long lines around
delegation_metadata parsing and the require_eq_keys! check; run cargo fmt to
reformat the file (or manually break long expressions) so lines with
delegation_metadata_data,
DelegationMetadata::try_from_bytes_with_discriminator(&delegation_metadata_data),
and the require_eq_keys!(delegation_record.authority, &pinocchio_system::ID,
DlpError::InvalidAuthority) invocation are wrapped to comply with rustfmt line
length limits; ensure indentation remains consistent and then commit the
formatted changes.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/processor/fast/undelegate_confined_account.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T11:45:25.802Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.
Applied to files:
src/processor/fast/undelegate_confined_account.rs
🧬 Code graph analysis (1)
src/processor/fast/undelegate_confined_account.rs (5)
src/entrypoint.rs (1)
pinocchio(19-19)src/state/delegation_metadata.rs (1)
seeds(35-35)src/processor/fast/utils/requires.rs (6)
require_authorization(577-644)require_initialized_delegation_metadata(435-448)require_initialized_delegation_record(418-431)require_owned_pda(175-186)require_signer(191-199)require_uninitialized_pda(280-297)src/processor/fast/undelegate.rs (1)
process_undelegation_with_cpi(234-290)src/processor/fast/mod.rs (1)
to_pinocchio_program_error(20-24)
🪛 GitHub Actions: Run Tests
src/processor/fast/undelegate_confined_account.rs
[error] 64-70: rustfmt formatting check failed. Run 'cargo fmt' to fix code style issues in this file.
🔇 Additional comments (2)
src/processor/fast/undelegate_confined_account.rs (2)
109-133: LGTM! Clean CPI integration and proper resource cleanup.The undelegation flow correctly:
- Reuses the existing CPI helper with matching signer seeds
- Drops all data borrows before closing PDAs
- Cleans up all three PDAs (buffer, record, metadata) in the proper order
106-107: Consider adding size validation before copying account data.While Solana enforces a 10MB account size limit at the system level, adding an explicit check would make the constraint visible and cause faster failure if a misconfigured account is encountered.
🔎 Optional defensive validation
+ // Validate account size to prevent unexpected large allocations + const MAX_DELEGATED_ACCOUNT_SIZE: usize = 1_048_576; // 1MB + if delegated_account.data_len() > MAX_DELEGATED_ACCOUNT_SIZE { + return Err(ProgramError::InvalidAccountData); + } + (*undelegate_buffer_account.try_borrow_mut_data()?) .copy_from_slice(&delegated_account.try_borrow_data()?);This is defensive programming rather than a critical issue, since this is an admin-only operation and similar operations throughout the codebase follow the same pattern. This was previously noted in past reviews.
⛔ Skipped due to learnings
Learnt from: snawaz Repo: magicblock-labs/delegation-program PR: 111 File: src/diff/types.rs:87-102 Timestamp: 2025-10-28T18:27:42.542Z Learning: In Solana runtime, `account_info.data()` is guaranteed to have 8-byte alignment.
GabrielePicco
deleted the
feat/add-option-to-undelegate-confined-account
branch
Description
Summary by CodeRabbit
New Features
Security / Reliability
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.