Conversation
ananas-block
requested review from
SwenSchaeferjohann and
sergeytimoshin
as code owners
|
Caution Review failedThe pull request is closed. WalkthroughChanges add merkle-tree feature support to the SDK with new constants, public exports, and module additions. Also includes minor fixes to test configurations, path formatting in setup utilities, and removal of duplicate build script exclusion flags. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Mixed complexity across configuration changes, API exports, new module logic with validation, and test updates. Most changes are straightforward additions and fixes, though the merkle tree module introduces validation logic requiring verification. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
ananas-block
force-pushed
the
jorrit/fix-sdks-v0.15.0
branch
from
14fd6c1 to
79880dd
Compare
ananas-block
force-pushed
the
jorrit/fix-sdks-v0.15.0
branch
from
79880dd to
a97e9f1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sdk-libs/sdk/src/merkle_tree.rs (1)
22-29: Consider extracting the magic number 2400 to a named constant.The hardcoded value
2400appears multiple times and represents the maximum root history length. Consider defining it as a constant likeSTATE_MERKLE_TREE_MAX_ROOTS: usize = 2400for better maintainability.Apply this diff to extract the constant:
pub const STATE_MERKLE_TREE_DISCRIMINATOR: [u8; 8] = [172, 43, 172, 186, 29, 73, 219, 84]; pub const STATE_MERKLE_TREE_ACCOUNT_METADATA_LEN: usize = 224; + pub const STATE_MERKLE_TREE_MAX_ROOTS: usize = 2400; /// Reads a root from the concurrent state merkle tree by index pub fn read_state_merkle_tree_root( account_info: &AccountInfo, root_index: u16, ) -> Result<[u8; 32], ProgramError> { - if root_index as usize >= 2400 { + if root_index as usize >= STATE_MERKLE_TREE_MAX_ROOTS { msg!( "Invalid root index: {} greater than max root index {}", root_index, - 2400 + STATE_MERKLE_TREE_MAX_ROOTS );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
scripts/format.sh(0 hunks)sdk-libs/program-test/src/indexer/test_indexer.rs(1 hunks)sdk-libs/program-test/src/utils/setup_light_programs.rs(1 hunks)sdk-libs/sdk-types/src/constants.rs(1 hunks)sdk-libs/sdk/Cargo.toml(2 hunks)sdk-libs/sdk/src/instruction/mod.rs(1 hunks)sdk-libs/sdk/src/lib.rs(1 hunks)sdk-libs/sdk/src/merkle_tree.rs(1 hunks)
💤 Files with no reviewable changes (1)
- scripts/format.sh
🧰 Additional context used
🧠 Learnings (9)
📚 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:
sdk-libs/sdk/src/merkle_tree.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:
sdk-libs/sdk/src/merkle_tree.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:
sdk-libs/sdk/src/merkle_tree.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/checks.rs : Expose and maintain account validation helpers (check_owner, check_program, check_mut/non_mut, check_signer, check_discriminator, set_discriminator, check_pda_seeds, check_account_balance_is_rent_exempt, account_info_init) in checks.rs
Applied to files:
sdk-libs/sdk/src/merkle_tree.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/**/*.rs : On account initialization, call account_info_init to set the 8-byte discriminator
Applied to files:
sdk-libs/sdk/src/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 : 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/sdk/src/merkle_tree.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/**/*.rs : Validate account type with 8-byte discriminators using check_discriminator before deserialization
Applied to files:
sdk-libs/sdk/src/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/account_compression_cpi/mod.rs : Export each new operation module by adding pub mod <operation>; and re-export with pub use <operation>::*.
Applied to files:
sdk-libs/sdk/src/instruction/mod.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:
sdk-libs/sdk/src/lib.rs
🧬 Code graph analysis (2)
sdk-libs/sdk/src/merkle_tree.rs (1)
program-libs/account-checks/src/checks.rs (1)
check_owner(135-143)
sdk-libs/sdk/src/instruction/mod.rs (1)
js/stateless.js/src/state/types.ts (2)
CompressedProof(373-386)ValidityProof(397-410)
⏰ 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). (9)
- GitHub Check: lint
- 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: stateless-js-v1
- GitHub Check: cli-v2
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v2
🔇 Additional comments (7)
sdk-libs/program-test/src/utils/setup_light_programs.rs (1)
72-72: Critical bug fix: Directory separator added.This correctly adds the missing
/separator in the path construction, making it consistent with all other program paths in this function (lines 54, 60, 66, 79). Without this fix, the path would be malformed (e.g.,/path/to/binspl_noop.so), causing the NOOP program to fail loading at line 74.sdk-libs/sdk-types/src/constants.rs (1)
42-42: LGTM!The new
ADDRESS_TREE_V2constant follows the established naming and formatting conventions.sdk-libs/sdk/src/instruction/mod.rs (1)
52-54: LGTM!The grouped re-export of both
CompressedProofandValidityProofimproves the public API surface and maintains consistency with the module's purpose.sdk-libs/program-test/src/indexer/test_indexer.rs (1)
148-148: Verify that tests pass with the updated proof retrieval flag.The change from
falsetotrueinget_proof_of_leafmodifies the proof generation behavior. Please confirm that all related tests pass and the change aligns with the intended proof format for this use case.sdk-libs/sdk/src/lib.rs (1)
155-156: LGTM!The feature-gated
merkle_treemodule is correctly declared and appropriately placed within the module hierarchy.sdk-libs/sdk/Cargo.toml (2)
25-25: LGTM!The
merkle-treefeature is correctly defined and enables the appropriate dependency sub-feature.
49-49: LGTM!The optional dependency declaration follows the established pattern and correctly uses workspace versioning.
| let data = &account_data[required_size..]; | ||
| let merkle_tree = ConcurrentMerkleTreeZeroCopy::<Poseidon, 26>::from_bytes_zero_copy(data)?; | ||
|
|
||
| Ok(merkle_tree.roots[root_index as usize]) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify bounds safety when accessing merkle_tree.roots.
The function validates root_index < 2400, then accesses merkle_tree.roots[root_index as usize]. Please confirm that ConcurrentMerkleTreeZeroCopy<Poseidon, 26> guarantees a roots array of at least 2400 elements to prevent potential out-of-bounds access.
🏁 Script executed:
#!/bin/bash
# Verify the ConcurrentMerkleTreeZeroCopy type definition and roots array size
rg -A 10 "struct ConcurrentMerkleTreeZeroCopy" --type rust
rg "roots.*\[" --type rust -A 3 | rg "2400|ROOTS"Length of output: 1813
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for ConcurrentMerkleTree struct definition (the inner type)
rg -A 20 "struct ConcurrentMerkleTree[^Z]" --type rustLength of output: 3300
🏁 Script executed:
#!/bin/bash
# Search for CyclicBoundedVec definition and its size/capacity
rg -A 15 "struct CyclicBoundedVec" --type rustLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for CyclicBoundedVec definition or type alias
rg "type CyclicBoundedVec|CyclicBoundedVec.*=" --type rust -B 2 -A 5
# Also check the concurrent-merkle-tree lib.rs for imports
rg "use.*CyclicBoundedVec|mod.*" program-libs/concurrent-merkle-tree/src/lib.rs -A 2Length of output: 7239
🏁 Script executed:
#!/bin/bash
# Search for CyclicBoundedVec implementation in offset module
rg "pub.*CyclicBoundedVec|impl.*CyclicBoundedVec" --type rust -A 5
# Also search for how roots capacity is set
rg "roots.*capacity|capacity.*2400|ROOTS_LEN" --type rustLength of output: 4860
🏁 Script executed:
#!/bin/bash
# Find ROOTS constant
rg "const ROOTS|ROOTS\s*=" --type rust -B 2 -A 1
# Look at from_bytes_zero_copy implementation
rg -A 30 "pub fn from_bytes_zero_copy\(" program-libs/concurrent-merkle-tree/src/zero_copy.rs | head -50
# Check the actual merkle_tree.rs file around lines 50-53
cat sdk-libs/sdk/src/merkle_tree.rs | head -70 | tail -30Length of output: 19114
🏁 Script executed:
#!/bin/bash
# Find check_size_constraints implementation
rg -A 20 "fn check_size_constraints" --type rust
# Also search for any validation of roots capacity
rg "roots.*capacity|roots_capacity.*=" --type rust -B 2 -A 2 | grep -A 5 "check_size_constraints\|validate\|assert"Length of output: 7256
Bounds safety issue confirmed: roots array capacity is not validated.
The concern is valid. The code validates root_index < 2400 but does not guarantee that merkle_tree.roots has capacity >= 2400. The check_size_constraints() only validates that roots_size > 0, leaving a potential out-of-bounds access.
Recommended fix: Add validation in check_size_constraints_new() to ensure roots_size >= 2400 for this use case, or verify at the call site that the deserialized roots capacity meets the requirement before accessing the array.
🤖 Prompt for AI Agents
In sdk-libs/sdk/src/merkle_tree.rs around lines 50 to 53, the code uses
merkle_tree.roots[root_index as usize] after only validating root_index < 2400
but not ensuring merkle_tree.roots has at least that many entries; update the
validation to prevent out-of-bounds access by either (a) adding a size check in
check_size_constraints_new() to require roots_size >= 2400 (and return a clear
error if not), or (b) adding a runtime guard at this call site that verifies
merkle_tree.roots.len() > root_index as usize before indexing and returns an
error if the deserialized roots capacity is insufficient.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores