Draft
Conversation
Bootstrap spec-driven development tooling and define governance principles for light-protocol covering security, testing, zero-copy, branch naming, and repository structure conventions. Entire-Checkpoint: c733e3762c37
Contributor
📝 WalkthroughWalkthroughAdds a per-tree fee reimbursement flow: fee/rent helpers and cap, a new Changes
Sequence Diagram(s)sequenceDiagram
actor Forester
participant Registry as Registry Program
participant AccountComp as Account-Compression
participant Tree as Merkle Tree/Queue
participant FeeRecipient as Fee Recipient
Forester->>Registry: claim_fees(merkle_tree, fee_recipient, bump)
Registry->>Registry: Validate forester epoch PDA & protocol_fee_recipient
Registry->>AccountComp: CPI claim_fees (with CPI authority seeds)
AccountComp->>Tree: Read discriminator, lamports, fields
AccountComp->>AccountComp: Parse type, validate authority
AccountComp->>AccountComp: Compute rent_exemption & claimable_excess
alt claimable_excess > 0
AccountComp->>Tree: Decrease lamports on tree
AccountComp->>FeeRecipient: Transfer claimable_excess
end
AccountComp-->>Registry: Return success
Registry-->>Forester: Claim complete
sequenceDiagram
actor Forester
participant Registry as Registry Program
participant AccountComp as Account-Compression
participant Reimbursement as Reimbursement PDA
participant Authority as Authority Signer
Forester->>Registry: batch_append / batch_nullify / nullify
Registry->>Registry: Read network_fee from tree metadata
Registry->>AccountComp: CPI mutate tree (batch_append/nullify/nullify)
AccountComp-->>Registry: CPI success
rect rgba(100, 150, 200, 0.5)
Note over Registry,Reimbursement: Post-CPI reimbursement handling
end
alt network_fee >= FORESTER_REIMBURSEMENT_CAP
Registry->>Reimbursement: Transfer capped amount from Authority (system transfer)
else network_fee > FORESTER_REIMBURSEMENT_CAP
Registry->>Reimbursement: Transfer excess = network_fee - cap
end
Registry-->>Forester: Operation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 (2)
sdk-libs/program-test/src/registry_sdk.rs (1)
457-469:⚠️ Potential issue | 🟠 MajorSet a non-default
protocol_fee_recipientin test config.Line 468 sets
protocol_fee_recipienttoPubkey::default(), but the claim-fee wrapper rejects that value. Any test path that exercisesclaim_feeswith this test config will fail withInvalidFeeRecipient.Suggested fix
-pub fn protocol_config_for_tests() -> ProtocolConfig { +pub fn protocol_config_for_tests(protocol_fee_recipient: Pubkey) -> ProtocolConfig { ProtocolConfig { @@ - protocol_fee_recipient: Pubkey::default(), + protocol_fee_recipient, address_network_fee: 10000, @@ - let protocol_config = protocol_config_for_tests(); + let protocol_config = protocol_config_for_tests(*forester_authority);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/program-test/src/registry_sdk.rs` around lines 457 - 469, protocol_config_for_tests currently sets ProtocolConfig.protocol_fee_recipient to Pubkey::default(), which triggers InvalidFeeRecipient when exercising claim_fees; update protocol_config_for_tests to assign a non-default test public key (for example generate a unique test Pubkey or use a known test key) to protocol_fee_recipient so the claim-fee wrapper accepts it—modify the ProtocolConfig construction in protocol_config_for_tests to use that non-default Pubkey.programs/registry/src/account_compression_cpi/sdk.rs (1)
48-60:⚠️ Potential issue | 🟠 MajorThe SDK still leaves freshly initialized trees missing a required PDA.
The work-instruction builders now always wire
reimbursement_pda, but the existing tree-init flow in this module still only builds the tree/queue initialization. That means callers following the oldinitialize_* -> nullify/batch_*sequence will now fail on the first work instruction unless they discover and insertcreate_init_reimbursement_pda_instructionthemselves. Fold this into the high-level setup path, or make the extra step impossible to miss.Also applies to: 374-391, 432-477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/registry/src/account_compression_cpi/sdk.rs` around lines 48 - 60, The nullify/batch SDK now expects a reimbursement PDA (computed via get_reimbursement_pda) but the tree-init flow in this module doesn't create it, causing freshly initialized trees to miss that required PDA; update the high-level setup path (the tree/queue initialization functions that build NullifyLeaves and similar account structs) to also create and wire the reimbursement PDA by invoking the same create_init_reimbursement_pda_instruction (or equivalent initialization) and passing the produced reimbursement_pda into NullifyLeaves (and the other account builders referenced around the other locations using get_reimbursement_pda), so callers following initialize_* -> nullify/batch_* won't need to add the extra step manually.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 292-296: Replace the truncated crate name
"light-compressed-accoun" with the correct "light-compressed-account" in all
occurrences in the document; specifically update the two technology list entries
and the "Recent Changes" bullet that reference "light-compressed-accoun" so they
read "light-compressed-account".
- Around line 291-295: The MD022 lint fails because the headings "## Active
Technologies" and "## Recent Changes" need surrounding blank lines; update the
markdown so there is a blank line before and after each of those heading lines
(i.e., insert an empty line above "## Active Technologies" and ensure an empty
line between its content and "## Recent Changes", and add a blank line after "##
Recent Changes" as needed) to satisfy blanks-around-headings.
In `@program-libs/merkle-tree-metadata/src/fee.rs`:
- Around line 139-142: Reformat the assert_eq! invocation to match rustfmt/lint
style: run rustfmt or manually reflow the assert for the
hardcoded_rent_exemption(large) check so the arguments and trailing
comma/spacing follow the project's formatting conventions (target the assert_eq!
that compares hardcoded_rent_exemption(large) to Some((large + 128) * 6960) and
adjust spacing/line breaks accordingly).
In `@programs/registry/src/account_compression_cpi/claim_fees.rs`:
- Around line 9-27: ClaimFeesWrapper currently omits the standard wrapper
accounts; update its struct to match the common wrapper shape by adding the
missing log_wrapper and target_account fields while keeping the existing
optional registered_forester_pda (mutable), authority: Signer, cpi_authority PDA
with seeds, registered_program_pda: AccountInfo, account_compression_program:
Program<AccountCompression>, and merkle_tree_or_queue and fee_recipient muts;
ensure log_wrapper uses the same type used across other wrappers (the event/log
wrapper type in this codebase) and target_account is mutable AccountInfo so the
CPI plumbing stays uniform for ClaimFeesWrapper.
In `@programs/registry/src/account_compression_cpi/mod.rs`:
- Line 3: The module `claim_fees` is declared but not re-exported; update
`account_compression_cpi/mod.rs` to both declare and re-export the wrapper by
adding a `pub use claim_fees::*;` alongside the existing `pub mod claim_fees;`,
and then ensure `lib.rs` imports the wrapper (e.g., `use
account_compression_cpi::*;` or the specific items) so the new wrapper follows
the pattern required by `account_compression_cpi` exports.
In `@programs/registry/src/account_compression_cpi/nullify.rs`:
- Around line 32-39: The reimbursement_pda and system_program accounts are
currently required but only used when network_fee > FORESTER_REIMBURSEMENT_CAP,
breaking zero-fee nullifies; make these accounts optional: change the account
struct fields reimbursement_pda: Account<'info, ReimbursementPda> and
system_program: Program<'info, System> to Option<Account<'info,
ReimbursementPda>> and Option<Program<'info, System>> (and remove/guard the
#[account(...seeds...)] constraint when None), then update the nullify handler
(the function that reads network_fee and compares to FORESTER_REIMBURSEMENT_CAP)
to only access or perform CPI/seed-derived PDA operations when
reimbursement_pda.is_some() (i.e., when network_fee >
FORESTER_REIMBURSEMENT_CAP), and ensure you only pass system_program.unwrap()
into any CPI when reimbursement_pda is present.
In `@programs/registry/src/errors.rs`:
- Around line 45-46: The RegistryError enum variant name was changed to
InvalidFeeRecipient but the CPI wrapper in
account_compression_cpi::claim_fees.rs still expects
RegistryError::FeeRecipientDoesNotMatchProtocolConfig; make them consistent by
renaming the enum variant back to FeeRecipientDoesNotMatchProtocolConfig (and
keep/update its #[msg(...)] string accordingly) OR update the wrapper to
reference RegistryError::InvalidFeeRecipient everywhere it’s used; ensure the
symbol RegistryError::FeeRecipientDoesNotMatchProtocolConfig (or the new name)
matches across both RegistryError and the claim_fees CPI wrapper.
In `@programs/registry/src/lib.rs`:
- Around line 813-824: The handler claim_fees currently duplicates forester
gating by calling ForesterEpochPda::check_forester_in_program and manually
rejecting None; instead parse the target metadata/queue key from ctx.accounts
(e.g. merkle_tree_or_queue), determine work units as other handlers do, call the
shared check_forester() helper with the parsed metadata/queue and authority
(replacing the direct ForesterEpochPda::check_forester_in_program and None
branch on registered_forester_pda), and only after check_forester() succeeds
call process_claim_fees_wrapper(&ctx, bump); keep Context<ClaimFeesWrapper>,
registered_forester_pda and process_claim_fees_wrapper references consistent
with existing wrapper handler patterns.
In `@programs/registry/src/protocol_config/state.rs`:
- Around line 63-64: The struct initialization uses Pubkey::default() for
protocol_fee_recipient which effectively disables fee distribution until
patched; change the constructors that set protocol_fee_recipient (the places
using Pubkey::default()—seen next to address_network_fee) to initialize
protocol_fee_recipient to a real, explicit recipient Pubkey (e.g., the
configured treasury/fee-collector key or a module constant) or alternatively
change the field to Option<Pubkey> and make the zero-pubkey/migration behavior
explicit and validated at runtime; update all occurrences (including the other
instance noted around lines 86–87) and ensure any callers/validators of
protocol_config enforce non-empty recipient or handle None.
In `@sdk-libs/program-test/src/indexer/test_indexer.rs`:
- Around line 1735-1750: The current setup unconditionally calls
rpc.create_and_send_transaction(...).await.unwrap() for the
create_init_reimbursement_pda_instruction, which panics if the PDA already
exists; change this to handle the RPC result instead: call
rpc.create_and_send_transaction for the instruction returned by
create_init_reimbursement_pda_instruction(self.payer.pubkey(),
merkle_tree_keypair.pubkey()), inspect the returned Result/Err and if the error
indicates the account is "already in use" (Anchor/ RPC error text or specific
error code) treat it as success and continue, otherwise propagate or return the
error; reference the create_init_reimbursement_pda_instruction,
rpc.create_and_send_transaction, self.payer and merkle_tree_keypair symbols to
locate and update the call site.
In `@sdk-libs/program-test/src/program_test/light_program_test.rs`:
- Line 7: The import list in light_program_test.rs includes an unused symbol
`Rpc` causing CI failure; remove `Rpc` from the `use
rpc::{merkle_tree::MerkleTreeExt, Rpc, RpcError}` statement so it only imports
`merkle_tree::MerkleTreeExt` and `RpcError` (or reorder to
`rpc::{merkle_tree::MerkleTreeExt, RpcError}`) and ensure no other references to
`Rpc` remain in the file.
- Around line 417-419: The call to context.create_and_send_transaction(...) is
currently ignoring the Result and can hide setup failures for the reimbursement
PDA, so update the setup in LightProgramTest to propagate the error instead of
swallowing it: replace the ignored let _ =
context.create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer]).await
with code that returns or uses the Result (e.g., ? or .expect) so failures
surface to test harness, or explicitly match the Result and only ignore the
known idempotent error case while returning any other Err; reference
context.create_and_send_transaction, payer, and the LightProgramTest setup when
making the change.
---
Outside diff comments:
In `@programs/registry/src/account_compression_cpi/sdk.rs`:
- Around line 48-60: The nullify/batch SDK now expects a reimbursement PDA
(computed via get_reimbursement_pda) but the tree-init flow in this module
doesn't create it, causing freshly initialized trees to miss that required PDA;
update the high-level setup path (the tree/queue initialization functions that
build NullifyLeaves and similar account structs) to also create and wire the
reimbursement PDA by invoking the same create_init_reimbursement_pda_instruction
(or equivalent initialization) and passing the produced reimbursement_pda into
NullifyLeaves (and the other account builders referenced around the other
locations using get_reimbursement_pda), so callers following initialize_* ->
nullify/batch_* won't need to add the extra step manually.
In `@sdk-libs/program-test/src/registry_sdk.rs`:
- Around line 457-469: protocol_config_for_tests currently sets
ProtocolConfig.protocol_fee_recipient to Pubkey::default(), which triggers
InvalidFeeRecipient when exercising claim_fees; update protocol_config_for_tests
to assign a non-default test public key (for example generate a unique test
Pubkey or use a known test key) to protocol_fee_recipient so the claim-fee
wrapper accepts it—modify the ProtocolConfig construction in
protocol_config_for_tests to use that non-default Pubkey.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fa805eb-80b4-4ae9-a286-fcfe7b535b78
⛔ Files ignored due to path filters (24)
.specify/init-options.jsonis excluded by none and included by none.specify/memory/constitution.mdis excluded by none and included by none.specify/scripts/bash/check-prerequisites.shis excluded by none and included by none.specify/scripts/bash/common.shis excluded by none and included by none.specify/scripts/bash/create-new-feature.shis excluded by none and included by none.specify/scripts/bash/setup-plan.shis excluded by none and included by none.specify/scripts/bash/update-agent-context.shis excluded by none and included by none.specify/templates/agent-file-template.mdis excluded by none and included by none.specify/templates/checklist-template.mdis excluded by none and included by none.specify/templates/constitution-template.mdis excluded by none and included by none.specify/templates/plan-template.mdis excluded by none and included by none.specify/templates/spec-template.mdis excluded by none and included by none.specify/templates/tasks-template.mdis excluded by none and included by noneCargo.lockis excluded by!**/*.lockand included by noneprogram-tests/registry-test/Cargo.tomlis excluded by none and included by noneprogram-tests/registry-test/tests/test_fee_distribution.rsis excluded by none and included by noneprogram-tests/registry-test/tests/tests.rsis excluded by none and included by nonespecs/001-tree-fee-distribution/contracts/instructions.mdis excluded by none and included by nonespecs/001-tree-fee-distribution/data-model.mdis excluded by none and included by nonespecs/001-tree-fee-distribution/plan.mdis excluded by none and included by nonespecs/001-tree-fee-distribution/quickstart.mdis excluded by none and included by nonespecs/001-tree-fee-distribution/research.mdis excluded by none and included by nonespecs/001-tree-fee-distribution/spec.mdis excluded by none and included by nonespecs/001-tree-fee-distribution/tasks.mdis excluded by none and included by none
📒 Files selected for processing (23)
CLAUDE.mdprogram-libs/merkle-tree-metadata/src/fee.rsprograms/account-compression/src/errors.rsprograms/account-compression/src/instructions/batch_update_address_tree.rsprograms/account-compression/src/instructions/claim_fees.rsprograms/account-compression/src/instructions/mod.rsprograms/account-compression/src/instructions/update_address_merkle_tree.rsprograms/account-compression/src/lib.rsprograms/registry/src/account_compression_cpi/batch_append.rsprograms/registry/src/account_compression_cpi/batch_nullify.rsprograms/registry/src/account_compression_cpi/claim_fees.rsprograms/registry/src/account_compression_cpi/mod.rsprograms/registry/src/account_compression_cpi/nullify.rsprograms/registry/src/account_compression_cpi/sdk.rsprograms/registry/src/errors.rsprograms/registry/src/fee_reimbursement/initialize.rsprograms/registry/src/fee_reimbursement/mod.rsprograms/registry/src/fee_reimbursement/state.rsprograms/registry/src/lib.rsprograms/registry/src/protocol_config/state.rssdk-libs/program-test/src/indexer/test_indexer.rssdk-libs/program-test/src/program_test/light_program_test.rssdk-libs/program-test/src/registry_sdk.rs
Comment on lines
+291
to
+295
| ## Active Technologies | ||
| - Rust (Solana BPF target), Anchor framework for account-compression and registry programs + anchor-lang, light-batched-merkle-tree, light-merkle-tree-metadata, light-account-checks, light-compressed-accoun (001-tree-fee-distribution) | ||
| - Solana on-chain accounts (tree/queue accounts owned by account-compression, PDA owned by registry) (001-tree-fee-distribution) | ||
|
|
||
| ## Recent Changes |
Contributor
There was a problem hiding this comment.
Fix markdownlint MD022 around the new headings.
Line 291 and Line 295 need surrounding blank lines to satisfy blanks-around-headings and keep docs lint-clean.
Suggested fix
+
## Active Technologies
- Rust (Solana BPF target), Anchor framework for account-compression and registry programs + anchor-lang, light-batched-merkle-tree, light-merkle-tree-metadata, light-account-checks, light-compressed-accoun (001-tree-fee-distribution)
- Solana on-chain accounts (tree/queue accounts owned by account-compression, PDA owned by registry) (001-tree-fee-distribution)
+
## Recent Changes
- 001-tree-fee-distribution: Added Rust (Solana BPF target), Anchor framework for account-compression and registry programs + anchor-lang, light-batched-merkle-tree, light-merkle-tree-metadata, light-account-checks, light-compressed-accoun🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 291-291: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 295-295: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 291 - 295, The MD022 lint fails because the headings
"## Active Technologies" and "## Recent Changes" need surrounding blank lines;
update the markdown so there is a blank line before and after each of those
heading lines (i.e., insert an empty line above "## Active Technologies" and
ensure an empty line between its content and "## Recent Changes", and add a
blank line after "## Recent Changes" as needed) to satisfy
blanks-around-headings.
Comment on lines
+292
to
+296
| - Rust (Solana BPF target), Anchor framework for account-compression and registry programs + anchor-lang, light-batched-merkle-tree, light-merkle-tree-metadata, light-account-checks, light-compressed-accoun (001-tree-fee-distribution) | ||
| - Solana on-chain accounts (tree/queue accounts owned by account-compression, PDA owned by registry) (001-tree-fee-distribution) | ||
|
|
||
| ## Recent Changes | ||
| - 001-tree-fee-distribution: Added Rust (Solana BPF target), Anchor framework for account-compression and registry programs + anchor-lang, light-batched-merkle-tree, light-merkle-tree-metadata, light-account-checks, light-compressed-accoun |
Contributor
There was a problem hiding this comment.
Correct truncated crate name in the technology list.
light-compressed-accoun appears misspelled/truncated in both entries; this should be light-compressed-account.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 295-295: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 292 - 296, Replace the truncated crate name
"light-compressed-accoun" with the correct "light-compressed-account" in all
occurrences in the document; specifically update the two technology list entries
and the "Recent Changes" bullet that reference "light-compressed-accoun" so they
read "light-compressed-account".
Comment on lines
+9
to
+27
| pub struct ClaimFeesWrapper<'info> { | ||
| /// CHECK: only eligible foresters can claim fees. | ||
| #[account(mut)] | ||
| pub registered_forester_pda: Option<Account<'info, ForesterEpochPda>>, | ||
| pub authority: Signer<'info>, | ||
| /// CHECK: (seed constraints) used to invoke account compression program via cpi. | ||
| #[account(seeds = [CPI_AUTHORITY_PDA_SEED], bump)] | ||
| pub cpi_authority: AccountInfo<'info>, | ||
| /// CHECK: (account compression program) group access control. | ||
| pub registered_program_pda: AccountInfo<'info>, | ||
| pub account_compression_program: Program<'info, AccountCompression>, | ||
| /// CHECK: (account compression program) the tree/queue to claim from. | ||
| #[account(mut)] | ||
| pub merkle_tree_or_queue: AccountInfo<'info>, | ||
| pub protocol_config_pda: Account<'info, ProtocolConfigPda>, | ||
| /// CHECK: must match protocol_config.protocol_fee_recipient. | ||
| #[account(mut)] | ||
| pub fee_recipient: AccountInfo<'info>, | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the wrapper account set consistent.
ClaimFeesWrapper is the first registry account-compression wrapper here that drops log_wrapper, which forces special-case account plumbing for one instruction in an otherwise uniform wrapper family. Keep the standard wrapper context even if this CPI does not emit today. As per coding guidelines, Wrapper instruction Account context must include: optional registered_forester_pda (mutable), authority signer, cpi_authority PDA with seeds, registered_program_pda, account_compression_program, log_wrapper for events, and target_account (mutable).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/registry/src/account_compression_cpi/claim_fees.rs` around lines 9 -
27, ClaimFeesWrapper currently omits the standard wrapper accounts; update its
struct to match the common wrapper shape by adding the missing log_wrapper and
target_account fields while keeping the existing optional
registered_forester_pda (mutable), authority: Signer, cpi_authority PDA with
seeds, registered_program_pda: AccountInfo, account_compression_program:
Program<AccountCompression>, and merkle_tree_or_queue and fee_recipient muts;
ensure log_wrapper uses the same type used across other wrappers (the event/log
wrapper type in this codebase) and target_account is mutable AccountInfo so the
CPI plumbing stays uniform for ClaimFeesWrapper.
| @@ -1,5 +1,6 @@ | |||
| pub mod batch_append; | |||
| pub mod batch_nullify; | |||
| pub mod claim_fees; | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add the wrapper re-export for claim_fees.
This module declares claim_fees but does not re-export it, which breaks the required wrapper export pattern.
Suggested fix
pub mod claim_fees;
+pub use claim_fees::*;As per coding guidelines: programs/registry/src/account_compression_cpi/mod.rs: “Export new wrapper modules in account_compression_cpi/mod.rs using pub mod new_operation; and pub use new_operation::*;, then import in lib.rs”.
📝 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
| pub mod claim_fees; | |
| pub mod claim_fees; | |
| pub use claim_fees::*; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/registry/src/account_compression_cpi/mod.rs` at line 3, The module
`claim_fees` is declared but not re-exported; update
`account_compression_cpi/mod.rs` to both declare and re-export the wrapper by
adding a `pub use claim_fees::*;` alongside the existing `pub mod claim_fees;`,
and then ensure `lib.rs` imports the wrapper (e.g., `use
account_compression_cpi::*;` or the specific items) so the new wrapper follows
the pattern required by `account_compression_cpi` exports.
Comment on lines
+813
to
+824
| pub fn claim_fees(ctx: Context<ClaimFeesWrapper>, bump: u8) -> Result<()> { | ||
| if let Some(forester_pda) = ctx.accounts.registered_forester_pda.as_mut() { | ||
| ForesterEpochPda::check_forester_in_program( | ||
| forester_pda, | ||
| &ctx.accounts.authority.key(), | ||
| &ctx.accounts.merkle_tree_or_queue.key(), | ||
| 0, | ||
| )?; | ||
| } else { | ||
| return err!(RegistryError::InvalidForester); | ||
| } | ||
| process_claim_fees_wrapper(&ctx, bump) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Route claim_fees through check_forester().
Lines 815-823 reimplement the gate with check_forester_in_program and an explicit None rejection, so this handler can drift from the rest of the registry’s auth/work-accounting rules. Parse the target metadata/queue key here and reuse check_forester() before the CPI. As per coding guidelines, in wrapper instruction handler in lib.rs, load account metadata, determine work units, call check_forester(), then call the processing function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/registry/src/lib.rs` around lines 813 - 824, The handler claim_fees
currently duplicates forester gating by calling
ForesterEpochPda::check_forester_in_program and manually rejecting None; instead
parse the target metadata/queue key from ctx.accounts (e.g.
merkle_tree_or_queue), determine work units as other handlers do, call the
shared check_forester() helper with the parsed metadata/queue and authority
(replacing the direct ForesterEpochPda::check_forester_in_program and None
branch on registered_forester_pda), and only after check_forester() succeeds
call process_claim_fees_wrapper(&ctx, bump); keep Context<ClaimFeesWrapper>,
registered_forester_pda and process_claim_fees_wrapper references consistent
with existing wrapper handler patterns.
Comment on lines
+63
to
64
| protocol_fee_recipient: Pubkey::default(), | ||
| address_network_fee: 10000, |
Contributor
There was a problem hiding this comment.
Pubkey::default() is not a safe default for the new fee recipient.
The new claim-fees path reads protocol_config.protocol_fee_recipient at runtime, so any config created from these constructors starts with fee distribution effectively disabled until another instruction patches the field. Please initialize this to a real recipient during config creation, or make the zero-pubkey rollout/migration behavior explicit.
Also applies to: 86-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/registry/src/protocol_config/state.rs` around lines 63 - 64, The
struct initialization uses Pubkey::default() for protocol_fee_recipient which
effectively disables fee distribution until patched; change the constructors
that set protocol_fee_recipient (the places using Pubkey::default()—seen next to
address_network_fee) to initialize protocol_fee_recipient to a real, explicit
recipient Pubkey (e.g., the configured treasury/fee-collector key or a module
constant) or alternatively change the field to Option<Pubkey> and make the
zero-pubkey/migration behavior explicit and validated at runtime; update all
occurrences (including the other instance noted around lines 86–87) and ensure
any callers/validators of protocol_config enforce non-empty recipient or handle
None.
Comment on lines
+1735
to
+1750
| // Initialize reimbursement PDA for the state tree. | ||
| // Required because BatchAppend, BatchNullify, and NullifyLeaves | ||
| // require the reimbursement_pda account. | ||
| { | ||
| let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( | ||
| self.payer.pubkey(), | ||
| merkle_tree_keypair.pubkey(), | ||
| ); | ||
| rpc.create_and_send_transaction( | ||
| &[ix], | ||
| &self.payer.pubkey(), | ||
| &[&self.payer], | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid panicking on reimbursement PDA init when account already exists.
This unwrap() can fail on repeated setup runs because Anchor init returns “already in use” for existing PDAs. That turns an idempotent setup into a hard panic.
Suggested fix
- {
- let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction(
- self.payer.pubkey(),
- merkle_tree_keypair.pubkey(),
- );
- rpc.create_and_send_transaction(
- &[ix],
- &self.payer.pubkey(),
- &[&self.payer],
- )
- .await
- .unwrap();
- }
+ {
+ let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction(
+ self.payer.pubkey(),
+ merkle_tree_keypair.pubkey(),
+ );
+ if let Err(err) = rpc
+ .create_and_send_transaction(&[ix], &self.payer.pubkey(), &[&self.payer])
+ .await
+ {
+ let msg = err.to_string();
+ if !msg.contains("AccountAlreadyInUse") && !msg.contains("already in use") {
+ panic!("init_reimbursement_pda failed: {err}");
+ }
+ }
+ }📝 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
| // Initialize reimbursement PDA for the state tree. | |
| // Required because BatchAppend, BatchNullify, and NullifyLeaves | |
| // require the reimbursement_pda account. | |
| { | |
| let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( | |
| self.payer.pubkey(), | |
| merkle_tree_keypair.pubkey(), | |
| ); | |
| rpc.create_and_send_transaction( | |
| &[ix], | |
| &self.payer.pubkey(), | |
| &[&self.payer], | |
| ) | |
| .await | |
| .unwrap(); | |
| } | |
| // Initialize reimbursement PDA for the state tree. | |
| // Required because BatchAppend, BatchNullify, and NullifyLeaves | |
| // require the reimbursement_pda account. | |
| { | |
| let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( | |
| self.payer.pubkey(), | |
| merkle_tree_keypair.pubkey(), | |
| ); | |
| if let Err(err) = rpc | |
| .create_and_send_transaction(&[ix], &self.payer.pubkey(), &[&self.payer]) | |
| .await | |
| { | |
| let msg = err.to_string(); | |
| if !msg.contains("AccountAlreadyInUse") && !msg.contains("already in use") { | |
| panic!("init_reimbursement_pda failed: {err}"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk-libs/program-test/src/indexer/test_indexer.rs` around lines 1735 - 1750,
The current setup unconditionally calls
rpc.create_and_send_transaction(...).await.unwrap() for the
create_init_reimbursement_pda_instruction, which panics if the PDA already
exists; change this to handle the RPC result instead: call
rpc.create_and_send_transaction for the instruction returned by
create_init_reimbursement_pda_instruction(self.payer.pubkey(),
merkle_tree_keypair.pubkey()), inspect the returned Result/Err and if the error
indicates the account is "already in use" (Anchor/ RPC error text or specific
error code) treat it as success and continue, otherwise propagate or return the
error; reference the create_init_reimbursement_pda_instruction,
rpc.create_and_send_transaction, self.payer and merkle_tree_keypair symbols to
locate and update the call site.
Comment on lines
+417
to
+419
| let _ = context | ||
| .create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer]) | ||
| .await; |
Contributor
There was a problem hiding this comment.
Don’t swallow reimbursement-PDA setup failures.
Ignoring the result here can return a partially initialized LightProgramTest, which makes later tests fail far away from the real setup error. Propagate the failure, or explicitly ignore only the single idempotent case you expect.
Suggested change
- let _ = context
- .create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer])
- .await;
+ context
+ .create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer])
+ .await?;📝 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
| let _ = context | |
| .create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer]) | |
| .await; | |
| context | |
| .create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer]) | |
| .await?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk-libs/program-test/src/program_test/light_program_test.rs` around lines
417 - 419, The call to context.create_and_send_transaction(...) is currently
ignoring the Result and can hide setup failures for the reimbursement PDA, so
update the setup in LightProgramTest to propagate the error instead of
swallowing it: replace the ignored let _ =
context.create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer]).await
with code that returns or uses the Result (e.g., ? or .expect) so failures
surface to test harness, or explicitly match the Result and only ignore the
known idempotent error case while returning any other Err; reference
context.create_and_send_transaction, payer, and the LightProgramTest setup when
making the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
sdk-libs/program-test/src/indexer/test_indexer.rs (1)
1735-1746:⚠️ Potential issue | 🟠 MajorMake reimbursement PDA setup idempotent.
This
.unwrap()turns a repeated setup run into a panic when the PDA already exists. Treat the “already in use” case as success and only fail on unexpected RPC errors.Suggested fix
{ let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( self.payer.pubkey(), merkle_tree_keypair.pubkey(), ); - rpc.create_and_send_transaction(&[ix], &self.payer.pubkey(), &[&self.payer]) - .await - .unwrap(); + if let Err(err) = rpc + .create_and_send_transaction(&[ix], &self.payer.pubkey(), &[&self.payer]) + .await + { + let msg = err.to_string(); + if !msg.contains("AccountAlreadyInUse") && !msg.contains("already in use") { + panic!("init_reimbursement_pda failed: {err}"); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/program-test/src/indexer/test_indexer.rs` around lines 1735 - 1746, The create_init_reimbursement_pda_instruction call followed by rpc.create_and_send_transaction(...).await.unwrap() will panic if the PDA already exists; change this to handle RPC errors and treat the "already in use" / "account already exists" case as success: call rpc.create_and_send_transaction(...).await, match the Result/Err, inspect the RPC error (string or RpcError variant) for the indicator of an existing account (e.g., "already in use" / "account already exists" / RpcResponseError variant) and silently ignore/return Ok(()) in that case, but propagate or panic on any other error; apply this change around the create_init_reimbursement_pda_instruction invocation using the same payer and merkle_tree_keypair identifiers.programs/registry/src/account_compression_cpi/mod.rs (1)
4-4:⚠️ Potential issue | 🟠 MajorRe-export
claim_feesfrom the module root.Declaring the wrapper without
pub use claim_fees::*;leaves it outside the standardaccount_compression_cpiexport surface.Suggested fix
pub mod claim_fees; +pub use claim_fees::*;As per coding guidelines:
programs/registry/src/account_compression_cpi/mod.rs: “Export new wrapper modules inaccount_compression_cpi/mod.rsusingpub mod new_operation;andpub use new_operation::*;, then import inlib.rs”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/registry/src/account_compression_cpi/mod.rs` at line 4, The module wrapper declares pub mod claim_fees but does not re-export its symbols, leaving them out of the account_compression_cpi public surface; fix by adding a re-export line (pub use claim_fees::*;) in the account_compression_cpi mod (the same file containing pub mod claim_fees;) so all claim_fees symbols are exposed, and then ensure this module is imported from lib.rs per the project convention.programs/registry/src/account_compression_cpi/claim_fees.rs (1)
9-28:⚠️ Potential issue | 🟠 MajorAdd the standard
log_wrapperaccount to this wrapper.
claim_feesis now the odd one out in the registry CPI wrappers, so callers/builders need special-case account plumbing for this instruction.Suggested fix
/// CHECK: (account compression program) group access control. pub registered_program_pda: AccountInfo<'info>, pub account_compression_program: Program<'info, AccountCompression>, + /// CHECK: (account compression program) when emitting event. + pub log_wrapper: UncheckedAccount<'info>, /// CHECK: (account compression program) the tree/queue to claim from. #[account(mut)] pub merkle_tree_or_queue: AccountInfo<'info>,Based on learnings: Wrapper instruction Account context must include: optional
registered_forester_pda(mutable),authoritysigner,cpi_authorityPDA with seeds,registered_program_pda,account_compression_program,log_wrapperfor events, andtarget_account(mutable).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/registry/src/account_compression_cpi/claim_fees.rs` around lines 9 - 28, ClaimFeesWrapper is missing the standard log_wrapper (for event emission) and the target_account (the account-compression target to claim into); update the ClaimFeesWrapper struct (the one defining registered_forester_pda, authority, cpi_authority, registered_program_pda, account_compression_program, merkle_tree_or_queue, protocol_config_pda, fee_recipient) to include a mutable log_wrapper field (matching the other CPI wrappers' LogWrapper account type) and a mutable target_account/target (matching the other wrappers' account-compression target type or AccountInfo) with the same CHECK/mut constraints as the other registry CPI wrappers so callers/builders no longer need special casing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/account-compression/src/instructions/claim_fees.rs`:
- Around line 209-220: The current .unwrap_or(0) on compute_claimable_excess
silently masks failures (e.g., overflows) as zero excess; change this so callers
can distinguish errors: either propagate the computation error (return an Err
from the handler where compute_claimable_excess is called in claim_fees.rs) by
replacing .unwrap_or(0) with ? (or map the Option/Result into a ProgramError)
or, if you must keep a safe fallback, log the failure before treating it as zero
(use the program logger/msg! or process_logger to record the error and inputs
such as ctx.accounts.merkle_tree_or_queue.lamports(), rent_exemption,
params.rollover_fee, params.capacity, params.next_index) so we don’t lose
observability while preserving safety.
---
Duplicate comments:
In `@programs/registry/src/account_compression_cpi/claim_fees.rs`:
- Around line 9-28: ClaimFeesWrapper is missing the standard log_wrapper (for
event emission) and the target_account (the account-compression target to claim
into); update the ClaimFeesWrapper struct (the one defining
registered_forester_pda, authority, cpi_authority, registered_program_pda,
account_compression_program, merkle_tree_or_queue, protocol_config_pda,
fee_recipient) to include a mutable log_wrapper field (matching the other CPI
wrappers' LogWrapper account type) and a mutable target_account/target (matching
the other wrappers' account-compression target type or AccountInfo) with the
same CHECK/mut constraints as the other registry CPI wrappers so
callers/builders no longer need special casing.
In `@programs/registry/src/account_compression_cpi/mod.rs`:
- Line 4: The module wrapper declares pub mod claim_fees but does not re-export
its symbols, leaving them out of the account_compression_cpi public surface; fix
by adding a re-export line (pub use claim_fees::*;) in the
account_compression_cpi mod (the same file containing pub mod claim_fees;) so
all claim_fees symbols are exposed, and then ensure this module is imported from
lib.rs per the project convention.
In `@sdk-libs/program-test/src/indexer/test_indexer.rs`:
- Around line 1735-1746: The create_init_reimbursement_pda_instruction call
followed by rpc.create_and_send_transaction(...).await.unwrap() will panic if
the PDA already exists; change this to handle RPC errors and treat the "already
in use" / "account already exists" case as success: call
rpc.create_and_send_transaction(...).await, match the Result/Err, inspect the
RPC error (string or RpcError variant) for the indicator of an existing account
(e.g., "already in use" / "account already exists" / RpcResponseError variant)
and silently ignore/return Ok(()) in that case, but propagate or panic on any
other error; apply this change around the
create_init_reimbursement_pda_instruction invocation using the same payer and
merkle_tree_keypair identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c1de092-0f2e-45ab-a88e-37e3c3852183
⛔ Files ignored due to path filters (3)
.specify/memory/constitution.mdis excluded by none and included by noneprogram-tests/registry-test/tests/test_claim_fees.rsis excluded by none and included by noneprogram-tests/registry-test/tests/test_fee_distribution.rsis excluded by none and included by none
📒 Files selected for processing (9)
program-libs/merkle-tree-metadata/src/fee.rsprograms/account-compression/src/instructions/claim_fees.rsprograms/registry/src/account_compression_cpi/batch_append.rsprograms/registry/src/account_compression_cpi/batch_nullify.rsprograms/registry/src/account_compression_cpi/claim_fees.rsprograms/registry/src/account_compression_cpi/mod.rsprograms/registry/src/account_compression_cpi/nullify.rsprograms/registry/src/fee_reimbursement/initialize.rssdk-libs/program-test/src/indexer/test_indexer.rs
Comment on lines
+209
to
+220
| let excess = compute_claimable_excess( | ||
| ctx.accounts.merkle_tree_or_queue.lamports(), | ||
| rent_exemption, | ||
| params.rollover_fee, | ||
| params.capacity, | ||
| params.next_index, | ||
| ) | ||
| .unwrap_or(0); | ||
|
|
||
| if excess == 0 { | ||
| return Ok(()); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent failure on compute_claimable_excess error.
The .unwrap_or(0) at line 216 treats any computation failure identically to "no excess available." While this is a safe/conservative fallback (no transfer occurs), it silently hides potential arithmetic issues.
Consider whether callers should be able to distinguish between "zero claimable excess" and "computation failed." If observability is important, you could either:
- Return an error on computation failure, or
- Emit a log when unwrapping to
0on failure
That said, if compute_claimable_excess only returns None on overflow (which shouldn't happen with sane inputs), this may be acceptable as-is.
🔧 Optional: Make computation failure explicit
- let excess = compute_claimable_excess(
- ctx.accounts.merkle_tree_or_queue.lamports(),
- rent_exemption,
- params.rollover_fee,
- params.capacity,
- params.next_index,
- )
- .unwrap_or(0);
+ let excess = compute_claimable_excess(
+ ctx.accounts.merkle_tree_or_queue.lamports(),
+ rent_exemption,
+ params.rollover_fee,
+ params.capacity,
+ params.next_index,
+ )
+ .ok_or(AccountCompressionErrorCode::IntegerOverflow)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/account-compression/src/instructions/claim_fees.rs` around lines 209
- 220, The current .unwrap_or(0) on compute_claimable_excess silently masks
failures (e.g., overflows) as zero excess; change this so callers can
distinguish errors: either propagate the computation error (return an Err from
the handler where compute_claimable_excess is called in claim_fees.rs) by
replacing .unwrap_or(0) with ? (or map the Option/Result into a ProgramError)
or, if you must keep a safe fallback, log the failure before treating it as zero
(use the program logger/msg! or process_logger to record the error and inputs
such as ctx.accounts.merkle_tree_or_queue.lamports(), rent_exemption,
params.rollover_fee, params.capacity, params.next_index) so we don’t lose
observability while preserving safety.
…trees - Move `use light_client::rpc::Rpc` behind `#[cfg(feature = "devenv")]` to fix unused import lint error in non-devenv builds - Only init reimbursement PDAs for trees owned by account-compression program, skipping CPI test trees with different owners Entire-Checkpoint: 1c18e6c3a904
Entire-Checkpoint: 74252da0d938
Entire-Checkpoint: ef3d398e240d
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
416-424:⚠️ Potential issue | 🟠 MajorPropagate real reimbursement-setup failures instead of collapsing them into skips.
ok().flatten()turns an RPC failure into the same path as the validOk(None)case, andlet _ = ...awaithides failures from a non-idempotentinit_reimbursement_pdainstruction. Only skip the explicitNone/ non-account_compressioncases; bubble up real lookup and submission errors soLightProgramTest::new()cannot return half-initialized.🩹 Proposed fix
- if let Some(account) = context.get_account(tree_pubkey).await.ok().flatten() { - if account.owner == account_compression::ID { - let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( - payer.pubkey(), - tree_pubkey, - ); - let _ = context - .create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer]) - .await; - } - } + match context.get_account(tree_pubkey).await? { + Some(account) if account.owner == account_compression::ID => { + let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( + payer.pubkey(), + tree_pubkey, + ); + context + .create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer]) + .await?; + } + _ => {} + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/program-test/src/program_test/light_program_test.rs` around lines 416 - 424, The code swallows RPC lookup and transaction failures by using ok().flatten() and let _ = ...await; change the get_account handling to propagate real errors (do not use ok().flatten())—e.g., await context.get_account(tree_pubkey) and match on Result so an Err is returned upward instead of treated as None, only skipping when the lookup returns Ok(None) or the account owner isn't account_compression::ID; likewise remove the let _ = around context.create_and_send_transaction (await) and propagate its Result (use ? or proper error handling) so failures from light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction submission surface and prevent LightProgramTest::new() from returning half-initialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk-libs/program-test/src/program_test/light_program_test.rs`:
- Around line 396-424: The post-setup transaction counter reset is happening
before the devenv reimbursement PDA init loop, so calls to
context.create_and_send_transaction(...) in that block increment
context.transaction_counter and leave LightProgramTest::new() with a non-zero
counter; move the reset (context.transaction_counter = 0) to after the entire
#[cfg(feature = "devenv")] block (after the reimbursement PDA loop) or exclude
the reimbursement PDA transaction calls from the reset scope so that
transaction_counter is zero when returning from LightProgramTest::new(); locate
the reset and the loop by referencing context.transaction_counter and the
create_and_send_transaction call in the reimbursement PDA initialization code to
apply the change.
---
Duplicate comments:
In `@sdk-libs/program-test/src/program_test/light_program_test.rs`:
- Around line 416-424: The code swallows RPC lookup and transaction failures by
using ok().flatten() and let _ = ...await; change the get_account handling to
propagate real errors (do not use ok().flatten())—e.g., await
context.get_account(tree_pubkey) and match on Result so an Err is returned
upward instead of treated as None, only skipping when the lookup returns
Ok(None) or the account owner isn't account_compression::ID; likewise remove the
let _ = around context.create_and_send_transaction (await) and propagate its
Result (use ? or proper error handling) so failures from
light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction
submission surface and prevent LightProgramTest::new() from returning
half-initialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 063c8550-83cf-47c0-97f1-311f8ce00e43
📒 Files selected for processing (1)
sdk-libs/program-test/src/program_test/light_program_test.rs
Comment on lines
+396
to
+424
| // Initialize reimbursement PDAs for genesis state trees owned by | ||
| // account-compression. Skip trees owned by other programs (CPI test trees). | ||
| #[cfg(feature = "devenv")] | ||
| { | ||
| let payer = context.get_payer().insecure_clone(); | ||
| let all_state_trees: Vec<_> = context | ||
| .test_accounts | ||
| .v1_state_trees | ||
| .iter() | ||
| .map(|t| t.merkle_tree) | ||
| .chain( | ||
| context | ||
| .test_accounts | ||
| .v2_state_trees | ||
| .iter() | ||
| .map(|t| t.merkle_tree), | ||
| ) | ||
| .collect(); | ||
| for tree_pubkey in all_state_trees { | ||
| // Only init PDA if the tree is owned by account-compression. | ||
| if let Some(account) = context.get_account(tree_pubkey).await.ok().flatten() { | ||
| if account.owner == account_compression::ID { | ||
| let ix = light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( | ||
| payer.pubkey(), | ||
| tree_pubkey, | ||
| ); | ||
| let _ = context | ||
| .create_and_send_transaction(&[ix], &payer.pubkey(), &[&payer]) | ||
| .await; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 '\btransaction_counter\b|\bcreate_and_send_transaction\b' sdk-libs/program-test/srcRepository: Lightprotocol/light-protocol
Length of output: 26682
Reset transaction_counter after the reimbursement setup block or remove transactions from the post-setup reset scope.
The create_and_send_transaction calls in lines 419–424 increment transaction_counter immediately, making the reset on line 394 ineffective when devenv is enabled. Move context.transaction_counter = 0 to after line 427, or skip this block from the counter-reset scope, to ensure LightProgramTest::new() returns with a clean state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk-libs/program-test/src/program_test/light_program_test.rs` around lines
396 - 424, The post-setup transaction counter reset is happening before the
devenv reimbursement PDA init loop, so calls to
context.create_and_send_transaction(...) in that block increment
context.transaction_counter and leave LightProgramTest::new() with a non-zero
counter; move the reset (context.transaction_counter = 0) to after the entire
#[cfg(feature = "devenv")] block (after the reimbursement PDA loop) or exclude
the reimbursement PDA transaction calls from the reset scope so that
transaction_counter is zero when returning from LightProgramTest::new(); locate
the reset and the loop by referencing context.transaction_counter and the
create_and_send_transaction call in the reimbursement PDA initialization code to
apply the change.
…rees The test_invalid_registered_program test creates state trees directly via create_state_merkle_tree_and_queue_account (bypassing registry). Since reimbursement_pda is now required in NullifyLeaves, the PDA must be initialized before registry wrapper calls. Entire-Checkpoint: 7e75d9932bcc
Trees created by CLI/direct account-compression calls don't have reimbursement PDAs. Since BatchAppend/BatchNullify/NullifyLeaves now require the PDA, init it after tree setup in tests that create trees outside the LightProgramTest flow. Entire-Checkpoint: dae77dfe4e76
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@forester/tests/e2e_test.rs`:
- Around line 302-317: The loop that calls
create_init_reimbursement_pda_instruction currently ignores errors from
rpc.create_and_send_transaction, which hides failures to initialize required
reimbursement PDAs; update the logic in the loop over all_state_trees (using
payer / env.protocol.governance_authority and tree_pubkey) to first derive/check
the deterministic reimbursement PDA for each tree, skip sending the init
transaction only if the PDA already exists, and otherwise call
rpc.create_and_send_transaction and propagate/return a clear failure (including
tree_pubkey and payer info) on error instead of swallowing it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2796ce1-eee1-41fc-b098-2a472d404982
⛔ Files ignored due to path filters (1)
program-tests/system-cpi-test/tests/test_program_owned_trees.rsis excluded by none and included by none
📒 Files selected for processing (1)
forester/tests/e2e_test.rs
Comment on lines
+302
to
+317
| let payer = &env.protocol.governance_authority; | ||
| let all_state_trees: Vec<Pubkey> = env | ||
| .v1_state_trees | ||
| .iter() | ||
| .map(|t| t.merkle_tree) | ||
| .chain(env.v2_state_trees.iter().map(|t| t.merkle_tree)) | ||
| .collect(); | ||
| for tree_pubkey in all_state_trees { | ||
| let ix = | ||
| light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( | ||
| payer.pubkey(), | ||
| tree_pubkey, | ||
| ); | ||
| let _ = rpc | ||
| .create_and_send_transaction(&[ix], &payer.pubkey(), &[payer]) | ||
| .await; |
Contributor
There was a problem hiding this comment.
Don’t make required reimbursement-PDA setup best-effort.
This block establishes a hard precondition for the later batch ops, but it drops every send error. That is especially brittle here because it uses env.protocol.governance_authority, which this test only explicitly funds in local mode; on a fresh environment, a failure here just gets reported later as a much less local e2e failure. If reruns need to tolerate already-created PDAs, pre-check the deterministic reimbursement PDA and skip only that case; otherwise fail here with tree context.
Proposed fix
for tree_pubkey in all_state_trees {
+ let (reimbursement_pda, _) =
+ light_registry::account_compression_cpi::sdk::get_reimbursement_pda(&tree_pubkey);
+ if rpc.get_account(reimbursement_pda).await.unwrap().is_some() {
+ continue;
+ }
+
let ix =
light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction(
payer.pubkey(),
tree_pubkey,
);
- let _ = rpc
- .create_and_send_transaction(&[ix], &payer.pubkey(), &[payer])
- .await;
+ rpc.create_and_send_transaction(&[ix], &payer.pubkey(), &[payer])
+ .await
+ .unwrap_or_else(|e| {
+ panic!(
+ "failed to init reimbursement PDA for tree {}: {:?}",
+ tree_pubkey, e
+ )
+ });
}📝 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
| let payer = &env.protocol.governance_authority; | |
| let all_state_trees: Vec<Pubkey> = env | |
| .v1_state_trees | |
| .iter() | |
| .map(|t| t.merkle_tree) | |
| .chain(env.v2_state_trees.iter().map(|t| t.merkle_tree)) | |
| .collect(); | |
| for tree_pubkey in all_state_trees { | |
| let ix = | |
| light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( | |
| payer.pubkey(), | |
| tree_pubkey, | |
| ); | |
| let _ = rpc | |
| .create_and_send_transaction(&[ix], &payer.pubkey(), &[payer]) | |
| .await; | |
| let payer = &env.protocol.governance_authority; | |
| let all_state_trees: Vec<Pubkey> = env | |
| .v1_state_trees | |
| .iter() | |
| .map(|t| t.merkle_tree) | |
| .chain(env.v2_state_trees.iter().map(|t| t.merkle_tree)) | |
| .collect(); | |
| for tree_pubkey in all_state_trees { | |
| let (reimbursement_pda, _) = | |
| light_registry::account_compression_cpi::sdk::get_reimbursement_pda(&tree_pubkey); | |
| if rpc.get_account(reimbursement_pda).await.unwrap().is_some() { | |
| continue; | |
| } | |
| let ix = | |
| light_registry::account_compression_cpi::sdk::create_init_reimbursement_pda_instruction( | |
| payer.pubkey(), | |
| tree_pubkey, | |
| ); | |
| rpc.create_and_send_transaction(&[ix], &payer.pubkey(), &[payer]) | |
| .await | |
| .unwrap_or_else(|e| { | |
| panic!( | |
| "failed to init reimbursement PDA for tree {}: {:?}", | |
| tree_pubkey, e | |
| ) | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forester/tests/e2e_test.rs` around lines 302 - 317, The loop that calls
create_init_reimbursement_pda_instruction currently ignores errors from
rpc.create_and_send_transaction, which hides failures to initialize required
reimbursement PDAs; update the logic in the loop over all_state_trees (using
payer / env.protocol.governance_authority and tree_pubkey) to first derive/check
the deterministic reimbursement PDA for each tree, skip sending the init
transaction only if the PDA already exists, and otherwise call
rpc.create_and_send_transaction and propagate/return a clear failure (including
tree_pubkey and payer info) on error instead of swallowing it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Enhancements
Configuration
Tests
Documentation