Feat/disallow system program delegation by default

[GabrielePicco]

Problem

The normal delegation, if not checked, allows to delegate to the system program. This is a special case as these accounts are considered "confined" in the validator and cannot be undelegated.

Solution

• Disallow delegation to the system program by default
• Added a new instruction that allows to delegate to the system program

Summary by CodeRabbit

• New Features

  • Added a delegation option that allows accounts to be delegated to any validator identity, increasing flexibility.

• Bug Fixes

  • Standard delegation now blocks delegation to the system program and surfaces an explicit error when attempted.

• Tests

  • Added tests covering confined-validator delegation scenarios, including both rejection of system-program validators and allowance when using the new any-validator option.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/processor/fast/utils/pda.rs (1)

85-159: Add a clarifying comment about intentional aliasing of fees_vault.

When commit_fee_destination is the same as an element in fees_addresses (as in the undelegate flow where both are fees_vault), that account correctly receives both its regular fee distribution and the commit fee. Consider adding an inline comment at the commit fee transfer (around line 141) to explicitly document this intentional aliasing pattern.

🤖 Fix all issues with AI agents

In `@src/processor/fast/delegate.rs`:
- Line 11: The file imports solana_program::system_program while the rest of the
module uses pinocchio types; replace the mixed SDK usage by referencing
pinocchio_system::ID (or import pinocchio_system) for the id() comparison
instead of system_program::id(), update the import line that currently brings in
system_program and any id() calls that reference system_program to use
pinocchio_system::ID (or pinocchio_system::id()) so the module consistently uses
pinocchio types.

In `@tests/test_commit_fees_on_undelegation.rs`:
- Around line 98-123: The test duplicates construction of delegation record and
metadata data; extract and reuse the created data or parameters from the setup
helper so the test doesn't call get_delegation_record_data and
create_delegation_metadata_data_with_nonce twice. Modify the setup function that
calls program_test.add_account to return the delegation_record_data and
delegation_metadata_data (or at least the nonce/params used), or pull the nonce
and shared params into constants and reference them in both the setup and the
test; update references to get_delegation_record_data and
create_delegation_metadata_data_with_nonce in the test to use the
returned/shared values instead of reconstructing them.
- Around line 38-44: The test uses a hardcoded divisor (/ 100) for computing
expected_rent_fees; replace that with a calculation that applies the
RENT_FEES_PERCENTAGE constant (convert it to u64) so expected_rent_fees =
record_rent * u64::from(RENT_FEES_PERCENTAGE) / 100 + metadata_rent *
u64::from(RENT_FEES_PERCENTAGE) / 100, ensuring the test follows the
RENT_FEES_PERCENTAGE used elsewhere (see variables expected_rent_fees,
record_rent, metadata_rent).
- Around line 34-44: The magic number 100 used when computing
expected_commit_fee and expected_rent_fees should be extracted as a named
variable (e.g., commit_count) so the relation to the nonce
(create_delegation_metadata_data_with_nonce(..., 101)) is explicit; compute let
commit_count = 101 - 1 (or derive from the nonce parameter) and replace the
literal 100 in the formulas for expected_commit_fee and expected_rent_fees with
commit_count to improve clarity and avoid duplication.

In `@tests/test_delegation_confined_accounts.rs`:
- Around line 108-126: Add a new async test function (e.g.,
test_delegate_non_system_validator) that reuses setup_program_test_env to create
the BanksClient/payer/delegated, then registers a non-system-program validator
account (an Account with owner != system_program::id()), invoke the program's
delegate instruction (the same delegate instruction used elsewhere in tests)
against that non-system-program account, and assert the call succeeds and state
changes are as expected; reference setup_program_test_env, the delegate
instruction entrypoint, and the program id dlp::ID to locate where to hook the
test in.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between adddad7 and 80706b7.

📒 Files selected for processing (11)

• src/consts.rs
• src/discriminator.rs
• src/error.rs
• src/instruction_builder/delegate.rs
• src/lib.rs
• src/processor/fast/delegate.rs
• src/processor/fast/undelegate.rs
• src/processor/fast/utils/pda.rs
• tests/fixtures/accounts.rs
• tests/test_commit_fees_on_undelegation.rs
• tests/test_delegation_confined_accounts.rs

🧰 Additional context used

🧠 Learnings (2)

📚 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/delegate.rs
• src/processor/fast/undelegate.rs

📚 Learning: 2025-10-29T10:43:00.068Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/delegation-program PR: 90
File: tests/test_protocol_claim_fees.rs:30-30
Timestamp: 2025-10-29T10:43:00.068Z
Learning: In Solana test files using solana_program_test, use Rent::default() instead of Rent::get() because the Rent sysvar is not available in the test context. Rent::get() is only available in on-chain program execution.

Applied to files:

• src/processor/fast/delegate.rs

🧬 Code graph analysis (5)

tests/test_delegation_confined_accounts.rs (2)

src/pda.rs (1)

• delegation_record_pda_from_delegated_account (98-104)

src/instruction_builder/delegate.rs (2)

• delegate (16-29)
• delegate_with_any_validator (33-46)

tests/fixtures/accounts.rs (1)

src/state/delegation_metadata.rs (1)

• seeds (35-35)

src/lib.rs (1)

src/processor/fast/delegate.rs (1)

• process_delegate_with_any_validator (63-69)

src/instruction_builder/delegate.rs (6)

src/state/utils/discriminator.rs (1)

• discriminator (22-22)

src/state/delegation_metadata.rs (1)

• discriminator (24-26)

src/state/commit_record.rs (1)

• discriminator (30-32)

src/state/delegation_record.rs (1)

• discriminator (33-35)

src/state/program_config.rs (1)

• discriminator (15-17)

src/pda.rs (3)

• delegate_buffer_pda_from_delegated_account_and_owner_program (130-139)
• delegation_record_pda_from_delegated_account (98-104)
• delegation_metadata_pda_from_delegated_account (106-112)

src/processor/fast/undelegate.rs (1)

src/processor/fast/utils/pda.rs (1)

• close_pda_with_fees (85-160)

⏰ 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 (21)

src/consts.rs (1)

10-12: LGTM!

The commit fee constant is well-documented and appropriately placed alongside other fee-related constants.

src/error.rs (1)

134-136: LGTM!

The new error variant follows the existing pattern, has a sequential discriminant value, and provides a clear error message.

tests/fixtures/accounts.rs (1)

107-138: LGTM!

Clean refactoring that maintains backward compatibility while enabling tests to specify custom nonce values. The delegation pattern keeps the existing API stable.

src/processor/fast/undelegate.rs (4)

14-14: LGTM!

Import addition for the new COMMIT_FEE_LAMPORTS constant is correctly placed.

---

134-134: LGTM!

Extracting delegation_last_update_nonce from metadata to track commit count for fee calculation.

---

165-172: LGTM!

The delegation_last_update_nonce is correctly propagated to both cleanup paths (empty data fast-path and CPI path).

Also applies to: 221-228

---

336-377: Commit fee calculation and collection logic is correct.

The implementation properly:

1. Computes commit_count as nonce - 1 (first commit is free, per the constant's documentation)
2. Caps the fee to prevent draining more than the available rent reimbursement amount
3. Uses a mutable commit_fee that spans both PDA closures, allowing fee collection to continue across both accounts

One minor observation: the max_collectable formula uses (100 - RENT_FEES_PERCENTAGE) which represents the portion going to rent_reimbursement. This correctly ensures commit fees cannot exceed what would otherwise be returned to the rent payer.

src/lib.rs (1)

104-106: LGTM! Routing for new discriminator is consistent with existing patterns.

The new DelegateWithAnyValidator variant is correctly routed to process_delegate_with_any_validator. This follows the same pattern as the existing Delegate discriminator handling.

Note: This variant is only handled in fast_process_instruction, not in slow_process_instruction. The wildcard arm in slow_process_instruction will return InvalidInstructionData for this variant, which appears intentional since delegation operations are fast-path only.

src/discriminator.rs (1)

44-45: LGTM! New discriminator variant is well-defined.

The DelegateWithAnyValidator = 19 variant follows the sequential numbering convention and includes appropriate documentation linking to the processor function.

tests/test_delegation_confined_accounts.rs (2)

19-60: LGTM! Thorough negative test case with proper error verification.

The test correctly verifies that the default delegate instruction rejects system program as validator. The error matching is comprehensive, extracting and comparing the custom error code.

---

62-106: LGTM! Positive test case validates the new instruction path.

Good verification that delegate_with_any_validator succeeds with system program as validator and that the DelegationRecord.authority is correctly set.

src/processor/fast/delegate.rs (2)

54-69: LGTM! Clean refactoring with clear separation of concerns.

The delegation of both process_delegate and process_delegate_with_any_validator to a common inner function with a boolean flag is a clean approach. The function signatures remain simple and the intent is clear from the naming.

---

122-128: LGTM! System program validation is correctly implemented.

The validation logic:

1. Only runs when allow_system_program_validator is false
2. Correctly checks if the provided validator matches system_program::id()
3. Returns the appropriate error before any state changes occur

This placement after argument parsing but before PDA creation ensures early rejection with minimal compute cost.

src/instruction_builder/delegate.rs (2)

16-46: LGTM! Clean public API with good separation of concerns.

The two public functions delegate and delegate_with_any_validator provide clear entry points for each use case. The doc comments correctly reference their respective processor functions.

---

48-77: LGTM! Well-factored internal helper.

The build_delegate_instruction helper centralizes instruction construction and parameterizes the discriminator, making it easy to support multiple delegate variants without code duplication. The use of discriminator.to_vec() at line 61 correctly encodes the variant-specific discriminator.

tests/test_commit_fees_on_undelegation.rs (6)

1-20: LGTM!

Imports are well-organized and appropriate for the test setup and fee verification logic.

---

46-67: LGTM!

The transaction execution and fee verification logic is correct. The test properly validates that the protocol fees vault receives the expected fees after undelegation.

---

70-84: LGTM!

Program test initialization and validator account setup are correctly configured.

---

138-160: LGTM!

The protocol and validator fees vault accounts are correctly initialized with appropriate ownership and lamport balances.

---

162-163: LGTM!

The setup correctly starts the banks client and returns all necessary components for the test.

---

125-136: No action needed — the test program binary is already committed.

The file tests/buffers/test_delegation.so is tracked in the git repository and will be available in CI/CD. The relative path is correct for tests run from the repository root, which is standard practice.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@tests/test_delegation_confined_accounts.rs`:
- Around line 95-105: Replace the fragile chained .unwrap() calls with expect()
calls that provide clear context: when calling banks.get_account(...).await,
change the unwraps around the Option/Result to use expect() with a message like
"delegation record account missing for delegated pubkey" so missing account
failures are informative, and replace the unwrap() on
DelegationRecord::try_from_bytes_with_discriminator(&delegation_record_account.data)
with expect("failed to deserialize DelegationRecord for delegated pubkey") to
clarify deserialization errors; update the variables delegation_record_account
and delegation_record accordingly.

♻️ Duplicate comments (1)

tests/test_delegation_confined_accounts.rs (1)

108-126: Consider adding a test for non-system-program validators with the default delegate.

The setup helper is clean and reusable. As mentioned in a previous review, consider adding a test case that verifies the standard delegate instruction still works correctly with non-system-program validators to ensure the new validation doesn't inadvertently block legitimate use cases.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4cec66a and 54db5a5.

📒 Files selected for processing (6)

• src/discriminator.rs
• src/error.rs
• src/instruction_builder/delegate.rs
• src/lib.rs
• src/processor/fast/delegate.rs
• tests/test_delegation_confined_accounts.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/delegate.rs

🧬 Code graph analysis (3)

tests/test_delegation_confined_accounts.rs (2)

src/pda.rs (1)

• delegation_record_pda_from_delegated_account (98-104)

src/instruction_builder/delegate.rs (2)

• delegate (16-29)
• delegate_with_any_validator (33-46)

src/instruction_builder/delegate.rs (6)

src/state/delegation_metadata.rs (1)

• discriminator (24-26)

src/state/utils/discriminator.rs (1)

• discriminator (22-22)

src/state/commit_record.rs (1)

• discriminator (30-32)

src/state/delegation_record.rs (1)

• discriminator (33-35)

src/state/program_config.rs (1)

• discriminator (15-17)

src/pda.rs (3)

• delegate_buffer_pda_from_delegated_account_and_owner_program (130-139)
• delegation_record_pda_from_delegated_account (98-104)
• delegation_metadata_pda_from_delegated_account (106-112)

src/lib.rs (1)

src/processor/fast/delegate.rs (1)

• process_delegate_with_any_validator (62-68)

⏰ 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: test

🔇 Additional comments (8)

src/discriminator.rs (1)

44-45: LGTM!

The new discriminator variant DelegateWithAnyValidator = 19 is correctly added with sequential numbering and follows the established doc comment pattern referencing the corresponding processor function.

src/instruction_builder/delegate.rs (1)

16-77: LGTM! Clean refactoring to support both delegation variants.

The extraction of build_delegate_instruction helper eliminates code duplication while maintaining the same instruction structure. Both public functions correctly delegate to the helper with their respective discriminators (Delegate vs DelegateWithAnyValidator).

src/lib.rs (1)

104-106: LGTM!

The new discriminator routing correctly follows the established pattern, delegating to process_delegate_with_any_validator in the fast path. This is consistent with how the standard Delegate variant is handled.

src/error.rs (1)

134-135: LGTM!

The new error variant is correctly positioned with sequential numbering (42) and a clear, descriptive error message that accurately communicates the constraint being enforced.

src/processor/fast/delegate.rs (2)

53-68: LGTM! Clean separation of entry points.

The refactoring properly separates the two delegation modes via a boolean flag, maintaining the same signature for both public functions while routing to the shared implementation.

---

121-127: The None validator case is intentionally exempt and uses a trusted default constant.

The code correctly validates explicit validators against the system program restriction (lines 121-127), while the None case defaults to DEFAULT_VALIDATOR_IDENTITY (line 210)—a hardcoded trusted constant (MAS1Dt9qreoRMQ14YQuhg8UTZMMzDdKhmkZMECCzk57 in production) that is never the system program.

This asymmetry is intentional and acceptable: the default validator is trusted and doesn't require the system program check. Consider adding an inline comment (e.g., // Default validator is trusted and doesn't require system program validation) to make this design choice explicit.

tests/test_delegation_confined_accounts.rs (2)

1-17: LGTM!

Imports are clean and appropriate for the test functionality.

---

19-60: LGTM!

The test correctly validates that the standard delegate instruction rejects delegation to the system program. The error matching pattern properly extracts and verifies the custom error code.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@tests/test_delegation_confined_accounts.rs`:
- Around line 108-126: In setup_program_test_env replace the unwrap on
Keypair::from_bytes(&ON_CURVE_KEYPAIR).unwrap() with expect(...) so parsing
failures produce a clear message; specifically call
Keypair::from_bytes(&ON_CURVE_KEYPAIR).expect("failed to parse ON_CURVE_KEYPAIR
in setup_program_test_env") (or similar) to match the file's existing expect()
style and provide a helpful error string.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 54db5a5 and 449ec48.

📒 Files selected for processing (1)

• tests/test_delegation_confined_accounts.rs

⏰ 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: test

🔇 Additional comments (3)

tests/test_delegation_confined_accounts.rs (3)

1-17: LGTM!

Imports are well-organized and appropriate for the test functionality. The separation between Solana SDK imports and dlp crate imports is clear.

---

19-60: Well-structured test for the rejection path.

The test correctly verifies that the standard delegate instruction rejects the system program as a validator. The error matching pattern properly extracts and validates the custom error code.

---

62-106: Good coverage for the permissive delegation path.

The test properly verifies that delegate_with_any_validator allows the system program as validator and correctly records the delegation. The use of expect() with descriptive messages improves debuggability.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[snawaz]

Looks good to me! Let's merge!