chore: add rustfmt.toml with some sensible format settings

[snawaz]

This PR primary adds rustfmt.toml and a Makefile.

Other changes are due to make fmt. That's it.

Summary by CodeRabbit

Release Notes

• Chores

  • Added Makefile and code formatting configuration for build and development tooling.

• Refactor

  • Standardized code formatting and import organization across the codebase for improved maintainability.
  • Refined function signatures and structural organization for consistency.

• Deprecation

  • Marked call_handler function as deprecated; use call_handler_v2 instead.

[snawaz]

• feat: Impl V2 Commit #142
• feat: Impl V2 CommitFinalize #141
• feat: Impl V2 Delegate #140
• chore: add rustfmt.toml with some sensible format settings #139 👈 (View in Graphite)
• chore: upgrade pinocchio #138
• feat: add CommitFinalizeFromBuffer #137
• feat: add CommitFinalize #131
• feat: add AccountSizeClass and compute account-data sizes for 8 instructions #130
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[GabrielePicco]

LGTM!

[coderabbitai]

Actionable comments posted: 12

Caution

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

⚠️ Outside diff range comments (17)

src/processor/init_protocol_fees_vault.rs (1)

25-25: ⚠️ Potential issue | 🟡 Minor

Typo in doc comment: "permisionless" → "permissionless"

✏️ Proposed fix

-/// NOTE: this operation is permisionless and can be done by anyone
+/// NOTE: this operation is permissionless and can be done by anyone

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/init_protocol_fees_vault.rs` at line 25, Doc comment in
init_protocol_fees_vault.rs contains a typo: change the word "permisionless" to
"permissionless" in the comment above the operation (the doc line starting with
"NOTE: this operation is ...") so the comment reads "permissionless"; make the
edit where the doc string or inline comment is defined (the NOTE comment) to
correct the spelling.

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

90-105: ⚠️ Potential issue | 🟠 Major

Rent::default() in on-chain processor code should be Rent::get()?.

resize_pda computes new_minimum_balance using Rent::default(), which returns a hardcoded struct rather than reading the live sysvar. If the network's rent parameters differ from the compile-time defaults, the lamports top-up will be incorrect and the resized PDA may not be rent-exempt. Note that create_pda in the same file correctly uses Rent::get()?.

🐛 Proposed fix

 pub(crate) fn resize_pda<'a, 'info>(
     payer: &'a AccountInfo<'info>,
     pda: &'a AccountInfo<'info>,
     system_program: &'a AccountInfo<'info>,
     new_size: usize,
 ) -> Result<(), ProgramError> {
-    let new_minimum_balance = Rent::default().minimum_balance(new_size);
+    let new_minimum_balance = Rent::get()?.minimum_balance(new_size);

Based on learnings: in Solana on-chain program execution, Rent::get() is the correct API for reading the sysvar; Rent::default() is only safe in test contexts where the sysvar is unavailable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/utils/pda.rs` around lines 90 - 105, The resize_pda function is
using Rent::default() which reads hardcoded defaults; change it to read the
on-chain rent sysvar by calling Rent::get()? and use its
minimum_balance(new_size) (i.e., replace
Rent::default().minimum_balance(new_size) with
Rent::get()?.minimum_balance(new_size)) so new_minimum_balance is computed from
the live sysvar; ensure the function's Result propagation still returns Err if
Rent::get() fails (the existing signature returning Result<(), ProgramError> is
fine).

src/processor/top_up_ephemeral_balance.rs (1)

48-48: ⚠️ Potential issue | 🟡 Minor

Add explicit writability validation for payer.

The payer account is declared as [writable] in the doc comment (line 21) but is only validated as a signer via load_signer(payer, "payer")?; at line 48. No explicit writability check is performed. For consistency with how ephemeral_balance_account is validated via load_pda(..., true, ...), add an explicit check to ensure payer is writable before the CPI transfer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/top_up_ephemeral_balance.rs` at line 48, The payer is only
validated as a signer via load_signer(payer, "payer") but not checked for
writability; add an explicit writability check before the CPI transfer similar
to how ephemeral_balance_account is validated with load_pda(..., true, ...).
Concretely, after load_signer(payer, "payer")?; assert payer.is_writable (or
call the existing assert_writable/ensure_writable helper if one exists) and
return a descriptive error (e.g., "payer must be writable") if not, so the payer
account is guaranteed writable prior to the transfer.

src/processor/validator_claim_fees.rs (2)

53-59: ⚠️ Potential issue | 🔴 Critical

Two issues: unchecked u64 underflow can bypass the solvency guard; Rent::default() should be Rent::get().

1. Unchecked u64 underflow (critical). At lines 56 and 59, validator_fees_vault.lamports() - min_rent is a bare u64 subtraction. In release mode without overflow-checks = true, u64 underflow silently wraps to values like 18446744073709551615 when lamports < min_rent. Since both line 56 and line 59 evaluate the same wrapping expression, the guard at line 59 becomes huge_value < huge_value → false, letting execution continue with a garbage amount. Although checked_sub at line 82 ultimately saves from an actual lamport drain in the None branch, in the Some(explicit_amount) branch where amount ≤ lamports, checked_sub succeeds and the vault is drained below min_rent. The fix is to replace bare - with checked_sub; the cost of these checks is negligible on Solana.

2. Rent::default() (major). The Rent sysvar implements Sysvar::get and can be loaded efficiently without passing the sysvar account ID to the program. Using Rent::default() hard-codes the struct's compile-time constants instead of reading the live sysvar.

🛡️ Proposed fix

-    let min_rent = Rent::default().minimum_balance(8);
-    let amount = args
-        .amount
-        .unwrap_or(validator_fees_vault.lamports() - min_rent);
+    let min_rent = Rent::get()?.minimum_balance(8);
+    let available = validator_fees_vault
+        .lamports()
+        .checked_sub(min_rent)
+        .ok_or(ProgramError::InsufficientFunds)?;
+    let amount = args.amount.unwrap_or(available);

     // Ensure vault has enough lamports
-    if validator_fees_vault.lamports() - min_rent < amount {
+    if available < amount {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/validator_claim_fees.rs` around lines 53 - 59, Replace the
unchecked subtraction and default Rent usage: load the live rent via Rent::get()
(not Rent::default()) to compute min_rent, and replace all bare uses of
validator_fees_vault.lamports() - min_rent with checked_sub calls (e.g.,
validator_fees_vault.lamports().checked_sub(min_rent)) and properly handle the
Option return to early-return/error when lamports < min_rent so the solvency
guard before assigning amount (and in the args.amount Some branch) cannot be
bypassed; update the logic around args.amount.unwrap_or(...) and later
checked_sub usage to use the same checked result and fail fast on None.

---

70-71: ⚠️ Potential issue | 🔴 Critical

Unchecked multiplication before division can overflow.

amount * u64::from(PROTOCOL_FEES_PERCENTAGE) is the only unchecked arithmetic in this function; everything else uses checked_* or saturating_*. Integer overflow in a multiplication like num * size can occur silently in release builds, as seen in the Solana BPF loader itself. An overflowed intermediate wraps to a small value, causing protocol_fees to be underreported, and remaining_amount to exceed amount, breaking fee invariants.

🛡️ Proposed fix

-    let protocol_fees = (amount * u64::from(PROTOCOL_FEES_PERCENTAGE)) / 100;
+    let protocol_fees = amount
+        .checked_mul(u64::from(PROTOCOL_FEES_PERCENTAGE))
+        .ok_or(DlpError::Overflow)?
+        / 100;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/validator_claim_fees.rs` around lines 70 - 71, The
multiplication of amount by PROTOCOL_FEES_PERCENTAGE can overflow; replace the
unchecked multiply/divide with a checked/widened arithmetic path: compute
protocol_fees by either using
amount.checked_mul(u64::from(PROTOCOL_FEES_PERCENTAGE)).and_then(|v|
v.checked_div(100)) and handle the None case (return an error) or cast amount
and PROTOCOL_FEES_PERCENTAGE to a wider integer (e.g., u128), perform
(amount_u128 * percent_u128) / 100, then try_into() back to u64 and handle
conversion failure; update the calculation for protocol_fees and then compute
remaining_amount via checked_sub or saturating_sub using the validated
protocol_fees variable (referencing the variables protocol_fees,
remaining_amount, amount and the constant PROTOCOL_FEES_PERCENTAGE).

src/instruction_builder/call_handler.rs (1)

59-71: 🧹 Nitpick | 🔵 Trivial

Clarify that other_accounts represents a size budget, not a count.

The parameter name other_accounts: u32 at Line 61 is added directly to the total budget at Line 70 (+ other_accounts), so it represents a cumulative size budget for the remaining accounts, not a count. The name could mislead callers into passing an account count. Consider renaming to other_accounts_size_budget for clarity.

✏️ Suggested rename for clarity

 pub fn call_handler_size_budget(
     destination_program: AccountSizeClass,
-    other_accounts: u32,
+    other_accounts_size_budget: u32,
 ) -> u32 {
     total_size_budget(&[
         DLP_PROGRAM_DATA_SIZE_CLASS,
         AccountSizeClass::Tiny, // validator
         AccountSizeClass::Tiny, // validator_fees_vault_pda
         destination_program,
         AccountSizeClass::Tiny, // escrow_authority
         AccountSizeClass::Tiny, // escrow_account
-    ]) + other_accounts
+    ]) + other_accounts_size_budget
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instruction_builder/call_handler.rs` around lines 59 - 71, The parameter
other_accounts in call_handler_size_budget is misleading because its value is
added as a size budget, not a count; rename it to other_accounts_size_budget in
the function signature and all call sites, update any docs/tests to use the new
name, and keep the logic unchanged (i.e., return total_size_budget(...) +
other_accounts_size_budget) so callers clearly pass a size budget rather than a
count.

src/processor/whitelist_validator_for_program.rs (1)

20-30: ⚠️ Potential issue | 🟡 Minor

Doc comment is stale — missing delegation_program_data account.

The accounts list in the doc comment enumerates 6 accounts (indices 0–5), but the destructuring at Line 50 expects 7 accounts, including delegation_program_data between program_data and program_config_account. The instruction builder in src/instruction_builder/whitelist_validator_for_program.rs (Line 38) also passes delegation_program_data at index 4. Update the doc to reflect the actual account layout.

📝 Proposed doc fix

 /// Accounts:
 ///
 /// 0: `[signer]`   authority that has rights to whitelist validators
 /// 1: `[]`         validator identity to whitelist
 /// 2: `[]`         program to whitelist the validator for
 /// 3: `[]`         program data account
-/// 4: `[writable]` program config PDA
-/// 5: `[]`         system program
+/// 4: `[]`         delegation program data account
+/// 5: `[writable]` program config PDA
+/// 6: `[]`         system program

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/whitelist_validator_for_program.rs` around lines 20 - 30, The
doc comment for the Whitelist a validator for a program handler in
src/processor/whitelist_validator_for_program.rs is stale and must include the
missing delegation_program_data account; update the accounts list to reflect
seven accounts (add a new entry for `delegation_program_data` between
`program_data` and `program_config_account`) so it matches the account
destructuring in the handler and the instruction builder in
src/instruction_builder/whitelist_validator_for_program.rs which passes
`delegation_program_data`; ensure the numbering and brief descriptions align
with the existing entries (0: authority, 1: validator identity, 2: program, 3:
program_data, 4: delegation_program_data, 5: program_config_account, 6: system
program).

src/processor/fast/internal/commit_finalize_internal.rs (1)

20-21: ⚠️ Potential issue | 🟡 Minor

Stale doc comment: says "commit state" instead of "commit finalize".

📝 Proposed fix

-/// Arguments for the commit state internal function
+/// Arguments for the commit finalize internal function
 pub(crate) struct CommitFinalizeInternalArgs<'a> {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/fast/internal/commit_finalize_internal.rs` around lines 20 -
21, The doc comment for the struct CommitFinalizeInternalArgs<'a> is stale (says
"commit state"); update the comment to accurately describe the struct as
arguments for the "commit finalize" internal function (e.g., change "Arguments
for the commit state internal function" to "Arguments for the commit finalize
internal function") so the documentation matches the symbol
CommitFinalizeInternalArgs and its purpose.

src/instruction_builder/commit_finalize_from_buffer.rs (1)

73-92: ⚠️ Potential issue | 🟡 Minor

Two issues in commit_finalize_from_buffer_size_budget: stale doc comment and spurious program_config_pda entry.

1. Stale doc comment (Line 74): references commit_diff_from_buffer instead of commit_finalize_from_buffer.
2. Extra account entry (Line 89): program_config_pda is included in the budget but is absent from the actual instruction accounts. The commit_finalize_from_buffer instruction only loads 7 accounts (validator, delegated_account, delegation_record, delegation_metadata, data_buffer, validator_fees_vault, system_program), as confirmed by require_n_accounts!(accounts, 7) in the processor. This over-reports the size budget by one AccountSizeClass::Tiny entry, giving callers a higher-than-necessary SetLoadedAccountsDataSizeLimit value.

📝 Proposed fix

-///
-/// Returns accounts-data-size budget for commit_diff_from_buffer instruction.
-///
+///
+/// Returns accounts-data-size budget for commit_finalize_from_buffer instruction.
+///
 /// This value can be used with ComputeBudgetInstruction::SetLoadedAccountsDataSizeLimit
 ///
 pub fn commit_finalize_from_buffer_size_budget(
     delegated_account: AccountSizeClass,
 ) -> u32 {
     total_size_budget(&[
         DLP_PROGRAM_DATA_SIZE_CLASS,
         AccountSizeClass::Tiny, // validator
         delegated_account,      // delegated_account
         AccountSizeClass::Tiny, // delegation_record_pda
         AccountSizeClass::Tiny, // delegation_metadata_pda
         delegated_account,      // data_buffer
         AccountSizeClass::Tiny, // validator_fees_vault_pda
-        AccountSizeClass::Tiny, // program_config_pda
         AccountSizeClass::Tiny, // system_program
     ])
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instruction_builder/commit_finalize_from_buffer.rs` around lines 73 - 92,
Update the doc comment on commit_finalize_from_buffer_size_budget to reference
commit_finalize_from_buffer (not commit_diff_from_buffer), and remove the
spurious AccountSizeClass::Tiny entry for program_config_pda from the
total_size_budget array so the returned account-data-size budget matches the
actual accounts the processor expects (validator, delegated_account,
delegation_record_pda, delegation_metadata_pda, data_buffer,
validator_fees_vault_pda, system_program). Ensure the remaining array still
includes DLP_PROGRAM_DATA_SIZE_CLASS and the two delegated_account entries as
currently used.

src/processor/init_validator_fees_vault.rs (1)

22-26: ⚠️ Potential issue | 🟡 Minor

Doc comment account list is incomplete — delegation_program_data missing, indices 2–4 are off by one.

The code at line 43 destructures six accounts (payer, admin, delegation_program_data, validator_identity, validator_fees_vault, system_program), but the doc only lists five and maps the wrong pubkeys to indices 2–4.

📝 Proposed fix

 /// Accounts:
 ///
 /// 0; `[signer]` payer
 /// 1; `[signer]` magicblock admin that controls the vault
-/// 2; `[]`       validator_identity
-/// 3; `[]`       validator_fees_vault_pda
-/// 4; `[]`       system_program
+/// 2; `[]`       delegation_program_data
+/// 3; `[]`       validator_identity
+/// 4; `[]`       validator_fees_vault_pda
+/// 5; `[]`       system_program

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/init_validator_fees_vault.rs` around lines 22 - 26, The account
doc comment above the init_validator_fees_vault handler is wrong: it omits
delegation_program_data and mis-indexes accounts 2–4; update the comment to list
six accounts matching the destructured variables (payer, admin,
delegation_program_data, validator_identity, validator_fees_vault_pda,
system_program) and correct the index numbers so each label aligns with the
corresponding variable used in the function (e.g., index 2 =>
delegation_program_data, 3 => validator_identity, 4 => validator_fees_vault_pda,
5 => system_program).

src/processor/close_validator_fees_vault.rs (1)

19-24: ⚠️ Potential issue | 🟡 Minor

Doc comment has two inaccuracies that can mislead callers building the instruction manually.

1. Missing account: delegation_program_data is destructured at line 39 between admin and validator_identity, but it is absent from the account list — leaving indices 2–3 pointing to the wrong accounts.
2. Wrong mutability label: validator_identity is listed as [] (non-writable), but close_pda transfers the vault lamports directly into it; the Solana runtime will reject the transaction if this account is not marked writable.

📝 Proposed fix

 /// Accounts:
 ///
 /// 0; `[signer]`   payer
 /// 1; `[signer]`   admin that controls the vault
-/// 2; `[]`         validator_identity
-/// 3; `[]`         validator_fees_vault_pda
+/// 2; `[]`         delegation_program_data
+/// 3; `[writable]` validator_identity
+/// 4; `[writable]` validator_fees_vault_pda

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/close_validator_fees_vault.rs` around lines 19 - 24, Update the
doc comment for the close_validator_fees_vault instruction to (1) insert the
missing delegation_program_data account between admin and validator_identity so
the account indices align with the destructuring in the function (reference the
symbol delegation_program_data in the comment) and (2) change the
validator_identity account entry from non-writable (`[]`) to writable
(`[writable]` or `[mut]`) because close_pda transfers lamports into
validator_identity; ensure validator_fees_vault_pda remains listed after
validator_identity and that the doc comment matches the actual parameter order
used by the close_validator_fees_vault logic.

tests/test_commit_finalize.rs (1)

75-75: ⚠️ Potential issue | 🟡 Minor

Typo: "performmace" → "performance"

📝 Fix

-    // execute CommitFinalize and validate CU performmace
+    // execute CommitFinalize and validate CU performance

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_commit_finalize.rs` at line 75, Update the comment in
tests/test_commit_finalize.rs that reads "// execute CommitFinalize and validate
CU performmace" to correct the typo by changing "performmace" to "performance"
(the test symbol is test_commit_finalize/CommitFinalize comment); simply edit
the inline comment text to "// execute CommitFinalize and validate CU
performance".

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

221-231: ⚠️ Potential issue | 🟡 Minor

Unchecked lamport addition is inconsistent with checked_sub usage above

checked_sub is used when computing transfer_lamports and when deducting from the source (lines 222-226), but the credit to the destination at line 229 uses plain +. While the total SOL supply guarantees this can't overflow in practice, the asymmetry is a defensive-coding inconsistency.

🛡️ Proposed fix

-    transfer_destination
-        .set_lamports(transfer_destination.lamports() + transfer_lamports);
+    transfer_destination.set_lamports(
+        transfer_destination
+            .lamports()
+            .checked_add(transfer_lamports)
+            .ok_or(DlpError::Overflow)?,
+    );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/fast/finalize.rs` around lines 221 - 231, The credit to
transfer_destination currently uses plain addition which is inconsistent with
the checked subtraction above; update the code that sets transfer_destination's
lamports in finalize.rs (symbols: transfer_destination, set_lamports,
lamports(), transfer_lamports) to use a checked_add and propagate
DlpError::Overflow on overflow (mirror the pattern used for transfer_source
where checked_sub is used), ensuring you call
.checked_add(transfer_lamports).ok_or(DlpError::Overflow)? and pass that value
to set_lamports.

src/instruction_builder/commit_diff.rs (1)

65-78: 🧹 Nitpick | 🔵 Trivial

commit_diff_size_budget and commit_size_budget have identical bodies — consider deduplication

Both size-budget helpers in commit_diff.rs and commit_state.rs are byte-for-byte identical. While each instruction retains its own public API entry point, the shared logic could be extracted to avoid future drift.

♻️ Suggested refactor (e.g. in a shared `budget` module)

// shared helper (e.g. in src/instruction_builder/mod.rs or a budget.rs)
fn commit_accounts_size_budget(delegated_account: AccountSizeClass) -> u32 {
    total_size_budget(&[
        DLP_PROGRAM_DATA_SIZE_CLASS,
        AccountSizeClass::Tiny, // validator
        delegated_account,      // delegated_account
        delegated_account,      // commit_state_pda
        AccountSizeClass::Tiny, // commit_record_pda
        AccountSizeClass::Tiny, // delegation_record_pda
        AccountSizeClass::Tiny, // delegation_metadata_pda
        AccountSizeClass::Tiny, // validator_fees_vault_pda
        AccountSizeClass::Tiny, // program_config_pda
        AccountSizeClass::Tiny, // system_program
    ])
}

// then each public function delegates to it:
pub fn commit_size_budget(delegated_account: AccountSizeClass) -> u32 {
    commit_accounts_size_budget(delegated_account)
}

pub fn commit_diff_size_budget(delegated_account: AccountSizeClass) -> u32 {
    commit_accounts_size_budget(delegated_account)
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instruction_builder/commit_diff.rs` around lines 65 - 78, The two
functions commit_diff_size_budget and commit_size_budget contain identical
logic; extract the shared array/total_size_budget call into a private helper
(e.g., fn commit_accounts_size_budget(delegated_account: AccountSizeClass) ->
u32) that constructs the same total_size_budget(&[...]) using
DLP_PROGRAM_DATA_SIZE_CLASS and the same AccountSizeClass entries, then have
both public functions simply call that helper with delegated_account so the
implementations are deduplicated while preserving the public APIs.

src/instruction_builder/undelegate.rs (1)

22-22: 🧹 Nitpick | 🔵 Trivial

Unnecessary #[allow(clippy::too_many_arguments)].

The undelegate function only takes 4 parameters — well below Clippy's default threshold of 7. This attribute can be removed.

🧹 Proposed fix

-#[allow(clippy::too_many_arguments)]
 pub fn undelegate(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/instruction_builder/undelegate.rs` at line 22, Remove the unnecessary
Clippy allow attribute: delete the #[allow(clippy::too_many_arguments)]
annotation applied near the undelegate function in
src/instruction_builder/undelegate.rs since undelegate only has four parameters
and doesn't exceed Clippy's threshold; ensure no other uses of that attribute
around the undelegate function remain.

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

34-36: 🧹 Nitpick | 🔵 Trivial

Empty diff proceeds silently — nonce is still consumed.

When diffset.segments_count() == 0, the function logs a warning but continues to process_commit_state_internal, which will advance the nonce and create a commit record for an effective no-op. If this is intentional (e.g., to allow nonce advancement), consider documenting it; otherwise, returning early would save validator funds on rent for the commit state/record PDAs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/fast/commit_diff_from_buffer.rs` around lines 34 - 36, The
handler in commit_diff_from_buffer currently logs a warning when
diffset.segments_count() == 0 but then calls process_commit_state_internal which
consumes the nonce and writes a commit; either short-circuit by returning early
after the warning (so the nonce/PDAs are not advanced) or explicitly document
the intentional behavior and add a flag/comment to preserve it. Modify the
branch where diffset.segments_count() == 0 in commit_diff_from_buffer to return
immediately (e.g., Ok/appropriate result) to avoid calling
process_commit_state_internal, or if you intend to advance the nonce, add a
clear comment and tests to show that behavior is required.

tests/test_call_handler.rs (1)

44-90: 🧹 Nitpick | 🔵 Trivial

Extract duplicated test setup helpers into shared fixture module.

Six setup functions (setup_delegated_pda, setup_commit_state, setup_invalid_escrow_account, setup_delegated_ephemeral_balance, setup_ephemeral_balance, and setup_program_test_env) are identically replicated across test_call_handler.rs and test_call_handler_v2.rs. Extract them into a shared test utility module (e.g., tests/fixtures/ or tests/common.rs) to reduce duplication and maintenance burden.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_call_handler.rs` around lines 44 - 90, Multiple test setup helpers
(setup_delegated_pda, setup_commit_state, setup_invalid_escrow_account,
setup_delegated_ephemeral_balance, setup_ephemeral_balance,
setup_program_test_env) are duplicated across test files; extract them into a
single shared test fixtures module (e.g., tests::fixtures or tests::common) as
public functions, move the implementations there, and replace the duplicated
definitions in each test file with use/imports of the shared module; ensure any
helper dependencies (types, constants like DELEGATED_PDA_ID) are either moved to
the fixture module or re-exported, update imports in the tests to reference the
new module, and run tests to confirm no breakages.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Makefile`:
- Around line 1-8: Makefile targets build, test, and fmt are not declared .PHONY
so Make may skip them if same-named files/dirs exist; add a .PHONY declaration
listing build test fmt (placed near the top of the Makefile) so these targets
always run regardless of filesystem entries and satisfy checkmake static
analysis.

In `@rustfmt.toml`:
- Around line 1-4: The rustfmt config currently sets group_imports =
"StdExternalCrate" which is an unstable option; either enable nightly unstable
features by adding unstable_features = true to rustfmt.toml so group_imports is
honored, or remove the group_imports line to keep the config compatible with
stable rustfmt—update the rustfmt.toml accordingly referencing the existing
group_imports setting and the new unstable_features flag.
- Around line 3-4: The rustfmt configuration uses unstable options
(imports_granularity and group_imports) but lacks the required unstable_features
flag; update rustfmt.toml to enable unstable features by adding
unstable_features = true at the top so those options are honored by nightly
rustfmt (ensure the key is placed before the existing imports_granularity and
group_imports entries).

In `@src/instruction_builder/commit_diff.rs`:
- Around line 22-23: The top-level doc comment in
src/instruction_builder/commit_diff.rs incorrectly says "Builds a commit state
instruction." Update that doc comment to read "Builds a commit diff
instruction." and ensure the summary references
crate::processor::fast::process_commit_diff (leave that link intact); change the
text near the module/function description so the comment accurately reflects the
commit_diff builder rather than "commit state."

In `@src/processor/fast/undelegate.rs`:
- Around line 22-23: Remove the unused import by deleting the `use
crate::compute;` line that's gated with `#[cfg(feature = "log-cost")]`; ensure
only the cfg attribute or both the attribute and the `use crate::compute;`
statement are removed from src/processor/fast/undelegate.rs so no unused
`compute` import remains when the "log-cost" feature is enabled.

In `@src/processor/fast/utils/requires.rs`:
- Around line 395-396: is_uninitialized_account now uses the constant
pinocchio_system::ID but require_uninitialized_account still calls
pinocchio_system::id(); make them consistent by replacing the function call
pinocchio_system::id() with the constant pinocchio_system::ID in
require_uninitialized_account (or vice‑versa if you prefer the function) so both
is_uninitialized_account and require_uninitialized_account use the same symbol
(pinocchio_system::ID).
- Around line 805-809: Replace the unsafe raw pointer cast used to create
upgrade_authority_address with a safe array conversion: use <[u8;
32]>::try_from(&data[offset_of_upgrade_authority_address + 1 ..
offset_of_upgrade_authority_address + 33]) (or equivalent slice-to-array
conversion) and then construct the Address via Address::from(array); remove the
unsafe block that dereferences bytes.as_ptr() and ensure the resulting variable
name upgrade_authority_address is assigned from the Address::from(...) call.
- Around line 156-159: The use of unqualified TryInto inside the macro is a
stylistic inconsistency with the other fully-qualified paths; update the
unqualified TryInto call in the Ordering::Equal arm so it is fully qualified
(e.g., refer to the core convert trait) to match the surrounding qualified paths
like core::cmp::Ordering and $crate::error::DlpError, keeping the same error
mapping and InfallibleError usage.

In `@src/processor/utils/loaders.rs`:
- Around line 291-300: The test imports are inconsistent: `use
super::load_program` is used separately while other functions are imported with
full crate paths (`load_account`, `load_signer`, `load_sysvar`,
`load_uninitialized_account`); update the test `use` statements in the tests
module to import the test helpers from the same scope for consistency (e.g., use
super::{load_program, load_account, load_signer, load_sysvar,
load_uninitialized_account} or simply use super::*), ensuring you reference the
existing symbols load_program, load_account, load_signer, load_sysvar, and
load_uninitialized_account.

In `@tests/test_close_validator_fees_vault.rs`:
- Around line 24-29: Current test only uses admin.pubkey() for both payer and
admin when calling close_validator_fees_vault, so it doesn't cover the new
distinct-payer path; add a second test that obtains the default test payer from
program_test.start() (or test harness) and call
close_validator_fees_vault(payer.pubkey(), admin.pubkey(), validator.pubkey()),
then build and send the transaction with Transaction::new_signed_with_payer
using Some(&payer.pubkey()) as the payer and include both &payer and &admin as
signers to validate the separate payer/admin flow.

In `@tests/test_commit_fees_on_undelegation.rs`:
- Around line 38-53: The test recomputes delegation_record_data.len() and
delegation_metadata_data.len() locally which duplicates logic in
setup_program_test_env and can drift; update setup_program_test_env (or the
helper it calls) to return the seeded sizes (e.g., delegation_record_size and
delegation_metadata_size) or expose constants, then change the test to use those
returned sizes when computing record_rent, metadata_rent and expected_total_fees
(keep references to delegation_record_data,
create_delegation_metadata_data_with_nonce only in the seeding helper and stop
recreating them in the test).

In `@tests/test_undelegate_without_commit.rs`:
- Around line 63-71: The second, identical assertion block re-checking
delegation_record_pda is a duplicate; either remove it or replace it with the
intended PDA check (likely for undelegate_buffer_pda or delegation_metadata).
Locate the repeated block that queries banks.get_account(delegation_record_pda)
and assert is_none() and either delete that repeated block or change the
variable and PDA reference to the correct PDA (e.g., undelegate_buffer_pda) and
assert that account is_none() so the test verifies the intended account was
closed.

---

Outside diff comments:
In `@src/instruction_builder/call_handler.rs`:
- Around line 59-71: The parameter other_accounts in call_handler_size_budget is
misleading because its value is added as a size budget, not a count; rename it
to other_accounts_size_budget in the function signature and all call sites,
update any docs/tests to use the new name, and keep the logic unchanged (i.e.,
return total_size_budget(...) + other_accounts_size_budget) so callers clearly
pass a size budget rather than a count.

In `@src/instruction_builder/commit_diff.rs`:
- Around line 65-78: The two functions commit_diff_size_budget and
commit_size_budget contain identical logic; extract the shared
array/total_size_budget call into a private helper (e.g., fn
commit_accounts_size_budget(delegated_account: AccountSizeClass) -> u32) that
constructs the same total_size_budget(&[...]) using DLP_PROGRAM_DATA_SIZE_CLASS
and the same AccountSizeClass entries, then have both public functions simply
call that helper with delegated_account so the implementations are deduplicated
while preserving the public APIs.

In `@src/instruction_builder/commit_finalize_from_buffer.rs`:
- Around line 73-92: Update the doc comment on
commit_finalize_from_buffer_size_budget to reference commit_finalize_from_buffer
(not commit_diff_from_buffer), and remove the spurious AccountSizeClass::Tiny
entry for program_config_pda from the total_size_budget array so the returned
account-data-size budget matches the actual accounts the processor expects
(validator, delegated_account, delegation_record_pda, delegation_metadata_pda,
data_buffer, validator_fees_vault_pda, system_program). Ensure the remaining
array still includes DLP_PROGRAM_DATA_SIZE_CLASS and the two delegated_account
entries as currently used.

In `@src/instruction_builder/undelegate.rs`:
- Line 22: Remove the unnecessary Clippy allow attribute: delete the
#[allow(clippy::too_many_arguments)] annotation applied near the undelegate
function in src/instruction_builder/undelegate.rs since undelegate only has four
parameters and doesn't exceed Clippy's threshold; ensure no other uses of that
attribute around the undelegate function remain.

In `@src/processor/close_validator_fees_vault.rs`:
- Around line 19-24: Update the doc comment for the close_validator_fees_vault
instruction to (1) insert the missing delegation_program_data account between
admin and validator_identity so the account indices align with the destructuring
in the function (reference the symbol delegation_program_data in the comment)
and (2) change the validator_identity account entry from non-writable (`[]`) to
writable (`[writable]` or `[mut]`) because close_pda transfers lamports into
validator_identity; ensure validator_fees_vault_pda remains listed after
validator_identity and that the doc comment matches the actual parameter order
used by the close_validator_fees_vault logic.

In `@src/processor/fast/commit_diff_from_buffer.rs`:
- Around line 34-36: The handler in commit_diff_from_buffer currently logs a
warning when diffset.segments_count() == 0 but then calls
process_commit_state_internal which consumes the nonce and writes a commit;
either short-circuit by returning early after the warning (so the nonce/PDAs are
not advanced) or explicitly document the intentional behavior and add a
flag/comment to preserve it. Modify the branch where diffset.segments_count() ==
0 in commit_diff_from_buffer to return immediately (e.g., Ok/appropriate result)
to avoid calling process_commit_state_internal, or if you intend to advance the
nonce, add a clear comment and tests to show that behavior is required.

In `@src/processor/fast/finalize.rs`:
- Around line 221-231: The credit to transfer_destination currently uses plain
addition which is inconsistent with the checked subtraction above; update the
code that sets transfer_destination's lamports in finalize.rs (symbols:
transfer_destination, set_lamports, lamports(), transfer_lamports) to use a
checked_add and propagate DlpError::Overflow on overflow (mirror the pattern
used for transfer_source where checked_sub is used), ensuring you call
.checked_add(transfer_lamports).ok_or(DlpError::Overflow)? and pass that value
to set_lamports.

In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 20-21: The doc comment for the struct
CommitFinalizeInternalArgs<'a> is stale (says "commit state"); update the
comment to accurately describe the struct as arguments for the "commit finalize"
internal function (e.g., change "Arguments for the commit state internal
function" to "Arguments for the commit finalize internal function") so the
documentation matches the symbol CommitFinalizeInternalArgs and its purpose.

In `@src/processor/init_protocol_fees_vault.rs`:
- Line 25: Doc comment in init_protocol_fees_vault.rs contains a typo: change
the word "permisionless" to "permissionless" in the comment above the operation
(the doc line starting with "NOTE: this operation is ...") so the comment reads
"permissionless"; make the edit where the doc string or inline comment is
defined (the NOTE comment) to correct the spelling.

In `@src/processor/init_validator_fees_vault.rs`:
- Around line 22-26: The account doc comment above the init_validator_fees_vault
handler is wrong: it omits delegation_program_data and mis-indexes accounts 2–4;
update the comment to list six accounts matching the destructured variables
(payer, admin, delegation_program_data, validator_identity,
validator_fees_vault_pda, system_program) and correct the index numbers so each
label aligns with the corresponding variable used in the function (e.g., index 2
=> delegation_program_data, 3 => validator_identity, 4 =>
validator_fees_vault_pda, 5 => system_program).

In `@src/processor/top_up_ephemeral_balance.rs`:
- Line 48: The payer is only validated as a signer via load_signer(payer,
"payer") but not checked for writability; add an explicit writability check
before the CPI transfer similar to how ephemeral_balance_account is validated
with load_pda(..., true, ...). Concretely, after load_signer(payer, "payer")?;
assert payer.is_writable (or call the existing assert_writable/ensure_writable
helper if one exists) and return a descriptive error (e.g., "payer must be
writable") if not, so the payer account is guaranteed writable prior to the
transfer.

In `@src/processor/utils/pda.rs`:
- Around line 90-105: The resize_pda function is using Rent::default() which
reads hardcoded defaults; change it to read the on-chain rent sysvar by calling
Rent::get()? and use its minimum_balance(new_size) (i.e., replace
Rent::default().minimum_balance(new_size) with
Rent::get()?.minimum_balance(new_size)) so new_minimum_balance is computed from
the live sysvar; ensure the function's Result propagation still returns Err if
Rent::get() fails (the existing signature returning Result<(), ProgramError> is
fine).

In `@src/processor/validator_claim_fees.rs`:
- Around line 53-59: Replace the unchecked subtraction and default Rent usage:
load the live rent via Rent::get() (not Rent::default()) to compute min_rent,
and replace all bare uses of validator_fees_vault.lamports() - min_rent with
checked_sub calls (e.g., validator_fees_vault.lamports().checked_sub(min_rent))
and properly handle the Option return to early-return/error when lamports <
min_rent so the solvency guard before assigning amount (and in the args.amount
Some branch) cannot be bypassed; update the logic around
args.amount.unwrap_or(...) and later checked_sub usage to use the same checked
result and fail fast on None.
- Around line 70-71: The multiplication of amount by PROTOCOL_FEES_PERCENTAGE
can overflow; replace the unchecked multiply/divide with a checked/widened
arithmetic path: compute protocol_fees by either using
amount.checked_mul(u64::from(PROTOCOL_FEES_PERCENTAGE)).and_then(|v|
v.checked_div(100)) and handle the None case (return an error) or cast amount
and PROTOCOL_FEES_PERCENTAGE to a wider integer (e.g., u128), perform
(amount_u128 * percent_u128) / 100, then try_into() back to u64 and handle
conversion failure; update the calculation for protocol_fees and then compute
remaining_amount via checked_sub or saturating_sub using the validated
protocol_fees variable (referencing the variables protocol_fees,
remaining_amount, amount and the constant PROTOCOL_FEES_PERCENTAGE).

In `@src/processor/whitelist_validator_for_program.rs`:
- Around line 20-30: The doc comment for the Whitelist a validator for a program
handler in src/processor/whitelist_validator_for_program.rs is stale and must
include the missing delegation_program_data account; update the accounts list to
reflect seven accounts (add a new entry for `delegation_program_data` between
`program_data` and `program_config_account`) so it matches the account
destructuring in the handler and the instruction builder in
src/instruction_builder/whitelist_validator_for_program.rs which passes
`delegation_program_data`; ensure the numbering and brief descriptions align
with the existing entries (0: authority, 1: validator identity, 2: program, 3:
program_data, 4: delegation_program_data, 5: program_config_account, 6: system
program).

In `@tests/test_call_handler.rs`:
- Around line 44-90: Multiple test setup helpers (setup_delegated_pda,
setup_commit_state, setup_invalid_escrow_account,
setup_delegated_ephemeral_balance, setup_ephemeral_balance,
setup_program_test_env) are duplicated across test files; extract them into a
single shared test fixtures module (e.g., tests::fixtures or tests::common) as
public functions, move the implementations there, and replace the duplicated
definitions in each test file with use/imports of the shared module; ensure any
helper dependencies (types, constants like DELEGATED_PDA_ID) are either moved to
the fixture module or re-exported, update imports in the tests to reference the
new module, and run tests to confirm no breakages.

In `@tests/test_commit_finalize.rs`:
- Line 75: Update the comment in tests/test_commit_finalize.rs that reads "//
execute CommitFinalize and validate CU performmace" to correct the typo by
changing "performmace" to "performance" (the test symbol is
test_commit_finalize/CommitFinalize comment); simply edit the inline comment
text to "// execute CommitFinalize and validate CU performance".

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for any references to 'compute' in the undelegate.rs file
rg -n 'compute' src/processor/fast/undelegate.rs

Repository: magicblock-labs/delegation-program

Length of output: 100

---

Remove the unused compute import. The import on line 23 is never referenced in this file and will produce a compiler warning when the log-cost feature is enabled.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/processor/fast/undelegate.rs` around lines 22 - 23, Remove the unused
import by deleting the `use crate::compute;` line that's gated with
`#[cfg(feature = "log-cost")]`; ensure only the cfg attribute or both the
attribute and the `use crate::compute;` statement are removed from
src/processor/fast/undelegate.rs so no unused `compute` import remains when the
"log-cost" feature is enabled.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Test does not exercise the distinct-payer path introduced by the new payer parameter.

close_validator_fees_vault was updated in this PR to accept a separate payer argument, but the test passes admin.pubkey() for both payer and admin. This means two AccountMeta entries in the instruction resolve to the same account, which Solana handles fine, but the test only covers the degenerate case where the fee payer is the admin.

Consider adding a second test case using payer from program_test.start() (the default test payer) as the fee payer while keeping admin_keypair as the admin, to validate the separation of concerns the new parameter was designed to enable.

💡 Suggested additional test scaffold

 async fn setup_program_test_env() -> (BanksClient, Keypair, Keypair, Hash) {
     ...
-    let (banks, _, blockhash) = program_test.start().await;
-    (banks, admin_keypair, validator, blockhash)
+    let (banks, payer, blockhash) = program_test.start().await;
+    (banks, payer, admin_keypair, validator, blockhash)
 }

Then add a test that calls:

let ix = dlp::instruction_builder::close_validator_fees_vault(
    payer.pubkey(),  // distinct fee payer
    admin.pubkey(),  // admin
    validator.pubkey(),
);
let tx = Transaction::new_signed_with_payer(
    &[ix],
    Some(&payer.pubkey()),
    &[&payer, &admin],
    blockhash,
);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_close_validator_fees_vault.rs` around lines 24 - 29, Current test
only uses admin.pubkey() for both payer and admin when calling
close_validator_fees_vault, so it doesn't cover the new distinct-payer path; add
a second test that obtains the default test payer from program_test.start() (or
test harness) and call close_validator_fees_vault(payer.pubkey(),
admin.pubkey(), validator.pubkey()), then build and send the transaction with
Transaction::new_signed_with_payer using Some(&payer.pubkey()) as the payer and
include both &payer and &admin as signers to validate the separate payer/admin
flow.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Data for fee calculation duplicates setup — fragile coupling.

delegation_record_data (lines 38-39) and delegation_metadata_data (lines 40-45) are computed here solely for their .len() values to derive expected rent, but the identical calls already appear in setup_program_test_env (lines 112-132). If either site is updated independently, the expected fee assertion will silently diverge from the actual account sizes seeded in the test.

Consider extracting the shared constants or returning the sizes from setup_program_test_env to keep the two sites in sync:

♻️ Suggested approach

-async fn setup_program_test_env() -> (BanksClient, Keypair, Keypair, Hash) {
+async fn setup_program_test_env() -> (BanksClient, Keypair, Keypair, Hash, usize, usize) {
     ...
+    let record_len = delegation_record_data.len();
+    let metadata_len = delegation_metadata_data.len();
     let (banks, payer, blockhash) = program_test.start().await;
-    (banks, payer, validator, blockhash)
+    (banks, payer, validator, blockhash, record_len, metadata_len)
 }

Then in the test body, use the returned sizes directly instead of recomputing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_commit_fees_on_undelegation.rs` around lines 38 - 53, The test
recomputes delegation_record_data.len() and delegation_metadata_data.len()
locally which duplicates logic in setup_program_test_env and can drift; update
setup_program_test_env (or the helper it calls) to return the seeded sizes
(e.g., delegation_record_size and delegation_metadata_size) or expose constants,
then change the test to use those returned sizes when computing record_rent,
metadata_rent and expected_total_fees (keep references to
delegation_record_data, create_delegation_metadata_data_with_nonce only in the
seeding helper and stop recreating them in the test).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Duplicate/redundant assertion — likely a copy-paste error.

Lines 68-71 are a verbatim repeat of lines 63-66: same comment, same variable name (delegation_record_account), same banks.get_account(delegation_record_pda) call, same assert!(…is_none()). Because the account was already confirmed None five lines earlier, this assertion is vacuously true and covers no additional invariant.

Based on the surrounding test structure (commit_state → delegation_record → ? → delegation_metadata), this block was likely meant to assert a different PDA (e.g., an undelegate_buffer_pda). If no additional account needs to be checked, the duplicate block should simply be removed.

🐛 Proposed fix

-    // Assert the delegation_record_pda was closed
-    let delegation_record_account =
-        banks.get_account(delegation_record_pda).await.unwrap();
-    assert!(delegation_record_account.is_none());
-
     // Assert the delegated account seeds pda was closed

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Assert the delegation_record_pda was closed
	let delegation_record_account = banks.get_account(delegation_record_pda).await.unwrap();
	let delegation_record_account =
	banks.get_account(delegation_record_pda).await.unwrap();
	assert!(delegation_record_account.is_none());
	// Assert the delegation_record_pda was closed
	let delegation_record_account = banks.get_account(delegation_record_pda).await.unwrap();
	let delegation_record_account =
	banks.get_account(delegation_record_pda).await.unwrap();
	assert!(delegation_record_account.is_none());
	// Assert the delegation_record_pda was closed
	let delegation_record_account =
	banks.get_account(delegation_record_pda).await.unwrap();
	assert!(delegation_record_account.is_none());
	// Assert the delegated account seeds pda was closed

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_undelegate_without_commit.rs` around lines 63 - 71, The second,
identical assertion block re-checking delegation_record_pda is a duplicate;
either remove it or replace it with the intended PDA check (likely for
undelegate_buffer_pda or delegation_metadata). Locate the repeated block that
queries banks.get_account(delegation_record_pda) and assert is_none() and either
delete that repeated block or change the variable and PDA reference to the
correct PDA (e.g., undelegate_buffer_pda) and assert that account is_none() so
the test verifies the intended account was closed.