feat: configurable fees receiver #90
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a FeesVault account type and a SetFeesReceiver instruction (args, builder, processor), a fast-path instruction dispatcher, discriminator updates, PDA migration/resize logic, updates to init/claim/close processors to use delegation program data and FeesVault, and corresponding Rust and TypeScript test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant Program as DLP Program
participant DelegationPD as Delegation Program Data PDA
participant FeesVault as Fees Vault PDA
participant Receiver
Admin->>Program: set_fees_receiver(admin sig, fees_receiver pubkey)
Program->>DelegationPD: load_program_upgrade_authority()
alt admin is upgrade authority
Program->>FeesVault: load or migrate FeesVault (may resize PDA)
Program->>FeesVault: write FeesVault { fees_receiver }
Program-->>Admin: Ok
else unauthorized
Program-->>Admin: Unauthorized error
end
note over Program,FeesVault: Later: claim flow
Caller->>Program: protocol_claim_fees(fees_receiver)
Program->>FeesVault: load_initialized_protocol_fees_vault()
Program->>Program: verify provided fees_receiver == FeesVault.fees_receiver
alt match
Program->>FeesVault: compute transferable lamports (balance - rent)
Program->>Receiver: transfer lamports
Program-->>Caller: Ok
else mismatch
Program-->>Caller: InvalidArgument error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-29T10:43:00.058ZApplied to files:
🧬 Code graph analysis (2)src/processor/protocol_claim_fees.rs (1)
tests/test_protocol_claim_fees.rs (5)
🔇 Additional comments (2)
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.
Actionable comments posted: 40
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
tests/integration/tests/test-delegation.ts (3)
206-216: Bug: negative index doesn’t mutate the buffernew_data[-1] defines a property; it doesn’t update the last byte. This breaks commit semantics.
Apply:
- let account = await provider.connection.getAccountInfo(pda); - let new_data = account.data; - new_data[-1] = (new_data[-1] + 1) % 256; + const account = await provider.connection.getAccountInfo(pda); + const newData = new Uint8Array(account.data); + const last = newData.length - 1; + newData[last] = (newData[last] + 1) & 0xff; @@ - data: Uint8Array.from(new_data), + data: newData,
263-268: Bug: same negative-index issue (again)Fix mirrors the earlier commit-state block.
- let account = await provider.connection.getAccountInfo(pda); - let new_data = account.data; - new_data[-1] = (new_data[-1] + 1) % 256; + const account = await provider.connection.getAccountInfo(pda); + const newData = new Uint8Array(account.data); + const last = newData.length - 1; + newData[last] = (newData[last] + 1) & 0xff; @@ - data: Uint8Array.from(new_data), + data: newData,
601-619: protocol_claim_fees: missing ProgramConfig accountPer the refactor, this ix requires the ProgramConfig for the target program. Add it to keys so on-chain can read it.
function createClaimProtocolFeesVaultInstruction( feesReceiver: web3.PublicKey, program: web3.PublicKey ) { const feesVault = feesVaultPda(); + const programConfig = programConfigPdaFromProgramId(program); const keys = [ { pubkey: feesVault, isSigner: false, isWritable: true }, { pubkey: feesReceiver, isSigner: false, isWritable: true }, - { pubkey: program, isSigner: false, isWritable: false }, + { pubkey: programConfig, isSigner: false, isWritable: false }, + { pubkey: program, isSigner: false, isWritable: false }, ];tests/test_protocol_claim_fees.rs (1)
78-88: Fixture optimization: program_config account not used by this testYou can drop the program_config account here to reduce setup, since process_protocol_claim_fees doesn’t read it.
Apply this diff:
- // Setup the fees program config account - program_test.add_account( - program_config_from_program_id(&dlp::ID), - Account { - lamports: LAMPORTS_PER_SOL, - data: create_program_config_data(Pubkey::new_unique(), admin_keypair.pubkey()), - owner: dlp::id(), - executable: false, - rent_epoch: 0, - }, - ); + // (Optional) Omit ProgramConfig for this test; not read by protocol_claim_feessrc/processor/init_protocol_fees_vault.rs (1)
30-31: Typo: “permisionless” → “permissionless”Apply this diff:
-/// NOTE: this operation is permisionless and can be done by anyone +/// NOTE: this operation is permissionless and can be done by anyonesrc/pda.rs (1)
63-68: Unify fees-vault seed with a tag constant.All other PDAs use tag constants; this macro hardcodes the literal. Centralize to avoid drift.
Apply:
+pub const PROTOCOL_FEES_VAULT_TAG: &[u8] = b"fees-vault"; #[macro_export] macro_rules! fees_vault_seeds { () => { - &[b"fees-vault"] + &[$crate::pda::PROTOCOL_FEES_VAULT_TAG] }; }src/processor/fast/commit_state.rs (1)
67-71: Optional: drop unused _system_program account.Fast path no longer uses system program; removing it reduces account count and CU.
src/processor/fast/undelegate.rs (1)
156-170: Fast-path owner reassign on empty data: add a system program checkUnsafe assign is fine here. However, you rely on system_program later; add a require_program check once up front to ensure the system account is genuine.
@@ use super::{ - require_initialized_delegation_metadata, require_initialized_delegation_record, - require_initialized_protocol_fees_vault, require_initialized_validator_fees_vault, - require_owned_pda, require_signer, + require_initialized_delegation_metadata, require_initialized_delegation_record, + require_initialized_protocol_fees_vault, require_initialized_validator_fees_vault, + require_owned_pda, require_signer, require_program, }, @@ pub fn process_undelegate( - require_initialized_validator_fees_vault(validator, validator_fees_vault, true)?; + require_initialized_validator_fees_vault(validator, validator_fees_vault, true)?; + require_program(system_program, &pinocchio_system::id(), "system program")?;This guards against spoofed system accounts before CPI.
♻️ Duplicate comments (7)
tests/integration/tests/test-delegation.ts (2)
226-238: Repeat the CU-log guard hereSame risk as above; add the undefined check before parsing CU.
244-256: Repeat the CU-log guard here as wellApply the same null-check before parsing CU.
tests/test_validator_claim_fees.rs (1)
5-5: Same processor removal pattern as other test files.This file exhibits the same processor removal pattern as
test_init_validator_fees_vault.rsand other test files. The concerns raised about undocumented architectural changes and test behavior verification apply here as well.Also applies to: 81-81
tests/test_finalize.rs (1)
13-13: Duplicate processor removal pattern.Same systematic test infrastructure change as previously reviewed test files.
Also applies to: 86-86
tests/test_commit_state.rs (1)
10-10: Duplicate processor removal pattern.Same systematic test infrastructure change as previously reviewed test files.
Also applies to: 123-123
tests/test_delegate.rs (1)
4-4: Duplicate processor removal pattern.Same systematic test infrastructure change as previously reviewed test files. The blank line addition at line 81 is a minor formatting adjustment.
Also applies to: 80-81
src/processor/protocol_claim_fees.rs (1)
11-14: Docs: update accounts and step; remove “admin” wordingDocs still say “admin authority” and call fees receiver a “PDA”. It’s just an account; and this path is permissionless.
Apply this diff:
-/// 1. `[writable]` protocol fees vault PDA -/// 2. `[writable]` fees receiver PDA -/// 3. `[]` delegation program +/// 1. `[writable]` protocol fees vault PDA +/// 2. `[writable]` fees receiver account +/// 3. `[]` delegation program @@ -/// 1. Transfer lamports from protocol fees_vault PDA to the admin authority +/// 1. Transfer lamports from the protocol fees vault to the configured fees receiverAlso applies to: 20-23
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.locktests/buffers/test_delegation.sois excluded by!**/*.sotests/integration/Cargo.lockis excluded by!**/*.locktests/integration/package-lock.jsonis excluded by!**/package-lock.jsontests/integration/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (67)
.github/workflows/run-tests.yml(0 hunks)Cargo.toml(3 hunks)src/args/call_handler.rs(1 hunks)src/args/mod.rs(2 hunks)src/args/set_fees_receiver.rs(1 hunks)src/consts.rs(0 hunks)src/cu.rs(1 hunks)src/discriminator.rs(1 hunks)src/entrypoint.rs(1 hunks)src/error.rs(1 hunks)src/instruction_builder/init_protocol_fees_vault.rs(2 hunks)src/instruction_builder/mod.rs(2 hunks)src/instruction_builder/protocol_claim_fees.rs(1 hunks)src/instruction_builder/set_fees_receiver.rs(1 hunks)src/lib.rs(3 hunks)src/pda.rs(2 hunks)src/processor/call_handler.rs(1 hunks)src/processor/close_validator_fees_vault.rs(1 hunks)src/processor/commit_state_from_buffer.rs(0 hunks)src/processor/delegate.rs(0 hunks)src/processor/fast/commit_state.rs(5 hunks)src/processor/fast/commit_state_from_buffer.rs(1 hunks)src/processor/fast/delegate.rs(1 hunks)src/processor/fast/finalize.rs(6 hunks)src/processor/fast/mod.rs(1 hunks)src/processor/fast/undelegate.rs(7 hunks)src/processor/fast/utils/mod.rs(1 hunks)src/processor/fast/utils/pda.rs(1 hunks)src/processor/fast/utils/requires.rs(1 hunks)src/processor/init_protocol_fees_vault.rs(3 hunks)src/processor/mod.rs(1 hunks)src/processor/protocol_claim_fees.rs(3 hunks)src/processor/set_fees_receiver.rs(1 hunks)src/processor/utils/curve.rs(1 hunks)src/processor/utils/loaders.rs(6 hunks)src/processor/utils/pda.rs(0 hunks)src/processor/whitelist_validator_for_program.rs(1 hunks)src/state/delegation_metadata.rs(1 hunks)src/state/fees_vault.rs(1 hunks)src/state/mod.rs(1 hunks)src/state/program_config.rs(2 hunks)src/state/utils/discriminator.rs(1 hunks)tests/fixtures/accounts.rs(1 hunks)tests/integration/package.json(2 hunks)tests/integration/programs/test-delegation/Cargo.toml(1 hunks)tests/integration/programs/test-delegation/src/lib.rs(6 hunks)tests/integration/tests/test-delegation.ts(13 hunks)tests/test_call_handler.rs(8 hunks)tests/test_close_validator_fees_vault.rs(2 hunks)tests/test_commit_on_curve.rs(2 hunks)tests/test_commit_state.rs(2 hunks)tests/test_commit_state_from_buffer.rs(2 hunks)tests/test_commit_state_with_program_config.rs(3 hunks)tests/test_delegate.rs(2 hunks)tests/test_delegate_on_curve.rs(2 hunks)tests/test_finalize.rs(2 hunks)tests/test_init_fees_vault.rs(2 hunks)tests/test_init_validator_fees_vault.rs(2 hunks)tests/test_lamports_settlement.rs(2 hunks)tests/test_protocol_claim_fees.rs(3 hunks)tests/test_set_fees_receiver.rs(1 hunks)tests/test_top_up.rs(2 hunks)tests/test_undelegate.rs(2 hunks)tests/test_undelegate_on_curve.rs(2 hunks)tests/test_undelegate_without_commit.rs(2 hunks)tests/test_validator_claim_fees.rs(2 hunks)tests/test_whitelist_validator_for_program.rs(2 hunks)
💤 Files with no reviewable changes (5)
- src/consts.rs
- src/processor/commit_state_from_buffer.rs
- .github/workflows/run-tests.yml
- src/processor/delegate.rs
- src/processor/utils/pda.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-11T18:00:22.232Z
Learnt from: snawaz
PR: magicblock-labs/delegation-program#94
File: src/lib.rs:93-103
Timestamp: 2025-10-11T18:00:22.232Z
Learning: When reviewing Rust code, the `TryFromPrimitive` derive macro from the `num_enum` crate automatically generates `TryFrom<u8>` (and other primitive type conversions) for enums marked with `#[repr(u8)]`. Don't flag missing `TryFrom<u8>` implementations when this derive macro is present.
Applied to files:
src/discriminator.rs
📚 Learning: 2025-10-16T10:04:25.110Z
Learnt from: taco-paco
PR: magicblock-labs/delegation-program#105
File: src/args/call_handler.rs:6-8
Timestamp: 2025-10-16T10:04:25.110Z
Learning: The `data` field in the `CallHandlerArgs` struct (src/args/call_handler.rs) is intentionally designed to accept any length without max length validation, allowing flexible instruction formats including custom discriminators and arguments.
Applied to files:
src/args/call_handler.rs
📚 Learning: 2025-10-16T10:04:26.576Z
Learnt from: taco-paco
PR: magicblock-labs/delegation-program#105
File: src/args/call_handler.rs:6-8
Timestamp: 2025-10-16T10:04:26.576Z
Learning: In `src/args/call_handler.rs`, the `data` field of the `CallHandlerArgs` struct has no maximum length constraint by design, as it needs to support flexible instruction formats including custom discriminators and arguments in any format.
Applied to files:
src/args/call_handler.rs
🧬 Code graph analysis (26)
src/args/mod.rs (1)
src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)
src/instruction_builder/mod.rs (1)
src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)
src/instruction_builder/set_fees_receiver.rs (2)
src/discriminator.rs (1)
to_vec(42-45)src/pda.rs (1)
fees_vault_pda(149-151)
src/instruction_builder/protocol_claim_fees.rs (1)
src/pda.rs (1)
fees_vault_pda(149-151)
src/processor/init_protocol_fees_vault.rs (1)
src/processor/utils/loaders.rs (4)
load_program(176-188)load_program_upgrade_authority(191-226)load_signer(23-30)load_uninitialized_pda(59-75)
src/error.rs (1)
src/entrypoint.rs (1)
pinocchio(19-19)
src/entrypoint.rs (1)
src/lib.rs (2)
fast_process_instruction(41-82)slow_process_instruction(84-137)
src/processor/fast/delegate.rs (4)
src/processor/fast/mod.rs (1)
to_pinocchio_program_error(14-18)src/processor/fast/utils/pda.rs (1)
create_pda(12-62)src/processor/fast/utils/requires.rs (5)
require_uninitialized_pda(135-152)require_owned_pda(33-44)require_pda(62-84)require_signer(49-57)find_program_address(21-27)src/processor/utils/curve.rs (1)
is_on_curve_fast(1-26)
src/processor/mod.rs (1)
src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)
src/processor/fast/commit_state_from_buffer.rs (1)
src/processor/fast/commit_state.rs (1)
process_commit_state_internal(108-284)
src/processor/protocol_claim_fees.rs (1)
src/processor/utils/loaders.rs (2)
load_account(154-171)load_initialized_protocol_fees_vault(230-242)
src/processor/fast/mod.rs (2)
src/entrypoint.rs (1)
pinocchio(19-19)src/error.rs (2)
from(41-43)from(47-49)
tests/test_call_handler.rs (1)
src/discriminator.rs (1)
to_vec(42-45)
src/state/fees_vault.rs (1)
src/state/utils/discriminator.rs (1)
discriminator(23-23)
src/instruction_builder/init_protocol_fees_vault.rs (2)
src/processor/fast/utils/requires.rs (1)
find_program_address(21-27)src/pda.rs (1)
fees_vault_pda(149-151)
src/processor/set_fees_receiver.rs (3)
src/processor/utils/loaders.rs (3)
load_initialized_protocol_fees_vault(230-242)load_program_upgrade_authority(191-226)load_signer(23-30)src/processor/utils/pda.rs (1)
resize_pda(85-100)src/state/fees_vault.rs (2)
discriminator(14-16)size_with_discriminator(20-22)
tests/test_set_fees_receiver.rs (4)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-154)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-20)
src/processor/utils/loaders.rs (1)
src/pda.rs (1)
validator_fees_vault_pda_from_validator(153-159)
src/processor/fast/utils/pda.rs (1)
src/processor/utils/pda.rs (2)
create_pda(10-82)close_pda(104-117)
src/processor/fast/commit_state.rs (3)
src/processor/fast/utils/pda.rs (1)
create_pda(12-62)src/processor/fast/utils/requires.rs (7)
require_initialized_delegation_metadata(290-303)require_initialized_delegation_record(273-286)require_initialized_validator_fees_vault(223-244)require_owned_pda(33-44)require_program_config(248-269)require_signer(49-57)require_uninitialized_pda(135-152)src/processor/fast/mod.rs (1)
to_pinocchio_program_error(14-18)
src/lib.rs (3)
src/entrypoint.rs (2)
entrypoint(13-31)pinocchio(19-19)src/processor/call_handler.rs (1)
process_call_handler(46-123)src/processor/set_fees_receiver.rs (1)
process_set_fees_receiver(28-88)
src/processor/fast/finalize.rs (3)
src/processor/fast/utils/pda.rs (1)
close_pda(66-80)src/processor/fast/utils/requires.rs (8)
is_uninitialized_account(89-91)require_initialized_commit_record(324-337)require_initialized_commit_state(307-320)require_initialized_delegation_metadata(290-303)require_initialized_delegation_record(273-286)require_initialized_validator_fees_vault(223-244)require_owned_pda(33-44)require_signer(49-57)src/processor/fast/mod.rs (1)
to_pinocchio_program_error(14-18)
tests/test_protocol_claim_fees.rs (3)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-154)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-20)
src/processor/fast/utils/requires.rs (2)
src/pda.rs (2)
program_config_from_program_id(161-167)validator_fees_vault_pda_from_validator(153-159)src/instruction_builder/commit_state.rs (1)
commit_state(16-45)
tests/test_commit_state_with_program_config.rs (1)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-154)
src/processor/fast/undelegate.rs (3)
src/processor/fast/utils/pda.rs (3)
close_pda(66-80)close_pda_with_fees(85-137)create_pda(12-62)src/processor/fast/utils/requires.rs (7)
require_uninitialized_pda(135-152)require_initialized_delegation_metadata(290-303)require_initialized_delegation_record(273-286)require_initialized_protocol_fees_vault(206-218)require_initialized_validator_fees_vault(223-244)require_owned_pda(33-44)require_signer(49-57)src/processor/fast/mod.rs (1)
to_pinocchio_program_error(14-18)
🔇 Additional comments (82)
tests/integration/package.json (1)
10-10: Version discrepancy: PR objectives state v0.2.10, but code specifies ^0.2.11.The PR summary mentions migrating to "@magicblock-labs/ephemeral-rollups-sdk v0.2.10", but the package.json specifies "^0.2.11". Please clarify which version is intended and ensure consistency across documentation and code.
Additionally, verify that:
- Version 0.2.11 exists on the npm registry
- The caret (^) constraint is intentional and will not inadvertently pull in incompatible minor/major versions
- Integration tests pass against 0.2.11 and any compatible versions within the ^0.2.11 range
src/cu.rs (2)
1-2: LGTM: Appropriate imports for Pinocchio-based benchmarking.The imports correctly bring in the syscall for querying compute units and the logging macro.
4-7: LGTM: Clean struct design.The struct appropriately encapsulates the benchmark state with private fields.
src/processor/utils/curve.rs (1)
1-3: No action required — review comment is based on incorrect assumptionsThe function change doesn't break any call sites. The sole call at
src/processor/fast/delegate.rs:104passesdelegated_account.key(), which already returns&pinocchio::pubkey::Pubkey(compatible with the new signature). The codebase is pinocchio-based at runtime, not transitioning fromsolana_programtypes. No deprecated wrapper is needed.Likely an incorrect or invalid review comment.
tests/integration/programs/test-delegation/Cargo.toml (2)
18-20: Opt-in features look fineanchor-debug/custom-panic/custom-heap are opt-in and won’t affect default builds. No issues.
24-24: SDK bump to 0.2.12Looks good. Ensure workspace lockfiles are updated and CI runs against the published package.
src/error.rs (2)
36-37: New error variant is appropriateTooManySeeds = 14 aligns with seed-bound checks and error mapping.
46-50: The review comment is incorrect—pinocchio is a required, non-optional dependency.Pinocchio is declared as
pinocchio = "0.9.2"in Cargo.toml without theoptional = trueflag, making it a mandatory dependency that is always compiled. The codebase architecture depends on pinocchio being available:src/entrypoint.rsunconditionally callspinocchio::entrypoint::deserialize,src/lib.rsuses pinocchio types infast_process_instruction, and all fast path processors insrc/processor/fast/require pinocchio. Feature-gating only this impl block while the rest of the codebase uses pinocchio unconditionally would be inconsistent and would not prevent any build breaks. Additionally, theFrom<DlpError>impl forsolana_program::ProgramErrorin the same file is also not feature-gated, establishing the pattern that these conversions are unconditional.Likely an incorrect or invalid review comment.
tests/test_delegate_on_curve.rs (2)
2-2: Import change LGTMImports align with ProgramTest usage.
124-124: Using ProgramTest::new(..., None) is correctRelying on the program’s entrypoint is consistent with other tests. Keep prefer_bpf(true) as set.
src/args/call_handler.rs (1)
6-7: Docstring clarity LGTMExplicitly stating arbitrary formats for data is good and matches intent.
Based on learnings
tests/integration/tests/test-delegation.ts (6)
12-13: New imports are fineUsing SYSTEM_PROGRAM_ID and chai assert is appropriate here.
16-18: BPF loader constant LGTMCorrect program id for ProgramData derivations.
497-521: Init fees vault accounts LGTMProgramData derivation via BPF loader and account metas look correct.
531-533: Validator vault init: ProgramData derivation LGTMDeriving program-data with BPF loader for DLP is correct.
556-571: Claim validator fees vault: accounts look consistentWritable flags make sense; no issues spotted.
629-635: Whitelist builder: BPF loader usage LGTMBoth programData derivations are correct.
src/instruction_builder/mod.rs (1)
12-12: LGTM: Module addition follows the established pattern.The new
set_fees_receivermodule is properly declared and exported, consistent with the existing module structure.Also applies to: 29-29
Cargo.toml (1)
4-4: Verify version bump from 1.1.0 to 1.1.2.The version jumps from 1.1.0 to 1.1.2, skipping 1.1.1. Ensure this follows your versioning strategy and that 1.1.1 wasn't accidentally released or reserved.
src/state/mod.rs (1)
4-4: LGTM: State module addition follows the established pattern.The new
fees_vaultstate module is properly declared and exported, consistent with the existing module structure.Also applies to: 11-11
tests/test_close_validator_fees_vault.rs (1)
36-36: Test infrastructure update is consistent with the broader refactor.This test now uses
Nonefor the processor, matching the pattern applied across the test suite.tests/test_commit_on_curve.rs (1)
82-82: Test infrastructure update is consistent with the broader refactor.This test now uses
Nonefor the processor, matching the pattern applied across the test suite.tests/test_undelegate_on_curve.rs (1)
67-67: Test infrastructure update is consistent with the broader refactor.This test now uses
Nonefor the processor, matching the pattern applied across the test suite.tests/test_whitelist_validator_for_program.rs (1)
101-101: Test infrastructure update is consistent with the broader refactor.This test now uses
Nonefor the processor, matching the pattern applied across the test suite.tests/test_top_up.rs (1)
264-264: Verify test execution in local environment.The code change is confirmed: all 20 test files consistently use
Noneas the processor argument inProgramTest::new(), includingtests/test_top_up.rs:264. The default entrypoint is properly configured insrc/entrypoint.rswith standard Solana defaults.However, the sandbox environment cannot execute
cargo testto verify that tests pass with this configuration. Please run the following locally to confirm all tests pass:cargo test --test test_top_up -- --nocapture cargo test --test '*' -- --nocaptureEnsure no instruction variants tested in
test_top_up.rsfail with the default processor handling.tests/test_init_validator_fees_vault.rs (2)
52-52: Undocumented architectural change: processor removal from tests.The PR changes
ProgramTest::newto useNoneinstead of an explicit processor across multiple test files, but the PR objectives focus solely on configurable fees receiver functionality. This systematic change suggests an architectural shift that should be documented.Please clarify:
- Why was the processor argument changed to
Noneacross the test suite?- How does this relate to the fees receiver feature?
- Are there any behavioral differences in how tests execute?
- Should this architectural change be documented separately?
Additionally, the AI-generated summary mentions "dual-path execution model" and "Pinocchio fast path" which don't appear in the PR objectives. This inconsistency should be resolved.
5-5: This review comment is based on incorrect assumptions about the codebase.The actual code shows:
- Line 5 never imported a
processormacro—it only importsBanksClientandProgramTestfromsolana_program_test- Line 52 uses
ProgramTest::new("dlp", dlp::ID, None)withNoneas the processor parameter, which is the standard pattern- No
processor!macro exists anywhere in the test files; grep found zero matches across the entire tests directoryAll test files consistently use this same pattern:
ProgramTest::new()withNoneplusprefer_bpf(true)andprogram_test.start()to load and execute the BPF program. The test logic is sound and executes correctly.Likely an incorrect or invalid review comment.
src/processor/fast/utils/mod.rs (1)
1-2: Potential scope creep: "fast" module unrelated to PR objectives.This file introduces new
pdaandrequiresutility modules undersrc/processor/fast/utils/, but the PR objectives make no mention of:
- A "fast path" or "dual-path execution model"
- Pinocchio integration
- Processor architectural changes
The stated PR focus is "configurable fees receiver" with SetFeesReceiver instruction and protocol_claim_fees refactoring. These fast-path utilities appear to be unrelated functionality.
Please clarify:
- Why are fast-path modules being added in a fees-receiver PR?
- Should this be split into a separate PR for better review and version control?
- What is the relationship between fast-path processing and fees receiver configuration?
The AI summary extensively discusses "Pinocchio" and "fast path" but this directly contradicts the PR title and objectives.
src/args/mod.rs (1)
5-5: LGTM: Proper module addition for SetFeesReceiver feature.The addition of the
set_fees_receivermodule and its public re-export properly implements the SetFeesReceiver instruction mentioned in the PR objectives. The implementation follows the existing module pattern consistently.Also applies to: 14-14
src/state/program_config.rs (1)
12-12: LGTM: fees_receiver field addition aligns with PR objectives.The addition of the
fees_receiver: Pubkeyfield properly implements the configurable fees receiver feature described in the PR objectives.tests/test_init_fees_vault.rs (1)
3-3: LGTM: Consistent test infrastructure update.This follows the same pattern as other test files in the PR, removing explicit processor wiring in favor of the default/fast-path approach.
Also applies to: 29-29
src/discriminator.rs (1)
37-38: LGTM: SetFeesReceiver discriminator added correctly.The new discriminator variant follows the existing pattern and uses the next sequential value (16). The
TryFromPrimitivederive macro will automatically generate the necessary conversion implementations.Based on learnings
tests/test_commit_state_from_buffer.rs (1)
14-14: LGTM: Test setup aligned with new processor model.Consistent with the test infrastructure changes across the PR.
Also applies to: 88-88
src/instruction_builder/init_protocol_fees_vault.rs (1)
1-6: LGTM: Program data account added for authorization.The delegation_program_data PDA derivation follows the standard Solana pattern for upgradeable programs and is correctly added as a readonly account. This enables verification of the program's upgrade authority for authorization purposes.
Also applies to: 15-16, 22-22
src/args/set_fees_receiver.rs (1)
1-7: LGTM: Clean and simple argument structure.The
SetFeesReceiverArgsstruct is well-defined with appropriate Borsh serialization support for the new SetFeesReceiver instruction.src/state/delegation_metadata.rs (1)
29-37: LGTM: Serialized size calculation is correct.The
serialized_sizemethod accurately computes the Borsh-serialized size:
- Fixed-size fields (discriminator, u64, bool, Pubkey) are correctly sized
- The Vec<Vec> calculation properly accounts for Borsh's length prefixes: 4 bytes for the outer Vec + 4 bytes per inner Vec + data
This helper is useful for calculating rent-exempt balances.
tests/test_undelegate.rs (1)
8-8: LGTM: Test infrastructure updated consistently.This completes the coordinated test suite refactor to use the new processor model.
Also applies to: 79-79
tests/test_lamports_settlement.rs (1)
16-16: Approve test setup refactoring with manual verification required.The code inspection confirms that all 20 test files in the suite uniformly use
ProgramTest::new("dlp", dlp::ID, None)withprefer_bpf(true), establishing consistent default processor wiring throughout the test suite. The imports and setup at lines 16 and 534 are syntactically valid and align with the stated pattern.However, the test execution verification cannot be completed in the sandbox environment. Manual verification is required to confirm that all tests pass with this processor wiring approach before merging.
src/state/utils/discriminator.rs (1)
14-16: LGTM! Good refactoring to use a named constant.The introduction of the
SPACEconstant improves maintainability and makes the discriminator size explicit. The change from a hardcoded array size toSelf::SPACEis a good practice.src/processor/call_handler.rs (2)
93-108: Account ordering changed but semantics preserved.The account collection now iterates over
other_accountsfirst, then chains the escrow-related accounts. While the order has changed, the semantics are preserved (only the escrow account is marked as a signer). This change appears safe but may affect downstream code that depends on account ordering.
112-112: This review comment is based on incorrect assumptions about the code.The review falsely claims that
EXTERNAL_CALL_HANDLER_DISCRIMINATORwas previously prepended to the instruction data. However:
EXTERNAL_CALL_HANDLER_DISCRIMINATORdoes not exist in the codebase- No prepending logic has ever existed—line 112 simply passes
args.datadirectly, as documented- The test suite already includes discriminators within the
CallHandlerArgs.datafield (seeCOMMIT_HANDLER_DISCRIMINATOR,UNDELEGATE_HANDLER_DISCRIMINATORin tests), confirming this is the intended design- The
CallHandlerArgs.datafield is documented as intentionally accepting flexible instruction formats- Git history shows no recent changes to discriminator handling
This is not a breaking change; it is the correct and long-standing design of the call_handler instruction.
Likely an incorrect or invalid review comment.
tests/test_call_handler.rs (2)
222-222: Test harness updated to remove explicit processor.The change from
processor!(dlp::process_instruction)toNonealigns with the broader PR pattern of removing explicit processor hooks from tests. This likely relies on the program's default entrypoint.
27-28: No conflicts found between test and production discriminators; verification complete.The test discriminators
[1, 0, 1, 0]and[1, 0, 2, 0]are completely distinct from production discriminators like[196, 28, 41, 206, 48, 37, 51, 167]and[90, 147, 75, 178, 85, 88, 4, 137]. They are used exclusively within the test file for mocking test handlers and do not interfere with production code. The 4-byte size is intentional and appropriate for test mocking, while production correctly uses 8-byte discriminators per Anchor standards.tests/fixtures/accounts.rs (1)
141-144: LGTM! Test fixture correctly updated for new ProgramConfig structure.The function signature and initialization correctly reflect the addition of the
fees_receiverfield toProgramConfig. This change is consistent with the broader PR objective.tests/test_undelegate_without_commit.rs (1)
79-79: LGTM! Test harness update consistent with PR-wide pattern.The change from explicit processor to
Noneis consistent with other test files in this PR. This relies on the program's default entrypoint rather than an explicit processor hook.src/instruction_builder/protocol_claim_fees.rs (1)
9-16: All call sites updated correctly, butprogramparameter purpose remains undocumented.The function signature change has been properly applied to all test call sites. However, the
programparameter is loaded as a read-only account insrc/processor/protocol_claim_fees.rs:29but not utilized in the processor logic. Add a comment explaining why this parameter is required—whether it's for future validation, protocol consistency, or another reason—to clarify the design intent for maintainers.src/processor/close_validator_fees_vault.rs (1)
18-20: No issues found—instruction builder correctly updated.The instruction builder at
src/instruction_builder/close_validator_fees_vault.rshas been properly updated to match the new account order. The accounts are constructed in the correct sequence (indices 0-4: payer, admin, delegation_program_data, validator_identity, validator_fees_vault_pda), and this order is consistent with the processor's documentation and account destructuring. The test also correctly uses the updated builder.src/processor/whitelist_validator_for_program.rs (1)
75-78: No deployment documentation found in repository; manual verification required.The code at lines 75-78 correctly initializes
fees_receiverto*authority.key. Theset_fees_receiverinstruction exists and can be called to change the receiver (admin-only). Tests intests/test_set_fees_receiver.rsdemonstrate proper usage patterns.However, no deployment documentation exists in the repository (only
README.mdandLICENSE.md; nodocs/directory). The original request to verify that "deployment documentation mentions callingset_fees_receiver" cannot be confirmed, as such documentation is not present.Action: Verify whether deployment/migration documentation should be created with guidance on calling
set_fees_receiverafter initial configuration, or whether the inline code comments and existing tests are considered sufficient for users.src/instruction_builder/set_fees_receiver.rs (1)
15-24: Confirm program-data PDA derivation aligns with processor checksDeriving ProgramData via seeds [program_id] and bpf_loader_upgradeable is conventional; ensure the processor side validates admin against this same address and ordering of accounts matches its expectations. If they ever diverge, admin checks could silently weaken.
tests/test_commit_state_with_program_config.rs (1)
102-102: LGTM: ProgramTest uses BPF entrypointSwitching to ProgramTest::new(..., None) correctly exercises the on-chain entrypoint paths.
tests/test_set_fees_receiver.rs (1)
22-35: Migration scaffold looks goodLocal OldProgramConfig with discriminator traits is a clean way to simulate legacy data.
src/entrypoint.rs (1)
28-31: LGTM: clean fast→slow fallbackFallback to slow_entrypoint when fast path returns None is straightforward and safe given re-deserialization.
src/processor/set_fees_receiver.rs (1)
41-51: Confirm behavior when upgrade authority is NoneIf the program’s upgrade authority is removed, this returns Unauthorized and permanently blocks updates. Is that intended?
Would you like me to add a test that sets ProgramData.upgrade_authority_address = None and asserts Unauthorized?
tests/test_protocol_claim_fees.rs (1)
30-34: Align rent calc with on-chain logicIf the program switches to Rent::get(), update tests to compute min_rent the same way to avoid flakiness.
I can patch tests once you confirm the on-chain change.
src/processor/mod.rs (1)
8-9: Wiring looks goodModule registration and re-exports are consistent.
Also applies to: 14-24
src/processor/init_protocol_fees_vault.rs (2)
56-63: Confirm behavior when upgrade authority is NoneThis will Err(Unauthorized) if ProgramData has no upgrade authority. Acceptable for init?
If you want init to work after authority removal, we can fall back to ProgramConfig.fees_receiver (if present) or reject with a clearer error.
64-79: Ensure create_pda uses Rent::get() (cross-file)If create_pda computes rent via Rent::default(), switch to Rent::get()? for correctness on-chain. Please confirm and I can patch it.
src/pda.rs (1)
161-167: Confirm PDA derivation program-id consistency (fast vs slow).program_config_from_program_id uses crate::id() (solana), while fast-path checks use crate::fast::ID (pinocchio). They currently point to the same address, but any divergence would break PDAs. Consider standardizing on one (e.g., crate::fast::ID in fast-only flows), or add a comment asserting they are identical.
src/lib.rs (1)
64-82: Dispatch wiring looks solid.Fast-path routes expected variants; slow-path includes SetFeesReceiver and ProtocolClaimFees. No issues spotted.
src/processor/utils/loaders.rs (5)
48-51: Use of ProgramError::Immutable for non-writable PDAs — good changeSwitching to Immutable better reflects Solana semantics for non-writable accounts.
97-100: Consistent Immutable on initialized PDA writabilitySame positive change here; consistent error surface helps callers.
116-121: More informative owner-mismatch logsExpanded log context (label, account, owner) will speed up debugging on-chain.
132-133: Immutable for uninitialized-account writabilityCorrect error for non-writable accounts during init paths.
167-168: <!-- [scratchpad_start]
[task] Final verification of review comment for loaders.rs:167-168 change to Immutable
[observations]
- load_account function (line 154) already uses ProgramError::Immutable at line 167 for writability check
- Code pattern is CONSISTENT with:
- Same file: lines 50, 99, 132 (all use Immutable for writable checks)
- src/processor/fast/utils/requires.rs: lines 80, 125, 177 (all use Immutable for writable checks)
- Key validation (line 162) correctly uses InvalidAccountData
- Tests found:
- test_load_account_not_writeable (line 415) - only checks is_err(), NOT specific error type
- test_load_account_bad_key (line 396) - only checks is_err(), NOT specific error type
- test_load_uninitialized_account_not_writeable (line 339) - only checks is_err(), NOT specific error type
- grep search found no tests explicitly checking for InvalidAccountData in writable context
- No doc comments found referencing InvalidAccountData for writability
[analysis]- Code change is already in place and consistent
- Tests are generic (check is_err()) and don't break with this change
- No documentation issues found
- Change aligns with established pattern in codebase
[done]- All concerns verified
- No breaking changes identified
- Change is consistent with other helpers as stated in review
</scratchpad_end] -->Change is consistent with existing patterns; no test updates needed
The code at line 167 already returns
ProgramError::Immutablefor writability validation, matching the pattern used in other helper functions (lines 50, 99, 132) and the fast path utilities. Existing tests only verify error occurrence viais_err()rather than checking specific error types, so they remain compatible. No documentation found that referencesInvalidAccountDatafor this writable-account check.src/processor/fast/finalize.rs (3)
62-67: Validation flow is clear and minimalSigner, ownership, and PDA init checks via require_* are solid and fast-path friendly.
68-86: Early “skip if nothing to finalize” is correct but brittle to error variantsYou only skip when both require_* return InvalidAccountOwner and both PDAs are system-owned+empty. This matches require_initialized_pda behavior today. If future refactors change the error ordering (e.g., return InvalidSeeds earlier), this skip won’t trigger.
Consider a small helper to detect “both PDAs uninitialized” via is_uninitialized_account only, before checking owners, to decouple from error variants. Add a test covering this path.
149-186: Lamport settlement math looks correct and overflow-safeSource/destination selection and checked add/sub are right. Borrow scopes are tight, minimizing RefCell borrow issues.
tests/integration/programs/test-delegation/src/lib.rs (5)
39-43: Explicit DelegateConfig is goodUsing commit_frequency_ms: u32::MAX and passing validator explicitly makes tests deterministic.
52-56: Consistent config in delegate_twoMirrors the single-delegate path; reduces flakiness across validators.
Also applies to: 60-64
192-200: Account constraints are minimal; acceptable for tests but note risksUnchecked accounts (destination_account, counter, escrow_authority) are fine for test programs. If you see intermittent failures, add owner checks or length assertions on counter for clearer errors.
Also applies to: 202-213
79-95: Discriminator contract verified as correctThe discriminator [1, 0, 1, 0] is properly defined and matches across the handler definition (line 79) and test constants (tests/test_call_handler.rs:27). Integration tests confirm the delegation program successfully invokes the handler with this discriminator.
96-116: ****The review comment conflates two unrelated programs. The main program's
cpi_external_undelegate()function correctly usesEXTERNAL_UNDELEGATE_DISCRIMINATOR = [196, 28, 41, 206, 48, 37, 51, 167]when calling external programs via CPI. However, test-delegation is a separate, independent Anchor test program—not a target of the main program's CPI. It legitimately defines its own instruction discriminators (e.g.,[1, 0, 2, 0]for itsundelegate_base_action_handler). These programs are not connected, and there is no discriminator mismatch to fix.Likely an incorrect or invalid review comment.
src/processor/fast/undelegate.rs (7)
88-95: Solid upfront validationsSigner, owned delegated account, and fees vault initializations via require_* are correct.
96-111: Good guard against pending commitsRequiring both commit_state and commit_record to be uninitialized prevents undelegating with pending finalize.
118-125: Owner program consistency checkComparing to delegation_record.owner prevents CPIing to the wrong program.
134-150: Undelegatable flag logicReturning NotUndelegatable when the flag is false is correct and well-logged.
174-193: Undelegate buffer PDA creation looks correctSeeds, bump, payer=validator, and space carry-over match invariants.
232-288: CPI invariants are strong
- Close delegated account first.
- Enforce validator spent exactly min rent.
- Byte-for-byte state equivalence check.
- Return extra lamports precisely.
All arithmetic is checked; good.
309-318: AccountMeta flags for CPIDelegated account: writable, not signer; buffer: writable + signer; payer: writable + signer; system: readonly. This matches typical system Create/Allocate flows.
Optional: add a unit/integration test asserting failure when payer isn’t a signer to catch regressions.
src/processor/fast/utils/requires.rs (3)
62-84: require_pda: error surfaces and writability are rightSeeds first, then Immutable on non-writable is consistent with Solana norms.
98-129: require_uninitialized_account: solid checksSystem owner + empty data + optional writability enforced. Clear logs.
224-244: Validator fees vault PDA check mirrors slow-path logicGood to log both expected and actual keys, and to chain require_initialized_pda for owner + writable.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/test_protocol_claim_fees.rs(3 hunks)tests/test_set_fees_receiver.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_set_fees_receiver.rs (5)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-154)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)tests/test_protocol_claim_fees.rs (1)
setup_program_test_env(58-107)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-20)
tests/test_protocol_claim_fees.rs (4)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-154)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-20)tests/test_set_fees_receiver.rs (1)
setup_program_test_env(148-157)
⏰ 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: install
🔇 Additional comments (6)
tests/test_set_fees_receiver.rs (3)
22-34: LGTM! Clean migration testing structure.The
OldProgramConfigstruct correctly models the pre-migration state without thefees_receiverfield, enabling proper testing of the migration path from the old account structure to the new one.
36-90: Comprehensive test coverage for fees receiver flow.The test properly validates:
- Setting the fees receiver succeeds
- Claiming with an incorrect receiver fails (lines 56-60)
- Claiming with the correct receiver succeeds (lines 62-66)
- Lamport distribution is correct (vault reduced to rent-exempt minimum, receiver gets the fees)
- FeesVault deserializes with the correct stored fees_receiver (lines 81-89)
148-226: Well-structured test setup helpers.The setup functions are cleanly organized:
setup_program_test_env: Creates a normal test environment with initialized FeesVaultsetup_program_test_env_old_fees_vault: Creates a migration test environment with empty vault database_setup_program_test_env: Provides common setup logic with configurable parametersThe conditional logic (lines 197-212) correctly creates either
OldProgramConfigor the newProgramConfigstructure based on themigrateflag, enabling proper migration testing.tests/test_protocol_claim_fees.rs (3)
16-41: Test correctly updated for new protocol_claim_fees API.The test properly reflects the API changes:
- Line 24: Passes both
fees_receiver(admin) andprogram(dlp::ID) to the instruction- Line 25: Transaction only signed by payer (admin no longer needs to sign since they're just the recipient)
- Lines 30-33: Correctly verifies vault is reduced to rent-exempt minimum
- Lines 36-40: Correctly verifies admin received the claimed fees
The lamport calculations are correct: admin receives
LAMPORTS_PER_SOL - min_rentfrom the vault, resulting in a total of2 * LAMPORTS_PER_SOL - min_rent.
43-56: Negative test case properly validates receiver mismatch.This test correctly verifies that
protocol_claim_feesfails when the providedfees_receiverdoesn't match the stored value inFeesVault. This is an important security check that prevents unauthorized fee claims.
58-107: Test setup properly initializes all required accounts.The setup function correctly:
- Creates the admin account with sufficient lamports (lines 64-73)
- Initializes a
FeesVaultwith admin as thefees_receiver, serializes it with the discriminator, and adds it to the test bank (lines 76-91)- Creates the program config account using the test fixture helper (lines 93-103)
All accounts are properly configured with correct owners, lamports, and data.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/processor/close_validator_fees_vault.rs (3)
65-67: Require writable destination before closing the PDA.Make the error explicit if
validator_identityisn’t writable;close_pdawill fail anyway but this yields a clearer, earlier error.// Close the fees vault PDA - close_pda(validator_fees_vault, validator_identity)?; + if !validator_identity.is_writable { + msg!("validator_identity must be writable"); + return Err(ProgramError::InvalidAccountData); + } + close_pda(validator_fees_vault, validator_identity)?;
41-55: Optional: assert the provided ProgramData account matches this program’s ProgramData.If
load_program_upgrade_authoritydoesn’t validate the ProgramData PDA againstcrate::ID, add an explicit key check to prevent spoofed ProgramData.+ // Ensure ProgramData PDA matches this program + let expected_pd = + solana_program::pubkey::Pubkey::find_program_address(&[crate::ID.as_ref()], &solana_program::bpf_loader_upgradeable::id()).0; + if delegation_program_data.key != &expected_pd { + msg!("Invalid ProgramData account for this program"); + return Err(ProgramError::InvalidArgument); + }
41-44: Optional: drop unused payer requirement.
payeris only checked as signer and otherwise unused. Consider removing it (or not requiring signer) in a future breaking revision to reduce friction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/processor/close_validator_fees_vault.rs(1 hunks)tests/test_protocol_claim_fees.rs(3 hunks)tests/test_set_fees_receiver.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_set_fees_receiver.rs (4)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-20)
tests/test_protocol_claim_fees.rs (3)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-20)
⏰ 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 (1)
tests/test_set_fees_receiver.rs (1)
36-55: Good coverage for set/claim and migration.End-to-end flow and vault/receiver assertions look solid.
Also applies to: 62-90
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/processor/protocol_claim_fees.rs(2 hunks)src/processor/set_fees_receiver.rs(1 hunks)src/processor/utils/pda.rs(1 hunks)tests/integration/tests/test-delegation.ts(8 hunks)tests/test_protocol_claim_fees.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_protocol_claim_fees.rs (4)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-20)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)
src/processor/set_fees_receiver.rs (2)
src/processor/utils/loaders.rs (4)
load_initialized_protocol_fees_vault(230-242)load_program(176-188)load_program_upgrade_authority(191-226)load_signer(23-30)src/processor/utils/pda.rs (1)
resize_pda(85-100)
src/processor/protocol_claim_fees.rs (1)
src/processor/utils/loaders.rs (2)
load_account(154-171)load_initialized_protocol_fees_vault(230-242)
🔇 Additional comments (9)
src/processor/utils/pda.rs (1)
91-91: LGTM! Rent sysvar correctly retrieved.The switch from
Rent::default()toRent::get()?ensures accurate rent calculations on-chain by fetching from the sysvar rather than using hardcoded defaults.src/processor/set_fees_receiver.rs (1)
35-93: LGTM! Clean implementation with proper migration flow.The function correctly:
- Validates admin authority via program upgrade authority
- Handles migration from legacy FeesVault structure
- Resizes PDA only when needed with proper system program validation
- Updates and persists the fees receiver
The ordering (resize before setting fees_receiver) ensures atomicity—if resize fails, no state changes are persisted.
src/processor/protocol_claim_fees.rs (1)
36-70: LGTM! Vault-based authorization correctly implemented.The refactored flow properly:
- Loads FeesVault to obtain the configured receiver
- Validates the provided fees_receiver matches the vault config
- Prevents self-receiver aliasing (Line 48-51)
- Uses Rent::get()? for accurate rent calculation (Line 54)
- Safely transfers lamports with overflow/underflow checks
tests/test_protocol_claim_fees.rs (3)
17-41: LGTM! Test correctly validates happy path.The test properly:
- Calls the updated instruction with program ID
- Verifies fees vault retains minimum rent
- Confirms admin receives the transferred fees
43-56: LGTM! Negative test for wrong receiver.This test correctly validates that providing a receiver not matching the vault configuration fails.
58-81: LGTM! Self-receiver guard validated.This test properly exercises the self-receiver guard by first setting fees_receiver to the vault, then attempting to claim, which should fail.
tests/integration/tests/test-delegation.ts (3)
569-594: LGTM! SetFeesReceiver instruction builder correctly implemented.The function properly:
- Computes the fees vault PDA and delegation program data PDA
- Includes all required accounts (admin, fees_vault, delegation_program_data, system_program)
- Constructs the data payload with discriminator (16) and fees_receiver pubkey
50-54: LGTM! Test and instruction structure updated correctly.The initialization and claim instruction builder properly reflect the new vault-based flow with fees_receiver validation.
Also applies to: 596-614
320-333: ****The third account parameter (
_program) in theprocess_protocol_claim_feesprocessor is intentionally unused (prefixed with underscore on line 31 ofsrc/processor/protocol_claim_fees.rs). Since the processor never validates or uses this account, the value passed—whethertestDelegation.programIdorDELEGATION_PROGRAM_ID—has no effect on instruction execution. The concern about passing the "wrong" program ID is therefore not a functional issue.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/tests/test-delegation.ts (1)
593-609: Fix: Data buffer should be 8 bytes, not 9.The Rust implementation converts the discriminator using
num.to_le_bytes()on au64, which produces exactly 8 bytes. The TypeScript test has an extra trailing byte.// Current (incorrect): const data = Buffer.from([12, 0, 0, 0, 0, 0, 0, 0, 0]); // 9 bytes // Should be: const data = Buffer.from([12, 0, 0, 0, 0, 0, 0, 0]); // 8 bytes
♻️ Duplicate comments (1)
src/processor/close_validator_fees_vault.rs (1)
16-20: Docs out of sync with code: fix account list/order and writable flags.Code expects 5 accounts
[payer, admin, delegation_program_data, validator_identity, validator_fees_vault], but docs list 4 and mark validator accounts read‑only. This will break clients building tx metas from docs.Apply:
/// Accounts: /// -/// 0; `[signer]` payer -/// 1; `[signer]` admin that is the program upgrade authority -/// 2; `[]` validator_identity -/// 3; `[]` validator_fees_vault_pda +/// 0; `[signer]` payer +/// 1; `[signer]` program admin (upgrade authority) +/// 2; `[]` delegation program data (UpgradeableLoader ProgramData for this program) +/// 3; `[writable]` validator_identity (lamports recipient) +/// 4; `[writable]` validator_fees_vault_pda @@ -/// - validator admin need to be signer since the existence of the validator fees vault +/// - program admin must be a signer since the existence of the validator fees vault /// is used as proof later that the validator is whitelisted -/// - validator fees vault is closed +/// - validator fees vault is closedAlso ensure clients pass validator_identity and validator_fees_vault as writable; otherwise
close_pdawill fail at runtime when mutating lamports.Also applies to: 34-41, 56-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/instruction_builder/protocol_claim_fees.rs(1 hunks)src/processor/close_validator_fees_vault.rs(1 hunks)src/processor/protocol_claim_fees.rs(2 hunks)tests/integration/tests/test-delegation.ts(8 hunks)tests/test_protocol_claim_fees.rs(3 hunks)tests/test_set_fees_receiver.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T10:43:00.058Z
Learnt from: Dodecahedr0x
PR: magicblock-labs/delegation-program#90
File: tests/test_protocol_claim_fees.rs:30-30
Timestamp: 2025-10-29T10:43:00.058Z
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/protocol_claim_fees.rstests/test_protocol_claim_fees.rstests/test_set_fees_receiver.rs
🧬 Code graph analysis (4)
src/processor/protocol_claim_fees.rs (1)
src/processor/utils/loaders.rs (2)
load_account(154-171)load_initialized_protocol_fees_vault(230-242)
src/instruction_builder/protocol_claim_fees.rs (1)
src/pda.rs (1)
fees_vault_pda(149-151)
tests/test_protocol_claim_fees.rs (4)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-19)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)
tests/test_set_fees_receiver.rs (4)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-19)
⏰ 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 (11)
src/instruction_builder/protocol_claim_fees.rs (1)
9-18: Builder matches processor; fees_receiver correctly writable.Accounts order and writability align with processor expectations; removing admin simplifies the call site. LGTM.
tests/test_set_fees_receiver.rs (1)
165-170: Good rent-floor assertion for FeesVault.Using
Rent::default().minimum_balance(FeesVault::default().size_with_discriminator())is correct for solana_program_test. Based on learningstests/test_protocol_claim_fees.rs (3)
29-34: Correct rent calculation in test context.
Rent::default()is appropriate under ProgramTest; the assertion on vault lamports equals rent floor is precise. Based on learnings
42-51: Nice state validation: FeesVault data checked post-claim.Deserializing and asserting
fees_receiverguards against silent layout drift. LGTM.
68-91: Self-receiver negative case covers aliasing guard.This test aligns with the processor’s
InvalidArgumentearly return. LGTM.src/processor/protocol_claim_fees.rs (1)
52-58: Transfer logic, rent floor, and aliasing guard look solid.
- Uses
Rent::get()?with discriminator size for rent floor.- Prevents self‑transfer aliasing (
InvalidArgument).- Checked arithmetic for lamport moves.
LGTM.
Also applies to: 59-69
tests/integration/tests/test-delegation.ts (5)
12-12: LGTM! Import is used correctly.The
SYSTEM_PROGRAM_IDimport is properly utilized in the newcreateSetFeesReceiverInstructionfunction at line 580.
163-163: LGTM! Explicitly specifies validator.Adding the validator parameter aligns with the PR objectives to explicitly specify validators in delegation test programs.
215-215: LGTM! Proper type conversion.Using
Uint8Array.from()provides proper type safety and aligns with thedata: Uint8Arraytype definition in theCommitAccountInstructionArgsinterface (line 358).Also applies to: 267-267
493-500: LGTM! Correctly adds program data for validation.The addition of
delegationProgramDatato the accounts list enables proper authorization checks. The PDA derivation using BPF_LOADER is correct for accessing program data accounts, and the read-only permissions are appropriate.
566-591: LGTM! Well-structured instruction builder.The function correctly implements the SetFeesReceiver instruction:
- Account list properly includes admin (writable for potential migration costs), feesVault, program data for authorization, and system program
- Data construction correctly encodes discriminator 16 followed by the 32-byte fees receiver public key
- PDA derivations follow established patterns
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/processor/protocol_claim_fees.rs (1)
59-69: Stale inline comment (“admin”).Comment says “Transfer fees to the admin pubkey” but code transfers to the configured fees receiver. Fix the wording.
- // Transfer fees to the admin pubkey + // Transfer fees to the configured fees receiver
♻️ Duplicate comments (1)
tests/test_set_fees_receiver.rs (1)
144-152: fees_receiver account existence in tests (previously discussed).This test relies on program-test behavior; on real clusters, the destination must exist. Leaving as-is for migration focus is fine, but be aware of environment differences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/processor/protocol_claim_fees.rs(1 hunks)tests/test_protocol_claim_fees.rs(3 hunks)tests/test_set_fees_receiver.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T10:43:00.058Z
Learnt from: Dodecahedr0x
PR: magicblock-labs/delegation-program#90
File: tests/test_protocol_claim_fees.rs:30-30
Timestamp: 2025-10-29T10:43:00.058Z
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/protocol_claim_fees.rstests/test_set_fees_receiver.rstests/test_protocol_claim_fees.rs
🧬 Code graph analysis (3)
src/processor/protocol_claim_fees.rs (1)
src/processor/utils/loaders.rs (2)
load_account(154-171)load_initialized_protocol_fees_vault(230-242)
tests/test_set_fees_receiver.rs (5)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/state/fees_vault.rs (1)
discriminator(14-16)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-19)
tests/test_protocol_claim_fees.rs (5)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)tests/test_set_fees_receiver.rs (1)
setup_program_test_env(54-63)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-19)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)
⏰ 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 (3)
tests/test_set_fees_receiver.rs (2)
24-37: Migration scaffold is solid.Using OldProgramConfig with Borsh + discriminator ensures deterministic migration tests. LGTM.
54-69: Setup paths cover both fresh and migration states.Passing empty data for the old vault path and serialized FeesVault for the new path is clear and accurate. LGTM.
tests/test_protocol_claim_fees.rs (1)
18-53: Happy‑path test is accurate and minimal.Asserts for vault rent floor, admin balance, and FeesVault contents are correct and sufficient. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/test_protocol_claim_fees.rs (1)
66-81: Also assert admin balance unchanged after wrong-receiver failure.
You already check vault remains at LAMPORTS_PER_SOL; also assert admin didn’t gain funds.let fees_vault_pda = fees_vault_pda(); - let vault_after = banks.get_account(fees_vault_pda).await.unwrap().unwrap(); - assert_eq!(vault_after.lamports, LAMPORTS_PER_SOL); + let vault_after = banks.get_account(fees_vault_pda).await.unwrap().unwrap(); + assert_eq!(vault_after.lamports, LAMPORTS_PER_SOL); + let admin_after = banks.get_account(_admin.pubkey()).await.unwrap().unwrap(); + assert_eq!(admin_after.lamports, LAMPORTS_PER_SOL);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/processor/protocol_claim_fees.rs(1 hunks)tests/test_protocol_claim_fees.rs(3 hunks)tests/test_set_fees_receiver.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T10:43:00.058Z
Learnt from: Dodecahedr0x
PR: magicblock-labs/delegation-program#90
File: tests/test_protocol_claim_fees.rs:30-30
Timestamp: 2025-10-29T10:43:00.058Z
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:
tests/test_set_fees_receiver.rssrc/processor/protocol_claim_fees.rstests/test_protocol_claim_fees.rs
🧬 Code graph analysis (3)
tests/test_set_fees_receiver.rs (5)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/state/fees_vault.rs (1)
discriminator(14-16)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-19)
src/processor/protocol_claim_fees.rs (1)
src/processor/utils/loaders.rs (2)
load_account(154-171)load_initialized_protocol_fees_vault(230-242)
tests/test_protocol_claim_fees.rs (5)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)tests/test_set_fees_receiver.rs (1)
setup_program_test_env(88-97)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-19)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)
⏰ 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 (4)
src/processor/protocol_claim_fees.rs (1)
34-50: Validation flow looks solid.
PDA check, receiver match, and self-alias guard are correct and minimal. Nice.tests/test_set_fees_receiver.rs (2)
47-78: Good authorization coverage.
Clear negative test asserting DlpError::Unauthorized with precise matching. LGTM.
210-213: Keep Rent::default() in tests.
Correct choice for solana_program_test; Rent::get() isn’t available in test context.Based on learnings
tests/test_protocol_claim_fees.rs (1)
31-53: Tests are tight and aligned with on-chain logic.
Success path, data deserialization, and setup with discriminator bytes look good. Keep using Rent::default() here.Based on learnings
Also applies to: 133-149
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/processor/protocol_claim_fees.rs(1 hunks)tests/test_protocol_claim_fees.rs(3 hunks)tests/test_set_fees_receiver.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T10:43:00.058Z
Learnt from: Dodecahedr0x
PR: magicblock-labs/delegation-program#90
File: tests/test_protocol_claim_fees.rs:30-30
Timestamp: 2025-10-29T10:43:00.058Z
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/protocol_claim_fees.rstests/test_set_fees_receiver.rstests/test_protocol_claim_fees.rs
🧬 Code graph analysis (3)
src/processor/protocol_claim_fees.rs (1)
src/processor/utils/loaders.rs (2)
load_account(154-171)load_initialized_protocol_fees_vault(230-242)
tests/test_set_fees_receiver.rs (4)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-19)
tests/test_protocol_claim_fees.rs (4)
tests/fixtures/accounts.rs (1)
create_program_config_data(141-153)src/pda.rs (2)
fees_vault_pda(149-151)program_config_from_program_id(161-167)src/instruction_builder/protocol_claim_fees.rs (1)
protocol_claim_fees(9-19)src/instruction_builder/set_fees_receiver.rs (1)
set_fees_receiver(13-31)
⏰ 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 (6)
src/processor/protocol_claim_fees.rs (2)
34-46: Validation flow is solid.PDA check, state read, receiver match, and self-receiver rejection cover the critical paths cleanly.
53-61: Rent math + no‑op short‑circuit look correct.Using data_len() and early return prevents underfunding and avoids needless writes.
tests/test_protocol_claim_fees.rs (2)
25-53: Happy path test reads well.End‑to‑end flow and balance assertions match processor semantics.
55-89: Negative path coverage is good.Wrong receiver and state‑unchanged assertions are precise.
tests/test_set_fees_receiver.rs (2)
47-78: Unauthorized setter test is precise.Matching the custom error code hardens auth guarantees.
168-266: Helper covers migration and invariants well.Wrong‑receiver failure, state‑unchanged checks, successful claim, and post‑claim deserialization provide strong coverage.
Problem
Enable permissionless protocol fee claiming.
Solution
Stores the fee receiver in the ProgramConfig. Remove admin checks for claiming. Send protocol fees to the stored fee receiver.
Deploy Notes
The
set_fees_receiverinstruction must be called by the admin when deploying this PR as other methods relying on the ProgramConfig would fail otherwise.Greptile
Summary by CodeRabbit
New Features
Tests
Chores
Updated On: 2025-09-15 16:53:17 UTC
This PR implements configurable fees receiver functionality that enables permissionless protocol fee claiming. The core change modifies the
ProgramConfigstruct to include a newfees_receiverfield, allowing protocol fees to be sent to a designated address instead of always going to the program update authority.Key architectural changes include:
Account Structure Changes:
ProgramConfigwith afees_receiver: Pubkeyfield (+32 bytes)process_set_fees_receiverto handle existing accountsProtocol Fee Claiming Refactor:
protocol_claim_feesprotocol_claim_fees(admin: Pubkey)toprotocol_claim_fees(fees_receiver: Pubkey, program: Pubkey)New Instruction Implementation:
SetFeesReceiverdiscriminator (value 16) and corresponding processorSetFeesReceiverArgsstruct to pass the new fees receiver addressset_fees_receiverinstruction that migrates existing configsIntegration Changes:
@magicblock-labs/ephemeral-rollups-sdkv0.2.10)The change enables automatic fee distribution systems and makes the protocol more adaptable for staking program integration. The migration mechanism ensures backward compatibility during deployment by detecting old account formats and upgrading them with default values.
Confidence score: 3/5
src/processor/set_fees_receiver.rsand the updated fee claiming mechanism insrc/processor/protocol_claim_fees.rs