chore: pre release #2014
chore: pre release #2014
Conversation
WalkthroughThis PR introduces fee structure versioning (v1/v2 for input accounts), consolidates network fee configuration ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant InitBatched as initialize_batched_address_merkle_tree
participant Validation
participant Registry as registered_program_pda
participant TreeInit as init_batched_address_merkle_tree
Test->>InitBatched: Call with params
alt Non-Test Build
InitBatched->>Validation: Check params == default
alt Params != default
Validation-->>InitBatched: UnsupportedParameters
InitBatched-->>Test: Error
else Params == default
InitBatched->>Registry: Fetch registered_program_pda
alt registered_program_pda exists
Registry-->>InitBatched: Some(pda)
InitBatched->>Validation: Verify group_authority matches
alt Authority matches hardcoded key
Validation-->>InitBatched: Ok
InitBatched->>TreeInit: Initialize tree
TreeInit-->>InitBatched: Success
else Authority mismatch
Validation-->>InitBatched: UnsupportedParameters
InitBatched-->>Test: Error
end
else No registered_program_pda
Registry-->>InitBatched: None
InitBatched-->>Test: UnsupportedParameters
end
end
else Test Build
InitBatched->>Validation: Validate params via qualified path
Validation-->>InitBatched: Ok/Error
alt Validation passes
InitBatched->>TreeInit: Initialize tree
TreeInit-->>InitBatched: Success
else Validation fails
InitBatched-->>Test: Error
end
end
sequenceDiagram
participant Processor
participant SystemCtx as SystemContext
participant MerkleCtx as MerkleTreeContext
participant FeeHelper as set_additive_fee
Processor->>SystemCtx: Call set_network_fee_v1(fee, index)
SystemCtx->>FeeHelper: Delegate with fee & index
alt Overflow check passes
FeeHelper->>MerkleCtx: Accumulate fee
MerkleCtx-->>FeeHelper: Updated
FeeHelper-->>SystemCtx: Ok(())
else Overflow detected
FeeHelper-->>SystemCtx: ArithmeticOverflow
end
SystemCtx-->>Processor: Result<()>
Processor->>SystemCtx: Call set_network_fee_v2(fee, index)
SystemCtx->>MerkleCtx: Mark network fee set
SystemCtx->>MerkleCtx: Queue initial fee
MerkleCtx-->>SystemCtx: Complete
SystemCtx-->>Processor: ()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
| @@ -1,5 +1,3 @@ | |||
| #![cfg(feature = "test-sbf")] | |||
There was a problem hiding this comment.
removed on purpose, these are not necessary since we dont run non sbf tests in the same test crate and prevent rust analyzer from compiling the file.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
254-259: Replace unwraps with proper error mapping to avoid panics.Parsing failures should return RpcError instead of panicking.
- let mut tree = BatchedMerkleTreeAccount::state_from_bytes( + let mut tree = BatchedMerkleTreeAccount::state_from_bytes( tree_account_data.data_as_mut_slice(), &merkle_tree_pubkey.into(), - ) - .unwrap(); + ) + .map_err(|e| RpcError::CustomError(format!( + "Failed to parse batched state tree {}: {}", + merkle_tree_pubkey, e + )))?; … - let mut queue = BatchedQueueAccount::output_from_bytes( - queue_account_data.data_as_mut_slice(), - ) - .unwrap(); + let mut queue = BatchedQueueAccount::output_from_bytes( + queue_account_data.data_as_mut_slice(), + ) + .map_err(|e| RpcError::CustomError(format!( + "Failed to parse batched output queue {}: {}", + output_queue_pubkey, e + )))?;Also applies to: 269-273
program-tests/account-compression-test/tests/address_merkle_tree_tests.rs (2)
1382-1392: One leftover 5000 conflicts with new 10000 default.If not intentional, update to 10000 to keep test expectations coherent.
Apply:
- network_fee: Some(5000), + network_fee: Some(10000),
146-155: Update network_fee at line 1389 in address_merkle_tree_tests.rs from 5000 to 10000.Verification found that line 1389 in the same file still has
network_fee: Some(5000), which is inconsistent with the updated value at lines 146-155. Update this occurrence tonetwork_fee: Some(10000)to maintain consistency within the file.programs/system/src/processor/create_address_cpi_data.rs (1)
54-61: Likely index mix-up for legacy rollover_fee (queue vs tree).Elsewhere (e.g., create_outputs_cpi_data) legacy context is keyed by Merkle tree index. Using address_queue_index here risks wrong fee on multi-queue setups.
Apply:
- context - .get_legacy_merkle_context(new_address_params.address_queue_index()) + context + .get_legacy_merkle_context( + new_address_params.address_merkle_tree_account_index() + ) .unwrap() .rollover_fee,Optional: replace unwrap with error propagation similar to the previous comment.
programs/registry/src/lib.rs (1)
294-311: Inconsistent error handling and contradictory validation logic.The deprecation checks contain several issues:
Lines 298-301: Returns
ForesterUndefinedwhenprogram_owner.is_none(), but the semantic issue is that program_owner is missing, not forester. Consider usingRegistryError::ProgramOwnerUndefinedor similar.Lines 307-311: After establishing that program_owned trees are required and network fees are forbidden (deprecation restrictions), the code then checks if
forester.is_none()and returnsForesterUndefined. This contradicts the deprecation intent stated in lines 294-297. If V1 address trees must be program-owned with no fees, why require a forester?The comment on line 308 states "Trees without a network fee will not be serviced by light foresters", but line 303 already enforces that network_fee must be None. This creates a logical contradiction.
Consider this correction:
- // Address V1 trees are deprecated. - // Disable creation of forested address V1 trees. - // All address V1 trees must be program owned. - // All address V1 trees must not have fees. - if program_owner.is_none() { - msg!("Program owner must be defined."); - return err!(RegistryError::ForesterUndefined); - } - if merkle_tree_config.network_fee.is_some() { - msg!("Network fee must be None."); - return err!(RegistryError::InvalidNetworkFee); - } - // Only trees with a network fee will be serviced by light foresters. - if forester.is_none() { - msg!("Forester pubkey required for trees without a network fee."); - msg!("Trees without a network fee will not be serviced by light foresters."); - return err!(RegistryError::ForesterUndefined); - } + // Address V1 trees are deprecated. + // All address V1 trees must be program owned with a designated forester. + // All address V1 trees must not have network fees. + if program_owner.is_none() { + msg!("Program owner must be defined for deprecated V1 address trees."); + return err!(RegistryError::ProgramOwnerUndefined); + } + if forester.is_none() { + msg!("Forester must be defined for program-owned V1 address trees."); + return err!(RegistryError::ForesterUndefined); + } + if merkle_tree_config.network_fee.is_some() { + msg!("Network fee must be None for deprecated V1 address trees."); + return err!(RegistryError::InvalidNetworkFee); + }programs/system/Cargo.toml (1)
13-30: Removereinitfrom default features—it should not compile by default.The documentation (lines 18–23) explicitly outlines a 4-step deployment process where step 3 requires recompiling without the
reinitfeature before final redeployment. Havingreinitindefault = ["reinit"]contradicts this intent: the migration code will compile by default, when it should only be enabled during the specific deployment window.Additionally, a survey of 40+
Cargo.tomlfiles across the codebase shows thatprograms/system/Cargo.tomlis the sole outlier with a non-empty feature in defaults. All other programs use eitherdefault = []or optional infrastructure features like["custom-heap"]. This pattern reinforces thatreinitshould not ship by default.Change
default = ["reinit"]todefault = []and require the feature to be explicitly enabled during deployment via--features reinit.
🧹 Nitpick comments (16)
program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (1)
2941-2941: Remove unnecessaryprintlnmacro import from use statement.The
println!macro is part of the standard prelude and does not require an explicit import. Explicitly importing it is non-idiomatic and redundant.- use std::{collections::HashMap, println}; + use std::collections::HashMap;program-tests/system-cpi-test/tests/test_program_owned_trees.rs (1)
50-50: Consider removing the test entirely if the feature is permanently deprecated.While ignoring the test is appropriate for deprecated functionality, ignored tests can accumulate bit rot and become maintenance burdens. If program-owned state trees are permanently deprecated with no plan to restore them, consider removing this test entirely. If there's value in keeping it for historical reference or potential future use, document that rationale in a comment.
sdk-libs/program-test/src/indexer/test_indexer.rs (2)
1392-1400: Consider clarifying the comment about custom fees.The comment "We only allow program owned address trees with custom fees" could be misinterpreted. Setting
network_fee: Noneindicates that the owning program handles fees through its own mechanism rather than using the standard network fee. Consider rephrasing to make this clearer.- let config = if owning_program_id.is_some() { - // We only allow program owned address trees with custom fees. + let config = if owning_program_id.is_some() { + // Program-owned address trees use custom fee handling (network_fee = None). AddressMerkleTreeConfig { network_fee: None, ..AddressMerkleTreeConfig::default() }
1408-1412: Consider improving error handling and comment clarity.While this is test code where panics are somewhat acceptable, the
.unwrap()on line 1410 could provide a more helpful error message ifFORESTER_TEST_KEYPAIRconversion fails. Additionally, the inline comment on line 1412 could be more descriptive about why the forester is now required.- Some( - Keypair::try_from(FORESTER_TEST_KEYPAIR.as_slice()) - .unwrap() - .pubkey(), - ), // std forester, we now need to set it. + Some( + Keypair::try_from(FORESTER_TEST_KEYPAIR.as_slice()) + .expect("FORESTER_TEST_KEYPAIR should be a valid keypair") + .pubkey(), + ), // Forester is now required for address merkle tree initialization.sdk-libs/program-test/src/program_test/light_program_test.rs (2)
112-121: Gate V1 JSON loading behind skip_v1_trees to honor config.If skip_v1_trees is true, this block should be skipped to avoid enforcing “exactly 1 V1 address tree.” Wrap the block accordingly.
Apply around this block:
- // Load V1 address tree accounts from JSON files - { + // Load V1 address tree accounts from JSON files + if !config.skip_v1_trees { use crate::utils::load_accounts::load_account_from_dir; if context.test_accounts.v1_address_trees.len() != 1 { return Err(RpcError::CustomError(format!( "Expected exactly 1 V1 address tree, found {}. V1 address trees are deprecated and only one is supported.", context.test_accounts.v1_address_trees.len() ))); } … - } + }
126-151: Validate owner program before set_account to prevent mismatched artifacts.Check that loaded V1 address tree and queue are owned by account_compression::ID before injecting.
Apply after each load:
- let tree_account = - load_account_from_dir(&address_mt, Some("address_merkle_tree"))?; + let tree_account = + load_account_from_dir(&address_mt, Some("address_merkle_tree"))?; + if tree_account.owner != account_compression::ID { + return Err(RpcError::CustomError(format!( + "Unexpected owner for V1 address tree {}: {}, expected {}", + address_mt, tree_account.owner, account_compression::ID + ))); + } … - let queue_account = load_account_from_dir( + let queue_account = load_account_from_dir( &address_queue_pubkey, Some("address_merkle_tree_queue"), )?; + if queue_account.owner != account_compression::ID { + return Err(RpcError::CustomError(format!( + "Unexpected owner for V1 address queue {}: {}, expected {}", + address_queue_pubkey, queue_account.owner, account_compression::ID + ))); + }Based on learnings.
sdk-libs/program-test/src/accounts/initialize.rs (1)
151-167: LGTM on deprecating V1 address tree creation; keep docs in sync.Since V1 address trees are now loaded from JSON, ensure related comments/utilities (e.g., find_accounts_dir notes) reflect this usage to avoid confusion.
programs/account-compression/src/instructions/initialize_address_merkle_tree_and_queue.rs (1)
44-46: Default fee bump looks fine; consider a named constant.Use a shared DEFAULT_ADDRESS_NETWORK_FEE to avoid magic numbers and keep defaults in sync across crates.
program-libs/batched-merkle-tree/src/initialize_address_tree.rs (1)
47-50: Consistent fee default update.Changes are coherent with the wider bump to 10000. Consider centralizing the default in one place (const) to prevent drift between runtime and test utils.
Also applies to: 205-209, 222-226, 238-242
programs/package.json (1)
6-6: Harden cargo invocation.Add
--no-default-featuresand use space‑separated features for portability.- "build": "cd system/ && cargo build-sbf && cd .. && cd account-compression/ && cargo build-sbf --features 'test, migrate-state' && cd .. && cd registry/ && cargo build-sbf && cd .. && cd compressed-token/program && cargo build-sbf && cd ../../..", + "build": "cd system/ && cargo build-sbf && cd .. && cd account-compression/ && cargo build-sbf --no-default-features --features \"test migrate-state\" && cd .. && cd registry/ && cargo build-sbf && cd .. && cd compressed-token/program && cargo build-sbf && cd ../../..",program-tests/system-cpi-test/tests/test.rs (1)
1485-1498: Don’t leave large blocks commented out; gate or replace with an ignored test.Prefer
#[ignore = "Program owned state trees are deprecated"](or a feature gate) over commented code to keep intent explicit and test list clean.Example:
- // Program owned state trees are deprecated. - // perform_create_pda_failing( - // ... - // ).await.unwrap(); + #[ignore = "Program-owned state trees are deprecated"] + #[allow(unused_variables)] + async fn deprecated_program_owned_state_tree_case() { + // Retained for reference; remove once fully obsolete. + // perform_create_pda_failing(...).await.unwrap(); + }programs/account-compression/src/instructions/initialize_batched_state_merkle_tree.rs (1)
41-53: Pre‑release hard gate is OK; make it explicit, traceable, and maintainable.
- Replace magic pubkey with a named const and log a reason on failure.
- Consider guarding this whole block behind a dedicated cargo feature (e.g.,
pre-release-gate) so mainnet/testnet builds opt‑in explicitly.- Use a specific error code for “not allowed in this build” if available.
- if let Some(registered_program_pda) = ctx.accounts.registered_program_pda.as_ref() { - if registered_program_pda.group_authority_pda - != pubkey!("24rt4RgeyjUCWGS2eF7L7gyNMuz6JWdqYpAvb1KRoHxs") - { - return err!(AccountCompressionErrorCode::UnsupportedParameters); - } - } else { - return err!(AccountCompressionErrorCode::UnsupportedParameters); - } + const ALLOWED_GROUP_AUTHORITY: Pubkey = + pubkey!("24rt4RgeyjUCWGS2eF7L7gyNMuz6JWdqYpAvb1KRoHxs"); + if let Some(registered_program_pda) = ctx.accounts.registered_program_pda.as_ref() { + if registered_program_pda.group_authority_pda != ALLOWED_GROUP_AUTHORITY { + msg!("initialize_batched_state_merkle_tree: group_authority mismatch"); + return err!(AccountCompressionErrorCode::UnsupportedParameters); + } + } else { + msg!("initialize_batched_state_merkle_tree: registered_program_pda is required"); + return err!(AccountCompressionErrorCode::UnsupportedParameters); + }Optionally wrap this whole block with
#[cfg(feature = "pre-release-gate")]. Based on learnings.programs/system/src/processor/create_address_cpi_data.rs (1)
43-53: Legacy fee bump: avoid magic number and unwrap.
- Magic 5000: define a named const to document intent and keep changes centralized.
- unwrap on legacy context can trap; prefer explicit error propagation.
Apply:
+const LEGACY_V1_ADDRESS_FEE_ADDEND: u64 = 5_000; @@ - let mut network_fee = context - .get_legacy_merkle_context( - new_address_params.address_merkle_tree_account_index(), - ) - .unwrap() - .network_fee; + let mut network_fee = context + .get_legacy_merkle_context( + new_address_params.address_merkle_tree_account_index(), + ) + .ok_or(SystemProgramError::DeriveAddressError)? + .network_fee; if network_fee != 0 { - network_fee += 5000; + network_fee += LEGACY_V1_ADDRESS_FEE_ADDEND; }programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs (1)
47-47: Harden the non-test gate: extract the authority pubkey to a constant and fix the comment.
- Replace the inlined pubkey with a module-level constant; easier audits and reuse.
- The TODO mentions “state trees” but this instruction initializes address trees; fix to avoid confusion.
Apply within the shown lines:
- // TODO: test that only security group 24rt4RgeyjUCWGS2eF7L7gyNMuz6JWdqYpAvb1KRoHxs can create batched state trees + // TODO: test that only the security group can create batched address trees if let Some(registered_program_pda) = ctx.accounts.registered_program_pda.as_ref() { - if registered_program_pda.group_authority_pda - != pubkey!("24rt4RgeyjUCWGS2eF7L7gyNMuz6JWdqYpAvb1KRoHxs") + if registered_program_pda.group_authority_pda != SECURITY_GROUP_AUTHORITY { return err!(AccountCompressionErrorCode::UnsupportedParameters); }Add outside this range (top of file suggested):
const SECURITY_GROUP_AUTHORITY: Pubkey = pubkey!("24rt4RgeyjUCWGS2eF7L7gyNMuz6JWdqYpAvb1KRoHxs");Optionally consider distinct errors for:
- UnsupportedParameters (non-default params)
- MissingRegisteredProgram
- UnauthorizedGroup
Also applies to: 51-60
program-tests/account-compression-test/tests/batched_merkle_tree_test.rs (2)
68-154: Good negative test for non-test build gate (state trees).Covers the Unauthorized/UnsupportedParameters path with the expected instruction index; ignored by default to avoid CI noise.
You could also wrap with
#[cfg(not(feature = "test"))]to avoid compiling this test in test-feature builds.
156-183: Good negative test for non-test build gate (address trees).Mirrors state tree failing path; error index 1 matches the second instruction in the tx.
Same optional
#[cfg(not(feature = "test"))]gating applies if you prefer compile-time exclusion over#[ignore].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
program-libs/batched-merkle-tree/src/initialize_address_tree.rs(4 hunks)program-tests/account-compression-test/tests/address_merkle_tree_tests.rs(2 hunks)program-tests/account-compression-test/tests/batched_merkle_tree_test.rs(1 hunks)program-tests/registry-test/tests/tests.rs(7 hunks)program-tests/system-cpi-test/tests/test.rs(4 hunks)program-tests/system-cpi-test/tests/test_program_owned_trees.rs(2 hunks)program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs(1 hunks)program-tests/system-test/tests/test.rs(5 hunks)program-tests/utils/src/e2e_test_env.rs(15 hunks)program-tests/utils/src/lib.rs(1 hunks)programs/account-compression/Cargo.toml(1 hunks)programs/account-compression/src/instructions/initialize_address_merkle_tree_and_queue.rs(1 hunks)programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs(2 hunks)programs/account-compression/src/instructions/initialize_batched_state_merkle_tree.rs(2 hunks)programs/account-compression/src/lib.rs(1 hunks)programs/package.json(1 hunks)programs/registry/src/account_compression_cpi/rollover_batched_address_tree.rs(1 hunks)programs/registry/src/errors.rs(1 hunks)programs/registry/src/lib.rs(5 hunks)programs/registry/src/protocol_config/state.rs(3 hunks)programs/system/Cargo.toml(1 hunks)programs/system/src/accounts/remaining_account_checks.rs(3 hunks)programs/system/src/context.rs(4 hunks)programs/system/src/processor/cpi.rs(0 hunks)programs/system/src/processor/create_address_cpi_data.rs(2 hunks)programs/system/src/processor/create_inputs_cpi_data.rs(2 hunks)programs/system/src/processor/create_outputs_cpi_data.rs(1 hunks)sdk-libs/client/src/fee.rs(5 hunks)sdk-libs/program-test/src/accounts/initialize.rs(1 hunks)sdk-libs/program-test/src/indexer/test_indexer.rs(1 hunks)sdk-libs/program-test/src/program_test/light_program_test.rs(2 hunks)sdk-libs/program-test/src/utils/load_accounts.rs(1 hunks)
💤 Files with no reviewable changes (1)
- programs/system/src/processor/cpi.rs
🧰 Additional context used
📓 Path-based instructions (3)
programs/registry/src/account_compression_cpi/**/*.rs
📄 CodeRabbit inference engine (programs/registry/CLAUDE.md)
programs/registry/src/account_compression_cpi/**/*.rs: CPI processing functions must derive PDA signer seeds as [CPI_AUTHORITY_PDA_SEED, bump] and use CpiContext::new_with_signer with cpi_authority as the authority account and mapped target accounts.
Pass the data Vec through unchanged from the wrapper to the target program CPI; the target program performs deserialization.
Files:
programs/registry/src/account_compression_cpi/rollover_batched_address_tree.rs
programs/registry/src/account_compression_cpi/*.rs
📄 CodeRabbit inference engine (programs/registry/CLAUDE.md)
programs/registry/src/account_compression_cpi/*.rs: Each new operation module must define an Anchor context struct (e.g., NewOperationContext) with required accounts and a process_ function that prepares signer seeds, maps accounts to the target program, and executes the CPI.
Context structs for wrapper instructions must include standard accounts: optional registered_forester_pda (mut), authority Signer, cpi_authority with seeds/bump for CPI_AUTHORITY_PDA_SEED, registered_program_pda, target program handle, log_wrapper, and mutable target_account.
Files:
programs/registry/src/account_compression_cpi/rollover_batched_address_tree.rs
programs/registry/src/lib.rs
📄 CodeRabbit inference engine (programs/registry/CLAUDE.md)
programs/registry/src/lib.rs: Wrapper instruction handlers must: (1) load the target account metadata; (2) call check_forester with correct authority, target, forester PDA (optional), and computed work_units; (3) delegate via a CPI using a PDA signer.
Instruction handlers must compute work_units by operation type: batch operations use account.queue_batches.batch_size; single operations use DEFAULT_WORK_V1; custom operations compute based on complexity.
Load accounts according to type before check_forester: batched via BatchedMerkleTreeAccount::type_from_account_info(); regular via ctx.accounts.account.load()?.metadata; use custom deserialization when required.
Apply forester eligibility rules: with a forester PDA, validate epoch registration, eligibility, track work, and require network fee; without a forester PDA, ensure authority matches the tree’s designated forester.
Files:
programs/registry/src/lib.rs
🧠 Learnings (10)
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/error.rs : Maintain stable mapping of AccountError to ProgramError, including Pinocchio code mapping (1–11), in error.rs
Applied to files:
programs/registry/src/errors.rsprograms/system/src/context.rs
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/lib.rs : Load accounts according to type before check_forester: batched via BatchedMerkleTreeAccount::type_from_account_info(); regular via ctx.accounts.account.load()?.metadata; use custom deserialization when required.
Applied to files:
sdk-libs/program-test/src/utils/load_accounts.rssdk-libs/program-test/src/accounts/initialize.rsprograms/account-compression/src/instructions/initialize_batched_address_merkle_tree.rsprograms/account-compression/src/instructions/initialize_batched_state_merkle_tree.rssdk-libs/program-test/src/program_test/light_program_test.rsprogram-tests/registry-test/tests/tests.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/**/Cargo.toml : Define features solana, pinocchio, and test-only in Cargo.toml; default build should enable none
Applied to files:
programs/account-compression/Cargo.tomlprograms/package.jsonprograms/system/Cargo.toml
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/account_compression_cpi/**/*.rs : CPI processing functions must derive PDA signer seeds as [CPI_AUTHORITY_PDA_SEED, bump] and use CpiContext::new_with_signer with cpi_authority as the authority account and mapped target accounts.
Applied to files:
programs/registry/src/account_compression_cpi/rollover_batched_address_tree.rs
📚 Learning: 2025-10-16T06:33:19.426Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/compressible/CLAUDE.md:0-0
Timestamp: 2025-10-16T06:33:19.426Z
Learning: Applies to program-libs/compressible/src/config.rs : Ensure serialization compatibility across Anchor, Pinocchio, and Borsh for core account types used by dependent programs
Applied to files:
program-tests/system-cpi-test/tests/test.rssdk-libs/program-test/src/program_test/light_program_test.rsprogram-tests/account-compression-test/tests/batched_merkle_tree_test.rsprogram-tests/registry-test/tests/tests.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs} : Gate SDK-specific implementations with #[cfg(feature = "solana"|"pinocchio"|"test-only")]
Applied to files:
program-tests/system-cpi-test/tests/test.rsprograms/system/src/context.rsprogram-tests/account-compression-test/tests/batched_merkle_tree_test.rsprogram-tests/registry-test/tests/tests.rs
📚 Learning: 2025-10-16T06:33:19.426Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/compressible/CLAUDE.md:0-0
Timestamp: 2025-10-16T06:33:19.426Z
Learning: Applies to program-libs/compressible/src/config.rs : Maintain CompressibleConfig account structure with Anchor/Borsh/Pod (Pinocchio/Pod) serialization and related state validation methods (validate_active, validate_not_inactive) in src/config.rs
Applied to files:
program-tests/system-cpi-test/tests/test.rsprogram-tests/account-compression-test/tests/batched_merkle_tree_test.rs
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/lib.rs : Apply forester eligibility rules: with a forester PDA, validate epoch registration, eligibility, track work, and require network fee; without a forester PDA, ensure authority matches the tree’s designated forester.
Applied to files:
programs/registry/src/lib.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/!(account_info)/**/*.rs : Use AccountInfoTrait for runtime-agnostic account handling; avoid direct solana-program or pinocchio AccountInfo in general logic
Applied to files:
programs/system/src/context.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs} : Provide SDK-specific AccountInfoTrait implementations in account_info/{solana.rs,pinocchio.rs,test_account_info.rs}
Applied to files:
programs/system/src/context.rs
🧬 Code graph analysis (9)
programs/registry/src/account_compression_cpi/rollover_batched_address_tree.rs (1)
program-libs/merkle-tree-metadata/src/utils.rs (1)
if_equals_zero_u64(1-7)
programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs (1)
program-libs/batched-merkle-tree/src/initialize_address_tree.rs (3)
init_batched_address_merkle_tree_from_account_info(57-81)validate_batched_address_tree_params(132-167)default(36-51)
programs/account-compression/src/instructions/initialize_batched_state_merkle_tree.rs (1)
program-libs/batched-merkle-tree/src/initialize_state_tree.rs (2)
init_batched_state_merkle_tree_from_account_info(69-114)validate_batched_tree_params(203-248)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
sdk-libs/program-test/src/utils/load_accounts.rs (1)
load_account_from_dir(144-195)
sdk-libs/program-test/src/indexer/test_indexer.rs (2)
program-libs/batched-merkle-tree/src/initialize_address_tree.rs (1)
default(36-51)sdk-libs/program-test/src/accounts/address_tree.rs (1)
create_address_merkle_tree_and_queue_account(53-138)
program-tests/utils/src/e2e_test_env.rs (3)
program-tests/utils/src/system_program.rs (9)
inputs(329-333)inputs(344-352)inputs(396-400)inputs(403-407)inputs(469-473)input_compressed_accounts(134-137)input_compressed_accounts(191-194)input_compressed_accounts(244-247)input_compressed_accounts(251-254)program-tests/system-test/tests/test.rs (4)
input_compressed_accounts(202-205)input_compressed_accounts(244-247)input_compressed_accounts(272-275)input_compressed_accounts(276-279)program-tests/utils/src/spl.rs (8)
input_compressed_accounts(630-634)input_compressed_accounts(650-653)input_compressed_accounts(760-763)input_compressed_accounts(764-772)input_compressed_accounts(783-786)input_compressed_accounts(795-800)input_compressed_accounts(802-806)input_compressed_accounts(1137-1140)
program-tests/account-compression-test/tests/batched_merkle_tree_test.rs (6)
program-libs/batched-merkle-tree/src/initialize_address_tree.rs (2)
default(36-51)test_default(194-209)program-libs/batched-merkle-tree/src/initialize_state_tree.rs (2)
default(45-63)test_default(292-310)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(66-362)program-libs/batched-merkle-tree/src/queue.rs (1)
get_output_queue_account_size(550-565)program-libs/batched-merkle-tree/src/merkle_tree.rs (1)
get_merkle_tree_account_size(957-982)sdk-libs/program-test/src/utils/assert.rs (1)
assert_rpc_error(7-144)
programs/account-compression/src/lib.rs (1)
programs/account-compression/src/instructions/migrate_state.rs (1)
process_migrate_state(59-97)
program-tests/registry-test/tests/tests.rs (7)
program-libs/batched-merkle-tree/src/initialize_address_tree.rs (2)
default(36-51)test_default(194-209)programs/registry/src/protocol_config/state.rs (1)
default(51-70)sdk-libs/program-test/src/program_test/config.rs (3)
default(121-161)new(58-67)default_test_forester(97-105)program-libs/batched-merkle-tree/src/initialize_state_tree.rs (2)
default(45-63)test_default(292-310)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(66-362)program-tests/utils/src/lib.rs (1)
create_address_merkle_tree_and_queue_account_with_assert(78-182)sdk-libs/program-test/src/utils/assert.rs (1)
assert_rpc_error(7-144)
⏰ 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). (21)
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: system-programs (token test, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: cli-v2
- GitHub Check: Forester e2e test
- GitHub Check: cli-v1
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: Test program-libs-slow
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: Test program-libs-fast
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: stateless-js-v1
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: lint
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: stateless-js-v2
🔇 Additional comments (35)
program-tests/system-cpi-test/tests/test_program_owned_trees.rs (1)
110-111: LGTM! Transaction parameters updated to versioned input tracking.The change from
num_input_compressed_accountsto separatev1_input_compressed_accountsandv2_input_compressed_accountsfields aligns with the broader refactoring mentioned in the PR summary. Setting both to 0/false is appropriate for this test case with no input compressed accounts.sdk-libs/program-test/src/indexer/test_indexer.rs (1)
1390-1390: LGTM: Import added for test keypair.The import is necessary for the forester configuration changes below.
sdk-libs/program-test/src/program_test/light_program_test.rs (2)
112-151: ✓ Address merkle tree JSON files verified.Confirmed exactly 1 tree and 1 queue JSON file exist in
cli/accounts:
address_merkle_tree_amt1Ayt45jfbdw5YSo7iz6WZxUmnZsQTYXy82hVwyC2.jsonaddress_merkle_tree_queue_aq1S9z4reTSQAdgWHGD2zDaS39sjGrAxbR31vxJ2F4F.jsonThe "exactly 1" invariant checked at lines 116–122 is satisfied by the test setup.
300-347: Original verification script did not verify stated goal; script only confirmed base64 encoding format, not batch_size field presence or offset correctness.The provided shell script (
fd -t f '.json$' "$ACC_DIR"...) checks only that JSON account data uses base64 encoding (.account.data[1] == "base64"), which all accounts pass. However, the script does not:
- Decode base64 data to verify batch_size field exists at offset 240
- Validate the BatchedQueueMetadata layout claimed in the code comment
- Confirm consistency of batch_size values across accounts
To properly verify the batch_size extraction logic, you would need to decode account data and inspect bytes at offset 240-248, then confirm the offset matches the actual queue account structure.
The code's defensive practices are sound (length check ≥ 248 bytes, error handling via
try_into(), consistency verification). However, the semantic correctness of offset 240 cannot be confirmed without access to the queue account struct definition or sample decoded account data.program-tests/utils/src/lib.rs (1)
104-107: Prefer?over manual error branching; remove clippy allow.Restructure to bind the Ok value and return it at the end. This avoids
#[allow(clippy::question_mark)]and keeps flow clear.[ suggest_recommended_refactor ]
Apply:
- let result = create_address_merkle_tree_and_queue_account( + let sig = create_address_merkle_tree_and_queue_account( payer, registry, context, address_merkle_tree_keypair, address_queue_keypair, program_owner, forester, merkle_tree_config, queue_config, index, ) .await; - - #[allow(clippy::question_mark)] - if result.is_err() { - return result; - } + let sig = match sig { + Ok(s) => s, + Err(e) => return Err(e), + }; @@ - result + Ok(sig)programs/account-compression/Cargo.toml (1)
21-26: Good: empty default features and explicitmigrate-state.Matches the “default build enables none” guidance. Ensure CI/build scripts pass
--features testwhere needed (already adjusted in package.json). Based on learnings.program-tests/system-cpi-test/tests/test.rs (1)
988-990: Aligns with deprecation of program‑owned state trees.Passing
Nonefor program owner is consistent with the new policy.Please confirm all helper APIs ignore/assume user-owned trees when
None.Also applies to: 1471-1473, 1511-1514
program-tests/account-compression-test/tests/address_merkle_tree_tests.rs (1)
1140-1149: Consistent fee across custom wrap-around tests.This 10000 bump matches defaults; ensure all other custom configs in the suite use the same address network fee unless intentionally testing non-defaults.
To auto-fix a remaining 5000 in this file (if not intentional), apply:
- network_fee: Some(5000), + network_fee: Some(10000),programs/system/src/processor/create_address_cpi_data.rs (1)
78-82: Good: propagate errors from set_address_fee in V2 path.Adding ? keeps behavior consistent with nearby fallible calls.
programs/system/src/processor/create_outputs_cpi_data.rs (1)
73-79: All call sites properly updated; API rename verified.Verification confirms set_network_fee_v2 is defined in
context.rs:73and both call sites have been updated:
create_outputs_cpi_data.rs:75(this file)create_inputs_cpi_data.rs:62No stale references to the old function name remain. The versioning strategy (v1 for legacy, v2 for current) is intentionally applied.
programs/registry/src/account_compression_cpi/rollover_batched_address_tree.rs (1)
50-53: Address network fee configuration is correctly sourced.Verification confirms both
address_network_feeandnetwork_feefields exist inProtocolConfigand are intentionally used for their respective operations. The CPI seed/signer pattern andif_equals_zero_u64wrapper are correct.programs/registry/src/errors.rs (1)
31-34: Add #[msg] for error visibility, but discriminant concern is invalid.The enum lacks #[msg] on ProgramOwnerDefined for better error reporting to clients. However, the discriminant stability risk is unfounded: adding #[msg] does not alter enum variant values, and no code relies on numeric error positions. All 60+ error references use symbolic RegistryError:: calls or standard Solana error casting, not hardcoded numeric codes.
Apply the suggestion for improved error messages:
- ProgramOwnerDefined, + #[msg("Program owner already defined")] + ProgramOwnerDefined,programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs (2)
4-4: Import/use changes look fine.No issues with the re-export path; consistent with program-libs layout.
42-44: Verify feature enablement for test validation path.The feature
testis properly defined inprograms/account-compression/Cargo.tomland correctly gates the strict validation via#[cfg(feature = "test")]. However,program-tests/account-compression-test/declaresaccount-compression = { workspace = true }without enabling this feature. Since the program's default features are empty, the strict validation code will not execute during tests—only the fallback logic will run. Either enable--features testwhen building the program in tests, or confirm this is intentional.programs/system/src/processor/create_inputs_cpi_data.rs (1)
62-66: Setting v2 network fee early is good.Fee is set alongside selecting the batched tree and before sequence handling; ordering looks correct.
programs/system/src/accounts/remaining_account_checks.rs (1)
134-134: Code is correct: all v1 discriminators that require legacy context have it properly set.Verification confirms:
STATE_MERKLE_TREE_ACCOUNT_DISCRIMINATOR(line 128): context retrieved in both input and output hashingADDRESS_MERKLE_TREE_ACCOUNT_DISCRIMINATOR(line 167): context retrieved for address creationQUEUE_ACCOUNT_DISCRIMINATORAddressV1 (line 201): context retrieved for address creationAll three
set_legacy_merkle_contextcalls are necessary and actively used across the processor pipeline.programs/registry/src/lib.rs (3)
335-350: LGTM! Correct enforcement of program-owned tree restriction.The validation correctly forbids program-owned state trees and uses the appropriate
ProgramOwnerDefinederror. The network fee validation logic properly distinguishes between light-forester-serviced trees (with network_fee) and custom-forester trees (without network_fee but with explicit forester).
516-518: Breaking change: batched state trees now require network fees.The else branch at lines 516-518 changes the behavior from allowing trees without network fees (if they have a custom forester) to requiring all batched state trees to have network fees. This enforces that batched state trees can only be serviced by light foresters.
This is a significant policy change that prevents custom foresters from operating batched state trees. Ensure this aligns with the intended deprecation strategy.
Based on learnings.
604-606: Breaking change: batched address trees now require network fees.Similar to batched state trees, this changes the behavior to require all batched address trees to have network fees. Custom foresters can no longer operate batched address trees.
Ensure this policy change is intentional and documented.
Based on learnings.
program-tests/system-test/tests/test.rs (2)
926-927: LGTM! Consistent field renames for versioned input tracking.All occurrences of
TransactionParamscorrectly update fromnum_input_compressed_accountstov1_input_compressed_accountsand add the newv2_input_compressed_accountsboolean field. The changes are mechanically consistent across the test file.Also applies to: 1067-1068, 2016-2017, 2636-2637
1711-1716: Informative deprecation comment.The comment correctly documents that V1 address trees are deprecated and no longer regenerated in tests, relying on existing JSON files in devenv mode. This provides useful context for test maintenance.
program-tests/registry-test/tests/tests.rs (4)
490-527: Test logic inconsistency: conflicting program_owner and forester requirements.The test at lines 505-527 creates an address V1 tree with:
program_owner: Some(Pubkey::new_unique())(line 516)forester: None(line 517)network_fee: None(line 520)Based on the registry code changes (lines 294-311 in lib.rs), this should fail because:
- Line 307-311 in lib.rs requires
forester.is_some()even for program-owned treesHowever, the test expects
ForesterUndefinederror (line 526), which suggests the test is correctly expecting failure. But the comment says "Deprecated should fail" and "initialize a Merkle tree without a forester", implying the test validates that deprecated V1 trees without foresters are rejected.This aligns with the registry logic, but the registry logic itself is contradictory (as noted in the earlier review comment).
Verify that the test expectations align with the corrected registry validation logic after fixing the issue identified in lines 294-311 of lib.rs.
1808-1816: Test validates breaking change: custom forester state trees now forbidden.The test at lines 1808-1816 confirms that attempting to create a batched state tree with:
forester: Some(custom_forester)network_fee: NoneNow fails with
InvalidNetworkFee. This validates the breaking change that batched state trees must have network fees and cannot use custom foresters.
2083-2133: Good test coverage for mainnet initialization constraints.The new tests validate that mainnet initialization (without test features) enforces production-ready parameters and rejects test-only configurations. This helps prevent accidental deployment of test trees to mainnet.
1671-1671: Verify unused parameter: _input – Review comment is incorrect.The
_inputparameter in themigrate_statefunction is not unused. Inprograms/account-compression/src/lib.rs(line 297), it's actively passed toprocess_migrate_state(&_ctx, _input). The underscore prefix is a Rust naming convention, not an indicator that the parameter is unused. The migration operation properly receives and uses its input parameters.Likely an incorrect or invalid review comment.
sdk-libs/client/src/fee.rs (1)
76-86: Fee calculation logic for v1/v2 inputs is correctly implemented and intentionally asymmetric.Based on codebase verification:
- v1 implementation (lines 77-78): Fee scales linearly with
v1_input_compressed_accountscount- v2 implementation (line 80): Fee is flat, charged once per transaction regardless of batch size
This asymmetry is confirmed as intentional and correct:
- Both code paths are tested extensively in
program-tests/system-test/tests/test.rs(e.g., lines 2016-2023)- The if/else if structure explicitly ensures mutual exclusion between v1 and v2 charging
- Registry program validation in
programs/registry/src/lib.rsaccepts both fee structures without complaint- The
TransactionParamsstructure correctly distinguishes v1 as a count (u8) and v2 as a flag (bool), supporting the different charging modelsThe flat fee for v2 batched trees makes sense architecturally—since v2 processes multiple inputs in a single batch atomically, charging a single fee aligns with the batched processing model.
program-tests/utils/src/e2e_test_env.rs (2)
1317-1317: LGTM - Address network fee update aligns with protocol changes.The increase from 5000 to 10000 lamports correctly reflects the documented address creation fee structure introduced in this PR. Per the updated documentation in
context.rs, address creation charges 10000 lamports per address.
1546-1564: LGTM - TransactionParams refactoring correctly implements versioned input accounting.The replacement of
num_input_compressed_accountswith separatev1_input_compressed_accountsandv2_input_compressed_accountsfields correctly implements the new fee model:
- StateV2 (batched trees):
inputs=0, is_v2=true- fees charged once per tree regardless of input count- StateV1 (legacy trees):
inputs=count, is_v2=false- fees charged per inputThis pattern is consistently applied across all transaction types throughout the file (sol transfers, compressions, address creation, spl operations), ensuring uniform fee handling.
programs/system/src/context.rs (6)
12-15: LGTM - Necessary import for overflow protection.The addition of
ProgramErrorimport enables proper error handling for arithmetic overflow checks introduced in the fee accumulation methods.
43-43: LGTM - Field addition supports separate network fee tracking.The
network_feefield enables independent tracking of network fees in the legacy Merkle tree context, supporting the PR's fee separation objectives.
73-78: LGTM - Appropriate fee charging for V2 batched trees.The
set_network_fee_v2method correctly implements the "charge once per tree" model for V2 batched state trees. Thenetwork_fee_is_setguard prevents double-charging, and no overflow check is needed since this performs a single insertion rather than accumulation.
80-93: LGTM - Proper fee accumulation for V1 legacy trees with overflow protection.The
set_network_fee_v1method correctly implements per-input fee accumulation for V1 trees with appropriate overflow protection. The logic mirrorsset_address_fee, allowing both address fees and network fees to accumulate for the same tree index, which is the expected behavior for comprehensive fee accounting.
165-178: LGTM - Clear and comprehensive fee structure documentation.The updated documentation provides excellent clarity on the network fee distribution model:
- V1 state trees: per-input charging (5000 lamports each)
- V2 batched state trees: per-tree charging (5000 lamports once)
- Address creation: per-address charging (10000 lamports each)
The concrete examples help developers understand how fees accumulate across different transaction types.
59-71: LGTM - Critical security improvement with proper error handling throughout.The addition of
checked_addandResult<()>return type effectively prevents arithmetic overflow vulnerabilities. Both callers inderive_new_addressescorrectly propagate the Result using the?operator, ensuring errors are handled appropriately up the call stack.programs/system/Cargo.toml (1)
25-27: Clarify the removal of debug and readonly features.The
readonlyfeature (line 27) remains defined but is no longer in the default set. Additionally, based on the AI summary, adebugfeature was removed from defaults but is not visible in the provided code snippet—it appears to have been entirely removed.Please clarify:
- readonly: Is this feature still used elsewhere, or should it be removed entirely if no longer needed?
- debug: Was this feature completely removed, or is it still defined elsewhere in the codebase?
This will help confirm the simplification is intentional and complete.
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 (4)
programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs (1)
63-72: Unreachable code: The None case is impossible in production builds.Lines 57-59 ensure that
registered_program_pdacannot beNonein non-test builds, making theNonebranch at line 71 unreachable in production. This creates maintenance confusion because the code structure suggests flexibility that doesn't exist at runtime.Consider:
- Extracting the non-test authorization logic into a separate function to make the control flow clearer
- Adding a comment explaining that the None case only applies to test builds
- If the flexibility is intentional for tests, document why tests require different ownership semantics
Example refactor to clarify the flow:
#[cfg(not(feature = "test"))] { use crate::errors::AccountCompressionErrorCode; if params != InitAddressTreeAccountsInstructionData::default() { return err!(AccountCompressionErrorCode::UnsupportedParameters); } - if let Some(registered_program_pda) = ctx.accounts.registered_program_pda.as_ref() { - if registered_program_pda.group_authority_pda - != pubkey!("24rt4RgeyjUCWGS2eF7L7gyNMuz6JWdqYpAvb1KRoHxs") - { - return err!(AccountCompressionErrorCode::UnsupportedParameters); - } - } else { - return err!(AccountCompressionErrorCode::UnsupportedParameters); - } + // Production builds require authorized group PDA + let registered_program_pda = ctx.accounts.registered_program_pda.as_ref() + .ok_or(AccountCompressionErrorCode::UnsupportedParameters)?; + if registered_program_pda.group_authority_pda + != pubkey!("24rt4RgeyjUCWGS2eF7L7gyNMuz6JWdqYpAvb1KRoHxs") + { + return err!(AccountCompressionErrorCode::UnsupportedParameters); + } } // Check signer. +// Note: In production, registered_program_pda is guaranteed to be Some by the check above. +// The None case only applies when running with the 'test' feature. let owner = match ctx.accounts.registered_program_pda.as_ref() {programs/registry/src/lib.rs (1)
356-365: On-chain panic risk: unwrap() on optional CPI context.Avoid unwrap() in programs; it aborts the TX. Return a typed error if missing and proceed safely.
- check_cpi_context( - ctx.accounts - .cpi_context_account - .as_ref() - .unwrap() - .to_account_info(), - &ctx.accounts.protocol_config_pda.config, - )?; + let cpi_ai = ctx + .accounts + .cpi_context_account + .as_ref() + .ok_or(RegistryError::MissingCpiContext)? + .to_account_info(); + check_cpi_context(cpi_ai, &ctx.accounts.protocol_config_pda.config)?;If RegistryError::MissingCpiContext does not exist, add it in errors.rs. Based on learnings.
programs/system/src/context.rs (2)
154-163: Overflow risk in set_rollover_fee; align with checked_add + fallible return.This path still uses
+=without overflow checks and returns()unlike the new setters.- pub fn set_rollover_fee(&mut self, ix_data_index: u8, fee: u64) { + pub fn set_rollover_fee(&mut self, ix_data_index: u8, fee: u64) -> Result<()> { let payment = self .rollover_fee_payments .iter_mut() .find(|a| a.0 == ix_data_index); match payment { - Some(payment) => payment.1 += fee, - None => self.rollover_fee_payments.push((ix_data_index, fee)), + Some(payment) => { + payment.1 = payment.1.checked_add(fee).ok_or(ProgramError::ArithmeticOverflow)? + } + None => self.rollover_fee_payments.push((ix_data_index, fee)), }; + Ok(()) }Update call sites accordingly.
181-186: Possible panic on out-of-bounds account index in transfer_fees.Indexing
accounts[*i as usize]can panic. Return a typed error instead, consistent withget_index_or_insert.- for (i, fee) in self.rollover_fee_payments.iter() { - transfer_lamports_invoke(fee_payer, &accounts[*i as usize], *fee)?; - } + for (i, fee) in self.rollover_fee_payments.iter() { + let account = accounts + .get(*i as usize) + .ok_or(SystemProgramError::PackedAccountIndexOutOfBounds)?; + transfer_lamports_invoke(fee_payer, account, *fee)?; + }Prevents program aborts on malformed indices.
♻️ Duplicate comments (1)
programs/registry/src/lib.rs (1)
592-599: Remove debug logging of fees (duplicate of prior review).The msg! prints add log noise and compute cost; drop them before release.
- if network_fee != ctx.accounts.protocol_config_pda.config.address_network_fee { - msg!( - "ctx.accounts.protocol_config_pda.config.address_network_fee {:?}", - ctx.accounts.protocol_config_pda.config.address_network_fee - ); - msg!("network_fee {:?}", network_fee); + if network_fee != ctx.accounts.protocol_config_pda.config.address_network_fee { return err!(RegistryError::InvalidNetworkFee); }
🧹 Nitpick comments (2)
js/stateless.js/tests/e2e/compress.test.ts (1)
48-59: Fee calculation logic now adapts to V1/V2 semantics.The updated logic correctly implements versioned fee semantics:
- V2: Charges
STATE_MERKLE_TREE_NETWORK_FEEonce per transaction (if inputs or outputs exist)- V1: Charges
STATE_MERKLE_TREE_NETWORK_FEEper input account- Adds
networkAddressFeeusing the updatedADDRESS_TREE_NETWORK_FEEconstantHowever, the nested ternary operators (lines 48-54) reduce readability.
Consider extracting the conditional logic into a helper function for clarity:
- const networkInFee = tx.in - ? featureFlags.isV2() - ? STATE_MERKLE_TREE_NETWORK_FEE - : STATE_MERKLE_TREE_NETWORK_FEE.mul(bn(tx.in)) - : tx.out && featureFlags.isV2() - ? STATE_MERKLE_TREE_NETWORK_FEE - : bn(0); + const networkInFee = (() => { + if (featureFlags.isV2()) { + // V2: charge once if there are inputs or outputs + return tx.in || tx.out ? STATE_MERKLE_TREE_NETWORK_FEE : bn(0); + } + // V1: charge per input + return tx.in ? STATE_MERKLE_TREE_NETWORK_FEE.mul(bn(tx.in)) : bn(0); + })();programs/system/src/context.rs (1)
59-71: Good: switched to checked_add and fallible API; consider de‑duping with a small helper.Both setters share identical “find-or-insert + checked_add” logic. Extract a private helper to reduce drift:
+impl SystemContext<'_> { + #[inline] + fn add_fee_checked(&mut self, index: u8, fee: u64) -> Result<()> { + if let Some(p) = self.rollover_fee_payments.iter_mut().find(|a| a.0 == index) { + p.1 = p.1.checked_add(fee).ok_or(ProgramError::ArithmeticOverflow)?; + } else { + self.rollover_fee_payments.push((index, fee)); + } + Ok(()) + } +} @@ - pub fn set_address_fee(&mut self, fee: u64, index: u8) -> Result<()> { - let payment = self.rollover_fee_payments.iter_mut().find(|a| a.0 == index); - match payment { /* ... */ } - Ok(()) - } + pub fn set_address_fee(&mut self, fee: u64, index: u8) -> Result<()> { + self.add_fee_checked(index, fee) + } @@ - pub fn set_network_fee_v1(&mut self, fee: u64, index: u8) -> Result<()> { - let payment = self.rollover_fee_payments.iter_mut().find(|a| a.0 == index); - match payment { /* ... */ } - Ok(()) - } + pub fn set_network_fee_v1(&mut self, fee: u64, index: u8) -> Result<()> { + self.add_fee_checked(index, fee) + }Keeps arithmetic and error handling consistent across fee types.
Also applies to: 80-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
js/stateless.js/src/constants.ts(1 hunks)js/stateless.js/tests/e2e/compress.test.ts(3 hunks)js/stateless.js/tests/e2e/test-rpc.test.ts(1 hunks)programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs(2 hunks)programs/registry/src/lib.rs(5 hunks)programs/system/src/context.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
programs/registry/src/lib.rs
📄 CodeRabbit inference engine (programs/registry/CLAUDE.md)
programs/registry/src/lib.rs: Wrapper instruction handlers must: (1) load the target account metadata; (2) call check_forester with correct authority, target, forester PDA (optional), and computed work_units; (3) delegate via a CPI using a PDA signer.
Instruction handlers must compute work_units by operation type: batch operations use account.queue_batches.batch_size; single operations use DEFAULT_WORK_V1; custom operations compute based on complexity.
Load accounts according to type before check_forester: batched via BatchedMerkleTreeAccount::type_from_account_info(); regular via ctx.accounts.account.load()?.metadata; use custom deserialization when required.
Apply forester eligibility rules: with a forester PDA, validate epoch registration, eligibility, track work, and require network fee; without a forester PDA, ensure authority matches the tree’s designated forester.
Files:
programs/registry/src/lib.rs
🧠 Learnings (6)
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/lib.rs : Load accounts according to type before check_forester: batched via BatchedMerkleTreeAccount::type_from_account_info(); regular via ctx.accounts.account.load()?.metadata; use custom deserialization when required.
Applied to files:
programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/lib.rs : Apply forester eligibility rules: with a forester PDA, validate epoch registration, eligibility, track work, and require network fee; without a forester PDA, ensure authority matches the tree’s designated forester.
Applied to files:
programs/registry/src/lib.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/error.rs : Maintain stable mapping of AccountError to ProgramError, including Pinocchio code mapping (1–11), in error.rs
Applied to files:
programs/system/src/context.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs} : Gate SDK-specific implementations with #[cfg(feature = "solana"|"pinocchio"|"test-only")]
Applied to files:
programs/system/src/context.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/!(account_info)/**/*.rs : Use AccountInfoTrait for runtime-agnostic account handling; avoid direct solana-program or pinocchio AccountInfo in general logic
Applied to files:
programs/system/src/context.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs} : Provide SDK-specific AccountInfoTrait implementations in account_info/{solana.rs,pinocchio.rs,test_account_info.rs}
Applied to files:
programs/system/src/context.rs
🧬 Code graph analysis (3)
programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs (1)
program-libs/batched-merkle-tree/src/initialize_address_tree.rs (3)
init_batched_address_merkle_tree_from_account_info(57-81)validate_batched_address_tree_params(132-167)default(36-51)
js/stateless.js/tests/e2e/compress.test.ts (2)
js/stateless.js/src/constants.ts (3)
featureFlags(16-35)STATE_MERKLE_TREE_NETWORK_FEE(356-356)ADDRESS_TREE_NETWORK_FEE(361-361)js/stateless.js/src/state/bn.ts (1)
bn(3-12)
js/stateless.js/tests/e2e/test-rpc.test.ts (1)
js/stateless.js/src/constants.ts (2)
featureFlags(16-35)STATE_MERKLE_TREE_NETWORK_FEE(356-356)
⏰ 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). (21)
- GitHub Check: lint
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: system-programs (token test, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: cli-v2
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: Forester e2e test
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
🔇 Additional comments (13)
js/stateless.js/src/constants.ts (1)
361-361: Address network fee doubled from 5000 to 10000.The constant ADDRESS_TREE_NETWORK_FEE changed from new BN(5000) to new BN(10000), effectively doubling the per-address creation fee. This aligns with the PR objective to raise address network fees and matches other changes in the PR that raise default network_fee values to 10000.
Ensure that:
- This breaking change is documented in release notes or migration guides
- Dependent systems and integrations are aware of the fee increase
- The rationale for the 100% increase is communicated to users
js/stateless.js/tests/e2e/test-rpc.test.ts (1)
63-73: Conditional fee calculation adapts to V1/V2 behavior.The test now correctly computes
expectedFeebased on featureFlags.isV2(), charging STATE_MERKLE_TREE_NETWORK_FEE for V2 and zero for V1. This aligns with the PR's versioned fee semantics and improves test maintainability.js/stateless.js/tests/e2e/compress.test.ts (1)
241-244: Good refactoring: compute fee once and reuse.Storing the result in
expectedTxFeesand reusing it eliminates redundant computation and improves readability.programs/account-compression/src/instructions/initialize_batched_address_merkle_tree.rs (2)
51-59: Add documentation explaining the intentional group authority security gate for batched operations.This hardcoded check is a deliberate security gate (feature-gated for production only), but lacks documentation. Add a comment explaining:
- Why only the batched initialization functions require this specific group authority
- That
24rt4RgeyjUCWGS2eF7L7gyNMuz6JWdqYpAvb1KRoHxsis a Light Protocol group authority PDA- That this gate is intentional and not intended to be configurable
41-60: ****The test at
program-tests/registry-test/tests/tests.rs:1838-1844does not exercise custom parameters as a passing case. Rather, it validates that custom parameters are properly rejected:assert_rpc_error(result, 1, RegistryError::InvalidNetworkFee.into()).unwrap()confirms the initialization fails when non-default parameters are provided. This test actually validates the production restriction, meaning there is no testing gap—the code correctly rejects non-default parameters in both test and production environments when the appropriate validation runs.Likely an incorrect or invalid review comment.
programs/registry/src/lib.rs (6)
335-340: Program-owned state trees disabled — OK.The ownership guard is correct and uses ProgramOwnerDefined consistently.
503-507: Program-owned batched State trees disabled — OK.Consistent with the State V2 ownership policy.
516-518: Require network_fee for batched State init — OK.For light foresters, fee must be present and match config.
586-590: Program-owned batched Address trees disabled — OK.Matches the V2 ownership constraints.
604-606: Explicitly error on missing fee — OK.Keeps init paths unambiguous for light foresters.
342-346: Code is correct; State/Address tree fee fields properly split in ProtocolConfig.Verification confirms the split you questioned is implemented correctly:
- ProtocolConfig defines
network_fee(State trees) andaddress_network_fee(Address trees) on lines 37 and 42- No "state_network_fee" field exists
- Lines 342–346 correctly compare State tree fees against
config.network_fee- Address tree fees consistently use
config.address_network_fee(e.g., line 591)- CPI calls reflect this same pattern throughout
programs/system/src/context.rs (2)
43-44: No concerns—MerkleTreeContext is purely in-memory and not serialized.MerkleTreeContext has no serialization derives (Borsh, ZeroCopy, bytemuck, Account) and is only used in-memory within SystemContext's runtime state. Adding the
network_feefield poses no ABI or layout risks.
12-15: No action needed—review comment is incorrect.The code uses the correct type matching. The
Result<T>type alias inprograms/system/src/lib.rs:48is defined asstd::result::Result<T, ProgramError>whereProgramErrorispinocchio::program_error::ProgramError. The?operator on.ok_or(ProgramError::ArithmeticOverflow)incontext.rs(lines 66, 88) produces the same error type, so propagation works without any conversion impl. NoFrom<pinocchio::program_error::ProgramError>implementation is needed because the error type is already the pinocchio type directly.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
programs/registry/src/lib.rs (1)
593-597: Remove debug logging from production code.These debug
msg!calls printing protocol config and network fee values should be removed before merging to production as they add unnecessary noise to transaction logs and increase compute costs.Apply this diff:
if network_fee != ctx.accounts.protocol_config_pda.config.address_network_fee { - msg!( - "ctx.accounts.protocol_config_pda.config.address_network_fee {:?}", - ctx.accounts.protocol_config_pda.config.address_network_fee - ); - msg!("network_fee {:?}", network_fee); return err!(RegistryError::InvalidNetworkFee); }
🧹 Nitpick comments (4)
programs/registry/src/errors.rs (1)
33-34: Add descriptive error messages for new variants.The new error variants
ProgramOwnerDefinedandProgramOwnerUndefinedlack#[msg(...)]attributes. While some variants in this enum omit messages, adding descriptive messages improves developer experience and debugging.Apply this diff to add error messages:
#[msg("Insufficient funds in pool")] InsufficientFunds, + #[msg("Program owner must not be defined")] ProgramOwnerDefined, + #[msg("Program owner must be defined")] ProgramOwnerUndefined,programs/registry/src/lib.rs (2)
516-518: Add explanatory message for mandatory network fee requirement.The else branch now requires
network_feeto be defined for batched state trees, representing a breaking change from optional behavior. Adding amsg!()call would help developers understand whyNoneis invalid.Apply this diff:
} else { + msg!("Network fee is required for batched state trees serviced by light foresters."); return err!(RegistryError::InvalidNetworkFee); }
604-606: Add explanatory message for mandatory network fee requirement.The else branch now requires
network_feeto be defined for batched address trees, similar to the change in batched state trees. Adding amsg!()call would help developers understand whyNoneis invalid.Apply this diff:
} else { + msg!("Network fee is required for batched address trees serviced by light foresters."); return err!(RegistryError::InvalidNetworkFee); }sdk-libs/client/src/fee.rs (1)
69-69: Update comment to reflect v1 vs v2 fee charging models.The comment states "Network fee is charged per input and per address," but this is only accurate for v1 transactions. For v2, the network fee is charged as a flat rate (line 77), not per input.
Update the comment to clarify:
- // Network fee is charged per input and per address + // Network fee: v1 charges per input, v2 charges flat rate; both charge per address let mut network_fee: i64 = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
programs/registry/src/errors.rs(1 hunks)programs/registry/src/lib.rs(5 hunks)programs/system/src/errors.rs(2 hunks)programs/system/src/processor/create_address_cpi_data.rs(2 hunks)programs/system/src/processor/create_inputs_cpi_data.rs(2 hunks)programs/system/src/processor/create_outputs_cpi_data.rs(2 hunks)sdk-libs/client/src/fee.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- programs/system/src/processor/create_outputs_cpi_data.rs
- programs/system/src/processor/create_address_cpi_data.rs
🧰 Additional context used
📓 Path-based instructions (1)
programs/registry/src/lib.rs
📄 CodeRabbit inference engine (programs/registry/CLAUDE.md)
programs/registry/src/lib.rs: Wrapper instruction handlers must: (1) load the target account metadata; (2) call check_forester with correct authority, target, forester PDA (optional), and computed work_units; (3) delegate via a CPI using a PDA signer.
Instruction handlers must compute work_units by operation type: batch operations use account.queue_batches.batch_size; single operations use DEFAULT_WORK_V1; custom operations compute based on complexity.
Load accounts according to type before check_forester: batched via BatchedMerkleTreeAccount::type_from_account_info(); regular via ctx.accounts.account.load()?.metadata; use custom deserialization when required.
Apply forester eligibility rules: with a forester PDA, validate epoch registration, eligibility, track work, and require network fee; without a forester PDA, ensure authority matches the tree’s designated forester.
Files:
programs/registry/src/lib.rs
🧠 Learnings (4)
📚 Learning: 2025-10-16T06:33:19.426Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/compressible/CLAUDE.md:0-0
Timestamp: 2025-10-16T06:33:19.426Z
Learning: Applies to program-libs/compressible/src/error.rs : Define error types with numeric codes in the 19xxx range and propagate hasher errors in the 7xxx range; include ProgramError conversions (Anchor, Pinocchio, Solana) in src/error.rs
Applied to files:
programs/system/src/errors.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/error.rs : Maintain stable mapping of AccountError to ProgramError, including Pinocchio code mapping (1–11), in error.rs
Applied to files:
programs/system/src/errors.rsprograms/registry/src/errors.rs
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/account_compression_cpi/*.rs : Context structs for wrapper instructions must include standard accounts: optional registered_forester_pda (mut), authority Signer, cpi_authority with seeds/bump for CPI_AUTHORITY_PDA_SEED, registered_program_pda, target program handle, log_wrapper, and mutable target_account.
Applied to files:
programs/system/src/processor/create_inputs_cpi_data.rs
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/lib.rs : Apply forester eligibility rules: with a forester PDA, validate epoch registration, eligibility, track work, and require network fee; without a forester PDA, ensure authority matches the tree’s designated forester.
Applied to files:
programs/registry/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: lint
- GitHub Check: cli-v2
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: system-programs (token test, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: Forester e2e test
- GitHub Check: stateless-js-v2
🔇 Additional comments (9)
programs/system/src/errors.rs (1)
143-144: LGTM! Error variant correctly added.The new
MissingLegacyMerkleContexterror variant is well-defined with a clear message and correctly mapped to error code 6066 in the conversion implementation. This follows the existing pattern and provides proper error handling for missing legacy Merkle tree contexts.Also applies to: 228-228
programs/system/src/processor/create_inputs_cpi_data.rs (2)
62-62: LGTM! Versioned method aligns with V2 semantics.The call to
set_network_fee_v2forBatchedStateTreeappropriately uses the versioned method, distinguishing V2 batched trees from legacy V1 trees handled below.
79-85: Excellent fix! Previous panic risk resolved with proper error handling.The unwrap has been replaced with
.ok_or(SystemProgramError::MissingLegacyMerkleContext)?, which was the critical safety concern raised in the previous review. This change:
- Prevents on-chain panics when legacy context is missing
- Explicitly extracts
network_feeandhashed_pubkeyfrom the legacy context- Uses versioned
set_network_fee_v1for V1 state trees- Properly propagates errors via the
?operatorThe new error variant has been correctly added to
SystemProgramErrorwith error code 6066.programs/registry/src/lib.rs (2)
294-311: Address V1 deprecation policy correctly enforced.The updated logic properly implements the deprecation policy for Address V1 trees:
- Requires program ownership (lines 298-301)
- Disallows network fees (lines 302-305)
- Requires a designated forester (lines 308-310)
The error variants
ProgramOwnerUndefinedandInvalidNetworkFeeare used appropriately, and the comments clearly explain the constraints.
335-339: State tree ownership policy correctly enforced.The check properly rejects program-owned state trees using the new
ProgramOwnerDefinederror variant. This aligns with the architectural distinction between Address V1 trees (which must be program-owned) and state trees (which must not be).sdk-libs/client/src/fee.rs (4)
28-28: Verify the address network fee increase is intentional.The
address_network_feehas been increased from the default of 5,000 to 10,000 in both theDefaultandtest_batched()configurations. This doubles the cost per new address created (line 81-82). Ensure this change aligns with economic/pricing requirements and has been communicated to stakeholders.Also applies to: 41-41
49-50: Clarify the behavior when both v1 and v2 input fields are set.The fee logic (lines 73-78) uses
if v1 != 0followed byelse if v2, meaning that if bothv1_input_compressed_accounts > 0ANDv2_input_compressed_accounts = true, only the v1 fee will be charged. Consider:
- Adding validation to reject transactions with both fields set if this combination is invalid
- Documenting the precedence if the combination is valid but discouraged
- Returning an error if this represents a malformed transaction
Should this code enforce mutual exclusivity between v1 and v2 input fields? If yes, add validation before the fee calculation:
if transaction_params.v1_input_compressed_accounts != 0 && transaction_params.v2_input_compressed_accounts { return Err(RpcError::CustomError( "Cannot set both v1 and v2 input compressed accounts".to_string() )); }
73-78: LGTM: Fee calculation correctly implements v1 per-input and v2 flat-rate charging.The fee calculation properly handles the two versioning schemes:
- v1: Charges
network_fee * v1_input_compressed_accountsfor per-input billing- v2: Charges a single
network_feeas a flat rateThe type conversions from
u8/u64toi64are safe and won't overflow.
80-83: LGTM: Address fee calculation and enhanced debug logging.The per-address fee calculation correctly multiplies
address_network_feeby the number of new addresses. The added debug logging for the fullfee_config(lines 112-115) will improve troubleshooting when fee mismatches occur.Also applies to: 112-115
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
program-tests/registry-test/tests/tests.rs (1)
1696-1816: Update misleading test documentation comment.The test documentation at line 1700 states:
/// 4. failing: create with state tree with custom forester and invalid non-zero network feeHowever, the actual test implementation at line 1801 sets
params.network_fee = None, not a non-zero value. The comment should be updated to accurately reflect what the test validates.Apply this diff to correct the documentation:
-/// 4. failing: create with state tree with custom forester and invalid non-zero network fee +/// 4. failing: create with state tree with custom forester and None network fee (unsupported configuration)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
program-tests/registry-test/tests/tests.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-16T06:33:19.426Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/compressible/CLAUDE.md:0-0
Timestamp: 2025-10-16T06:33:19.426Z
Learning: Applies to program-libs/compressible/src/config.rs : Ensure serialization compatibility across Anchor, Pinocchio, and Borsh for core account types used by dependent programs
Applied to files:
program-tests/registry-test/tests/tests.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs} : Gate SDK-specific implementations with #[cfg(feature = "solana"|"pinocchio"|"test-only")]
Applied to files:
program-tests/registry-test/tests/tests.rs
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/lib.rs : Load accounts according to type before check_forester: batched via BatchedMerkleTreeAccount::type_from_account_info(); regular via ctx.accounts.account.load()?.metadata; use custom deserialization when required.
Applied to files:
program-tests/registry-test/tests/tests.rs
🧬 Code graph analysis (1)
program-tests/registry-test/tests/tests.rs (12)
sdk-libs/program-test/src/utils/assert.rs (1)
assert_rpc_error(7-144)sdk-libs/program-test/src/program_test/rpc.rs (2)
None(472-472)new(36-43)sdk-libs/client/src/fee.rs (1)
default(19-31)program-libs/batched-merkle-tree/src/initialize_address_tree.rs (2)
default(36-51)test_default(194-209)programs/account-compression/src/instructions/initialize_address_merkle_tree_and_queue.rs (1)
default(37-48)programs/registry/src/protocol_config/state.rs (1)
default(51-70)program-libs/compressed-account/src/lib.rs (1)
default(200-202)sdk-libs/program-test/src/program_test/config.rs (3)
default(121-161)new(58-67)default_test_forester(97-105)program-libs/batched-merkle-tree/src/initialize_state_tree.rs (2)
default(45-63)test_default(292-310)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(66-362)program-tests/utils/src/lib.rs (1)
create_address_merkle_tree_and_queue_account_with_assert(78-182)program-tests/utils/src/batched_address_tree.rs (1)
create_address_merkle_tree_and_queue_account_with_assert(20-120)
⏰ 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). (21)
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v2
- GitHub Check: lint
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: system-programs (token test, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: Forester e2e test
- GitHub Check: stateless-js-v2
🔇 Additional comments (5)
program-tests/registry-test/tests/tests.rs (5)
108-108: LGTM! ProtocolConfig field correctly updated.The field rename from
place_holder_atoaddress_network_feealigns with the ProtocolConfig changes across the codebase, and the value of 10000 matches the documented defaults.Also applies to: 127-127
429-429: LGTM! Error expectation matches test inputs.The test correctly expects
ProgramOwnerUndefinedwhen attempting to initialize a state Merkle tree without a program owner or forester.
528-549: LGTM! Previous issue resolved.The test now correctly expects
ProgramOwnerUndefinedwhenprogram_ownerisNonebutforesterisSome(...). This properly validates the invariant that both must be provided together for program-owned trees.
1671-1671: LGTM! MigrateState field correctly updated.The field rename from
inputto_inputaligns with the updatedaccount_compression::instruction::MigrateStatestructure.
2083-2133: LGTM! Good validation for production readiness.These ignored tests properly validate that:
- V2 trees can initialize with production-safe default configs
- Test-only configs (using
test_default()) are rejected in production buildsThe
#[ignore]attributes with descriptive reasons are appropriate since these tests require building the account compression program without test features.
ananas-block
force-pushed
the
jorrit/chore-pre-release
branch
from
bf961c5 to
8633b7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
program-tests/registry-test/tests/tests.rs (3)
505-506: Missing forester should fail with ForesterUndefined.Test reflects the deprecation and expected error. Consider clarifying the comment text.
-// Deprecated should fail -// initialize a Merkle tree without a forester +// V1 address trees without a forester are deprecated and must fail +// Expect RegistryError::ForesterUndefinedAlso applies to: 510-527
550-571: V1 address: reject non-zero network_fee.Assertion against InvalidNetworkFee is correct. Minor: use a consistent error-code conversion style across the file (prefer
.into()everywhere) to avoid offset mistakes.
1809-1815: Init failure path: verify asserted instruction index.LightProgramTest::new performs multiple setup instructions; index 3 can drift. Please re-run to confirm the index, or consider a helper that asserts by error code irrespective of index to reduce brittleness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
program-tests/registry-test/tests/tests.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-16T06:33:19.426Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/compressible/CLAUDE.md:0-0
Timestamp: 2025-10-16T06:33:19.426Z
Learning: Applies to program-libs/compressible/src/config.rs : Ensure serialization compatibility across Anchor, Pinocchio, and Borsh for core account types used by dependent programs
Applied to files:
program-tests/registry-test/tests/tests.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs} : Gate SDK-specific implementations with #[cfg(feature = "solana"|"pinocchio"|"test-only")]
Applied to files:
program-tests/registry-test/tests/tests.rs
📚 Learning: 2025-10-15T03:46:26.767Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/registry/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:26.767Z
Learning: Applies to programs/registry/src/lib.rs : Load accounts according to type before check_forester: batched via BatchedMerkleTreeAccount::type_from_account_info(); regular via ctx.accounts.account.load()?.metadata; use custom deserialization when required.
Applied to files:
program-tests/registry-test/tests/tests.rs
🧬 Code graph analysis (1)
program-tests/registry-test/tests/tests.rs (8)
sdk-libs/client/src/fee.rs (1)
default(19-31)program-libs/batched-merkle-tree/src/initialize_address_tree.rs (2)
default(36-51)test_default(194-209)programs/account-compression/src/instructions/initialize_address_merkle_tree_and_queue.rs (1)
default(37-48)sdk-libs/program-test/src/program_test/config.rs (3)
default(121-161)new(58-67)default_test_forester(97-105)program-libs/batched-merkle-tree/src/initialize_state_tree.rs (2)
default(45-63)test_default(292-310)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(66-362)program-tests/utils/src/lib.rs (1)
create_address_merkle_tree_and_queue_account_with_assert(78-182)sdk-libs/program-test/src/utils/assert.rs (1)
assert_rpc_error(7-144)
⏰ 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). (21)
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: stateless-js-v1
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: Forester e2e test
- GitHub Check: cli-v2
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: lint
- GitHub Check: cli-v1
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: system-programs (token test, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: stateless-js-v2
🔇 Additional comments (7)
program-tests/registry-test/tests/tests.rs (7)
108-108: ProtocolConfig: address_network_fee wired correctly (10000).Matches SDK/client defaults and init params. LGTM.
Also applies to: 127-127
490-491: V1 address init: network_fee None for both tree and queue.Semantics are correct for V1 (no per-address network fee). LGTM.
Also applies to: 496-499
528-549: Missing program owner should fail with ProgramOwnerUndefined.Good correction; aligns with prior feedback and registry logic.
1671-1671: MigrateState input rename handled.Switch to
_inputmatches the instruction struct; prevents serialization mismatch. LGTM. (Based on learnings)
2083-2094: Ignored mainnet-style init test added.Appropriate gating; no concerns. (Based on learnings)
2099-2115: Mainnet state-tree init fail (UnsupportedParameters).Expectation looks right for test-default params; OK.
2116-2133: Mainnet address-tree init fail (UnsupportedParameters).Consistent with parameter gating; OK.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
program-tests/utils/src/e2e_test_env.rs (1)
1577-1627: Fix: Incorrect fee params for v2 compressed accounts.The function hardcodes
v1_input_compressed_accountsandv2_input_compressed_accountswithout checking the actual tree type of the input accounts (lines 1605-1606). Whileget_random_compressed_sol_accountsfilters accounts to ensure they're all the same version, it doesn't communicate which version to the caller. If the returned accounts are from StateV2 trees, settingv1_input_compressed_accounts: input_compressed_accounts.len()andv2_input_compressed_accounts: falseis incorrect—it should bev1_input_compressed_accounts: 0andv2_input_compressed_accounts: true.This will cause fee assertion failures when
fee_assertis enabled and v2 accounts are randomly selected.Apply a similar pattern to
transfer_sol_deterministic(lines 1546-1550):let output_merkle_trees = self.get_merkle_tree_pubkeys(num_output_merkle_trees); let transaction_parameters = if self.keypair_action_config.fee_assert { + let first_tree_type = if !input_compressed_accounts.is_empty() { + self.indexer + .get_state_merkle_trees() + .iter() + .find(|x| { + x.accounts.merkle_tree.to_bytes() + == input_compressed_accounts[0].merkle_context.merkle_tree_pubkey.to_bytes() + }) + .unwrap() + .tree_type + } else { + TreeType::StateV1 + }; + let (inputs, is_v2) = if first_tree_type == TreeType::StateV2 { + (0u8, true) + } else { + (input_compressed_accounts.len() as u8, false) + }; Some(TransactionParams { num_new_addresses: 0, - v1_input_compressed_accounts: input_compressed_accounts.len() as u8, - v2_input_compressed_accounts: false, + v1_input_compressed_accounts: inputs, + v2_input_compressed_accounts: is_v2, num_output_compressed_accounts: num_output_merkle_trees as u8, compress: 0, fee_config: FeeConfig::default(), })Note: This same issue affects multiple functions in this file (see subsequent comments).
♻️ Duplicate comments (14)
program-tests/utils/src/e2e_test_env.rs (10)
1629-1671: Fix: Same tree type issue indecompress_sol.Lines 1647-1648 hardcode v1 fee parameters without checking the tree type of
input_compressed_accounts, identical to the issue intransfer_sol. This will cause incorrect fee assertions when v2 accounts are used.Apply the same fix pattern shown in the
transfer_solcomment to determine the correct tree type before constructingTransactionParams.
1739-1785: Fix: Same tree type issue incompress_sol.Lines 1756-1757 hardcode v1 fee parameters without checking the tree type of
input_compressed_accounts, identical to the issue intransfer_sol. This will cause incorrect fee assertions when v2 accounts are used.Apply the same fix pattern shown in the
transfer_solcomment.
1873-1951: Fix: Same tree type issue intransfer_spl.Lines 1925-1926 hardcode v1 fee parameters without checking the tree type of the token accounts. Token accounts include
tree_info.tree, which can be looked up inself.indexer.get_state_merkle_trees()to determine the tree type, similar to the SOL transfer functions.Check the tree type of the first token account's merkle tree before constructing
TransactionParams.
1953-2008: Fix: Same tree type issue inapprove_spl.Lines 1985-1986 hardcode v1 fee parameters without checking tree type.
Apply the tree type checking pattern from
transfer_sol_deterministic.
2010-2058: Fix: Same tree type issue inrevoke_spl.Lines 2039-2040 hardcode v1 fee parameters without checking tree type.
Apply the tree type checking pattern from
transfer_sol_deterministic.
2060-2113: Fix: Same tree type issue inburn_spl.Lines 2089-2090 hardcode v1 fee parameters without checking tree type.
Apply the tree type checking pattern from
transfer_sol_deterministic.
2115-2159: Fix: Same tree type issue infreeze_spl.Lines 2140-2141 hardcode v1 fee parameters without checking tree type.
Apply the tree type checking pattern from
transfer_sol_deterministic.
2161-2199: Fix: Same tree type issue inthaw_spl.Lines 2179-2180 hardcode v1 fee parameters without checking tree type.
Apply the tree type checking pattern from
transfer_sol_deterministic.
2201-2264: Fix: Same tree type issue incompress_spl.Lines 2239-2240 hardcode v1 fee parameters without checking tree type.
Apply the tree type checking pattern from
transfer_sol_deterministic.
2266-2343: Fix: Same tree type issue indecompress_spl.Lines 2318-2319 hardcode v1 fee parameters without checking tree type.
Apply the tree type checking pattern from
transfer_sol_deterministic.programs/system/src/context.rs (4)
83-89: V2 fee gating is still global; violates “once per tree” semantics and risks duplicates.This repeats earlier feedback: with multiple V2 trees in one instruction, a single boolean undercharges; pushing blindly risks duplicate entries and lacks overflow safeguards. Please switch to per-index idempotency and call the additive helper; update signature to Result for consistency.
#!/bin/bash # Find all call sites to assess multiplicity per instruction and ensure '?' is used. rg -nP --type=rust -C2 '\bset_network_fee_v2\s*\(' programs/system/src/
150-159: Use checked_add in set_rollover_fee to prevent overflow.Replace += with checked_add and map to a proper error, mirroring set_additive_fee.
- Some(payment) => payment.1 += fee, + Some(payment) => { + payment.1 = payment + .1 + .checked_add(fee) + .ok_or(ProgramError::ArithmeticOverflow)? + },
161-176: Replace magic numbers with named constants and align docs/tests.5_000 and 10_000 should come from constants (e.g., NETWORK_FEE_PER_V1_INPUT, NETWORK_FEE_PER_ADDRESS) to keep code/docs/tests in sync.
177-182: Bounds-check indices before indexing accounts in transfer_fees.Potential OOB panic on malformed indexes; return a typed error instead.
- for (i, fee) in self.rollover_fee_payments.iter() { - transfer_lamports_invoke(fee_payer, &accounts[*i as usize], *fee)?; - } + for (i, fee) in self.rollover_fee_payments.iter() { + let idx = *i as usize; + if idx >= accounts.len() { + return Err(SystemProgramError::AccountIndexOutOfBounds.into()); + } + transfer_lamports_invoke(fee_payer, &accounts[idx], *fee)?; + }
🧹 Nitpick comments (7)
sdk-libs/client/src/fee.rs (2)
47-55: Consider validating v1/v2 mutual exclusivity.The fee structure assumes
v1_input_compressed_accountsandv2_input_compressed_accountsare mutually exclusive (either count v1 inputs or flag v2 usage). However, there's no validation enforcing this invariant. If both are set (e.g.,v1_input_compressed_accounts = 5andv2_input_compressed_accounts = true), the fee calculation at lines 73-78 would only charge the v1 fee due to the if-else logic, potentially leading to incorrect fee assertions.Consider adding validation in a constructor or making the relationship explicit with an enum:
+impl TransactionParams { + pub fn new( + input_type: InputType, + num_output_compressed_accounts: u8, + num_new_addresses: u8, + compress: i64, + fee_config: FeeConfig, + ) -> Self { + let (v1_input_compressed_accounts, v2_input_compressed_accounts) = match input_type { + InputType::V1(count) => (count, false), + InputType::V2 => (0, true), + InputType::None => (0, false), + }; + Self { + v1_input_compressed_accounts, + v2_input_compressed_accounts, + num_output_compressed_accounts, + num_new_addresses, + compress, + fee_config, + } + } +} + +pub enum InputType { + V1(u8), + V2, + None, +}Alternatively, document the mutual exclusivity assumption clearly in the struct's doc comments.
69-83: Update comment to reflect v2 flat fee structure.The comment at line 69 states "Network fee is charged per input and per address," but this is only accurate for v1 inputs. For v2 inputs, a single flat
network_feeis charged regardless of the number of inputs. The per-address fee remains accurate.Update the comment to clarify the versioned fee structure:
- // Network fee is charged per input and per address + // Network fee: v1 charges per input, v2 charges a flat fee; address fee is always per-address let mut network_fee: i64 = 0;js/stateless.js/src/constants.ts (1)
359-366: Versioned address-network fee: confirm semantics and add a version-aware helper/alias.
- The JSDoc text for V1 and V2 is identical (“per address”). The PR summary hints V1 historically differed. Please confirm intended V1 semantics and align comments accordingly.
- To reduce churn in callers and avoid future regressions, expose a version-aware export (and optional deprecated alias) so tests and apps don’t need to branch everywhere.
Example additions (placed after these constants):
+/** + * Returns the address network fee per created address for the active protocol version. + */ +export const addressNetworkFeePerAddress = () => + featureFlags.isV2() ? ADDRESS_TREE_NETWORK_FEE_V2 : ADDRESS_TREE_NETWORK_FEE_V1; + +/** + * @deprecated Use ADDRESS_TREE_NETWORK_FEE_V1/ADDRESS_TREE_NETWORK_FEE_V2 + * or addressNetworkFeePerAddress(). Will be removed after the next release. + */ +export const ADDRESS_TREE_NETWORK_FEE = + addressNetworkFeePerAddress();If V1 is truly per-tx (not per-address), reflect that in both the JSDoc and helper naming (e.g., addressNetworkFeeForTx vs …PerAddress).
js/stateless.js/tests/e2e/compress.test.ts (2)
49-56: V2 input-fee branching matches “per-tx” charge; add parity check against program logic.
Logic LGTM. Please confirm that in V2 a tx with no inputs but with outputs still pays STATE_MERKLE_TREE_NETWORK_FEE, matching on-chain behavior. Consider adding a focused test for the V2 path.
57-67: Address network fee is hard-coded to V1; branch on feature flag now.
Implement the version-aware branch (or assert V1 in this suite) to prevent silent drift when tests run under V2.- /// Network fee charged per address created - const networkAddressFee = tx.addr - ? ADDRESS_TREE_NETWORK_FEE_V1.mul(bn(tx.addr)) - : bn(0); - // TODO: adapt once we use v2 address trees in tests. - // tx.addr - // ? featureFlags.isV2() - // ? ADDRESS_TREE_NETWORK_FEE_V2.mul(bn(tx.addr)) - // : ADDRESS_TREE_NETWORK_FEE_V1.mul(bn(tx.addr)) - // : bn(0); + /// Network fee charged per address created + const networkAddressFee = tx.addr + ? (featureFlags.isV2() + ? ADDRESS_TREE_NETWORK_FEE_V2.mul(bn(tx.addr)) + : ADDRESS_TREE_NETWORK_FEE_V1.mul(bn(tx.addr))) + : bn(0);programs/system/src/context.rs (1)
41-44: Add network_fee to MerkleTreeContext — OK, but document units/source.Field addition is fine. Please document units (lamports) and whether it mirrors registry’s address_network_fee to avoid drift.
programs/system/src/processor/create_address_cpi_data.rs (1)
43-50: Legacy network_fee fetch + fee enqueue — LGTM (minor naming nit).Logic is correct. Consider renaming network_fee to address_network_fee_legacy for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
js/stateless.js/src/constants.ts(1 hunks)js/stateless.js/tests/e2e/compress.test.ts(3 hunks)program-tests/utils/src/e2e_test_env.rs(14 hunks)programs/system/src/context.rs(4 hunks)programs/system/src/processor/create_address_cpi_data.rs(2 hunks)sdk-libs/client/src/fee.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/error.rs : Maintain stable mapping of AccountError to ProgramError, including Pinocchio code mapping (1–11), in error.rs
Applied to files:
programs/system/src/context.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs} : Gate SDK-specific implementations with #[cfg(feature = "solana"|"pinocchio"|"test-only")]
Applied to files:
programs/system/src/context.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/!(account_info)/**/*.rs : Use AccountInfoTrait for runtime-agnostic account handling; avoid direct solana-program or pinocchio AccountInfo in general logic
Applied to files:
programs/system/src/context.rs
📚 Learning: 2025-10-11T21:59:25.222Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:25.222Z
Learning: Applies to program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs} : Provide SDK-specific AccountInfoTrait implementations in account_info/{solana.rs,pinocchio.rs,test_account_info.rs}
Applied to files:
programs/system/src/context.rs
🧬 Code graph analysis (2)
programs/system/src/processor/create_address_cpi_data.rs (2)
program-libs/compressed-account/src/address.rs (1)
derive_address_legacy(5-12)programs/system/src/errors.rs (2)
from(160-236)from(240-242)
js/stateless.js/tests/e2e/compress.test.ts (2)
js/stateless.js/src/constants.ts (3)
featureFlags(16-35)STATE_MERKLE_TREE_NETWORK_FEE(356-356)ADDRESS_TREE_NETWORK_FEE_V1(361-361)js/stateless.js/src/state/bn.ts (1)
bn(3-12)
⏰ 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). (21)
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: stateless-js-v2
- GitHub Check: lint
- GitHub Check: cli-v2
- GitHub Check: cli-v1
- GitHub Check: system-programs (token test, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: stateless-js-v1
🔇 Additional comments (12)
sdk-libs/client/src/fee.rs (2)
35-44: LGTM: Batched config increases address network fee.The
test_batched()configuration appropriately sets a higheraddress_network_fee(10,000 vs default 5,000) for batched operations.
112-115: LGTM: Enhanced logging for fee debugging.The additional logging of the complete
fee_configstructure will help diagnose fee calculation mismatches during testing.program-tests/utils/src/e2e_test_env.rs (3)
1546-1561: LGTM: Correct tree type handling in deterministic transfer.The function correctly checks
bundle.tree_typeand setsv1_input_compressed_accountsandv2_input_compressed_accountsaccordingly. StateV2 trees use a flat fee (v2=true, inputs=0), while v1 trees charge per input.
1704-1720: LGTM: Correct tree type handling in deterministic compress.Like
transfer_sol_deterministic, this function correctly checksbundle.tree_typeto set the appropriate v1/v2 fee parameters.
1787-1871: LGTM: Address creation correctly sets no inputs.The
create_addressfunction correctly setsv1_input_compressed_accounts: 0andv2_input_compressed_accounts: falsesince address creation transactions don't consume input compressed accounts.js/stateless.js/tests/e2e/compress.test.ts (2)
247-247: Readability win with expectedTxFees variable.
Nice improvement; keeps assertions cleaner and consistent.Also applies to: 251-251
7-10: Verification complete: no unversioned uses found.The search confirms all uses of
ADDRESS_TREE_NETWORK_FEEare properly versioned:
ADDRESS_TREE_NETWORK_FEE_V1at line 59 and exported at constants.ts:361ADDRESS_TREE_NETWORK_FEE_V2in comments at lines 64–65 and exported at constants.ts:366- Imports in compress.test.ts are correct
No unversioned
ADDRESS_TREE_NETWORK_FEEexists anywhere in the codebase.programs/system/src/context.rs (4)
12-15: Import change looks good.Brings ProgramError into scope for overflow handling; consistent with Result alias usage.
59-61: Good: set_address_fee now returns Result and reuses the helper.Error propagation and dedup via helper are consistent with other setters.
64-66: Good: set_network_fee_v1 unified through additive helper.Consistent overflow handling and error propagation.
68-81: Helper implementation is solid.Uses checked_add and returns ProgramError::ArithmeticOverflow; matches crate::Result conventions.
programs/system/src/processor/create_address_cpi_data.rs (1)
75-79: Confirm target account for V2 address fee.Here you enqueue the address fee against the V2 tree index (not a queue). If that’s intentional for batched trees, add a short comment; if not, align with V1 behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor