refactor: libs to nostd #1997
refactor: libs to nostd #1997
Conversation
- Add no_std attribute with std/alloc feature flags - Replace std::result types with core::result equivalents - Add conditional Vec imports with #[cfg(feature = "alloc")] - Add keccak feature for optional v1 address derivation - Gate solana_msg::msg! logging with #[cfg(feature = "std")] - Remove unused light-zero-copy dependency - Maintain backward compatibility with default = ["std"] - All tests passing, dependent crates build successfully Three supported modes: - no_std without alloc: Pure no_std (Vec unavailable) - no_std with alloc: For BPF programs with allocator - std mode (default): Standard library with full compatibility Part of ongoing no_std refactoring for Solana program compatibility.
ananas-block
requested review from
SwenSchaeferjohann and
sergeytimoshin
as code owners
WalkthroughAdds a new workspace crate Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OldIndexer as light_compressed_account::indexer_event
participant EventCrate as light_event
Note over Caller,OldIndexer: Before
Caller->>OldIndexer: import types / call parse
OldIndexer-->>Caller: parsed event / types
Note over Caller,EventCrate: After
Caller->>EventCrate: import types / call parse
EventCrate-->>Caller: parsed event / types
sequenceDiagram
participant Serializer
participant Anchor as anchor_lang::AnchorSerialize
participant Borsh as borsh::BorshSerialize
Note left of Serializer: Serialization chosen at compile-time
Serializer->>Anchor: if feature(std) && feature(anchor)
Anchor-->>Serializer: Anchor serialization used
Serializer->>Borsh: else
Borsh-->>Serializer: Borsh serialization used
Note right of Serializer: Controlled via #[cfg_attr(...)]
sequenceDiagram
participant HasherCaller
participant KeccakLocal
participant KeccakSolana
participant ErrorPath
HasherCaller->>KeccakLocal: if !target_os=solana && feature(keccak)
KeccakLocal-->>HasherCaller: compute via sha3 crate
HasherCaller->>ErrorPath: if !target_os=solana && !feature(keccak)
ErrorPath-->>HasherCaller: return HasherError::KeccakFeatureNotEnabled
HasherCaller->>KeccakSolana: if target_os=solana
KeccakSolana-->>HasherCaller: perform solana host keccak call
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
ananas-block
force-pushed
the
jorrit/refactor-compressed-account-nostd
branch
from
8dda9ef to
cd07341
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: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
sdk-libs/sdk-types/src/instruction/account_info.rs (1)
76-80: Confirm the API asymmetry and allocation issue.The original review comment is correct. Both
input_compressed_accountandoutput_compressed_accountperform heap allocations, but onlyinput_compressed_accountis feature-gated with#[cfg(feature = "alloc")].Evidence:
- Line 195–197 (input_compressed_account): Creates
Vec::new()- Line 214 (output_compressed_account): Calls
output.data.clone()Both require allocator support. When the
allocfeature is disabled:
input_compressed_accountbecomes unavailable (compiler-enforced)output_compressed_accountremains callable but still allocates, breaking no-alloc guaranteesRecommendation: Add
#[cfg(feature = "alloc")]to the trait declaration and implementation ofoutput_compressed_account(lines 81 and 206) to maintain API consistency and enforce no-alloc correctness.program-libs/compressed-account/Cargo.toml (1)
27-36: Fix unconditional std dependencies to support no_std buildslight-zero-copy has std as a default feature and requires
default-features = falseto disable it; borsh similarly has std as a default feature and needs the same treatment. The crate is designed for no_std (lib.rs has#![cfg_attr(not(feature = "std"), no_std)]) but currently breaks when compiled without the std feature because light-zero-copy/std is forced on unconditionally.-light-zero-copy = { workspace = true, features = ["std", "mut", "derive"] } +light-zero-copy = { workspace = true, default-features = false, features = ["mut", "derive"] } -borsh = { workspace = true } +borsh = { workspace = true, default-features = false }And in [features]:
-std = ["alloc", "borsh/std", "dep:solana-msg"] +std = ["alloc", "borsh/std", "dep:solana-msg", "light-zero-copy/std"]sdk-libs/client/src/rpc/client.rs (1)
301-305: Do not unwrap parser result (panic risk on RPC path)event_from_light_transaction(...).unwrap() will panic on parse errors. Propagate as RpcError instead.
Apply:
- let parsed_event = - event_from_light_transaction(program_ids.as_slice(), vec.as_slice(), vec_accounts) - .unwrap(); + let parsed_event = event_from_light_transaction( + program_ids.as_slice(), + vec.as_slice(), + vec_accounts, + ) + .map_err(|e| RpcError::CustomError(format!("Failed to parse event: {e:?}")))?;program-tests/create-address-test-program/src/lib.rs (2)
103-124: Bounds-checkremaining_accountsbefore indexed access.
remaining_accounts[1]and[2]will panic if fewer than 3 accounts are supplied whenwrite_cpi_contextis true.- } else { + } else { + require!( + ctx.remaining_accounts.len() >= 3, + ErrorCode::AccountNotEnoughKeys + ); let mut account_metas = vec![]; account_metas.push(AccountMeta { pubkey: *cpi_accounts.fee_payer().key, is_signer: true, is_writable: true, });
281-288: Capacity arithmetic can underflow; use saturating math.
Vec::with_capacity(1 + len - PROGRAM_ACCOUNTS_LEN)underflows iflen < PROGRAM_ACCOUNTS_LEN, yielding a huge capacity.- let mut account_metas = - Vec::with_capacity(1 + cpi_accounts.account_infos().len() - PROGRAM_ACCOUNTS_LEN); + let extra = cpi_accounts + .account_infos() + .len() + .saturating_sub(PROGRAM_ACCOUNTS_LEN); + let mut account_metas = Vec::with_capacity(1 + extra);program-libs/hasher/src/hash_to_field_size.rs (1)
20-35: Avoid unwrap; use the existing Result to propagate errors consistentlyKeccak::hashv already returns a Result in this codebase (see its use with
?below). Replaceunwrap()with?to avoid panics and align error handling across functions.Apply this diff:
- // SAFETY: cannot panic Hasher::hashv returns an error because Poseidon can panic. - let mut hashed_value = Keccak::hashv(slices.as_slice()).unwrap(); + // Keccak::hashv is fallible (trait-unified), propagate errors instead of panicking. + let mut hashed_value = Keccak::hashv(slices.as_slice())?;program-libs/compressed-account/src/instruction_data/with_readonly.rs (1)
525-542: Strengthen bounds to prevent OOM/DoS on untrusted input
if bytes.len() < num_slices { ... }is too weak. Each element consumes many bytes; a largenum_slicescan trigger huge allocations (Vec::with_capacity(num_slices)) and long loops.Add:
- A hard cap (e.g., MAX_INPUT_ACCOUNTS).
- A size-based check using
size_of::<ZInAccountMeta>() + 1(for the Option tag) with checked multiplication.Proposed patch:
- let (num_slices, mut bytes) = Ref::<&[u8], U32>::from_prefix(bytes)?; - let num_slices = u32::from(*num_slices) as usize; - // Prevent heap exhaustion attacks by checking if num_slices is reasonable - // Each element needs at least 1 byte when serialized - if bytes.len() < num_slices { + let (num_slices, mut bytes) = Ref::<&[u8], U32>::from_prefix(bytes)?; + let num_slices = u32::from(*num_slices) as usize; + const MAX_INPUT_ACCOUNTS: usize = 10_000; // tune as appropriate + if num_slices > MAX_INPUT_ACCOUNTS { + return Err(ZeroCopyError::InsufficientMemoryAllocated(num_slices, MAX_INPUT_ACCOUNTS)); + } + // Approximate lower bound per entry: meta + 1 byte Option tag for address + let min_per = core::mem::size_of::<ZInAccountMeta>() + 1; + let needed = num_slices.checked_mul(min_per).ok_or_else(|| { + ZeroCopyError::InsufficientMemoryAllocated(usize::MAX, num_slices) + })?; + if bytes.len() < needed { return Err(ZeroCopyError::InsufficientMemoryAllocated( - bytes.len(), - num_slices, + bytes.len(), + needed, )); } let mut slices = Vec::with_capacity(num_slices);Based on learnings.
program-libs/compressed-account/src/address.rs (1)
7-17: Remove heap allocation in legacy address derivation for no_std compatibility
concat()allocates. Use the infallible const-array helper you already import; this also aligns with the new no_std direction.pub fn derive_address_legacy( merkle_tree_pubkey: &Pubkey, seed: &[u8; 32], ) -> Result<[u8; 32], CompressedAccountError> { - let hash = hash_to_bn254_field_size_be( - [merkle_tree_pubkey.as_ref(), seed.as_ref()] - .concat() - .as_slice(), - ); + let slices = [merkle_tree_pubkey.as_ref(), seed.as_ref()]; + let hash = hashv_to_bn254_field_size_be_const_array::<3>(&slices) + .expect("hashv_to_bn254_field_size_be_const_array::<3> should be infallible for Keccak"); Ok(hash) }program-libs/compressed-account/src/instruction_data/invoke_cpi.rs (1)
36-49: Fix compile error and encoding divergence in LightInstructionData::data()The
#[cfg(feature = "alloc")]guard allows this method to compile only when the struct derivesBorshSerialize, but whenall(feature = "std", feature = "anchor")is true, the struct derivesAnchorSerializeinstead—causing a compile error. Additionally, the 4-byte length prefix (line 45) diverges from the trait default, which encodes only discriminator || payload without a length byte.Fix:
impl LightInstructionData for InstructionDataInvokeCpi { - #[cfg(feature = "alloc")] + #[cfg(all(feature = "alloc", not(feature = "anchor")))] fn data(&self) -> Result<Vec<u8>, crate::CompressedAccountError> { use borsh::BorshSerialize; let inputs = self .try_to_vec() .map_err(|_| crate::CompressedAccountError::InvalidArgument)?; - let mut data = Vec::with_capacity(12 + inputs.len()); + let mut data = Vec::with_capacity(8 + inputs.len()); data.extend_from_slice(self.discriminator()); - data.extend_from_slice(&(inputs.len() as u32).to_le_bytes()); data.extend_from_slice(inputs.as_slice()); Ok(data) } }When
feature = "anchor"is enabled, the struct derivesAnchorSerializeand the trait default implementation (which usesAnchorSerialize::try_to_vec()) applies.program-libs/compressed-account/src/lib.rs (2)
41-44: Fix typos in user-facing error messages“thresold” → “threshold”; “lenght” → “length”.
- #[error("Invalid rollover thresold")] + #[error("Invalid rollover threshold")] InvalidRolloverThreshold, - #[error("Invalid input lenght")] + #[error("Invalid input length")] InvalidInputLength,
149-160: Panicking From conversions accept untrusted input without validationTreeType::from calls receive account data bytes without length checks or value validation:
remaining_account_checks.rs:90extracts bytes [8..16] without verifying data length ≥ 16context.rs:139extracts bytes [8..16] without verifying data length ≥ 16QueueType::from at
queue.rs:515receives a u64 parameter without pre-validation before conversion.All three conversion sites will panic on invalid values. Use TryFrom, validate inputs before calling From, or document safety invariants if values are guaranteed valid.
sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs (1)
128-133: BorshSerialize impl uses std::io::Write — breaks no_stdUse borsh::io::Write to be portable (borsh defines its own Writer in no_std). Also align the Result type.
-impl BorshSerialize for LightSystemProgramCpi { - fn serialize<W: std::io::Write>(&self, writer: &mut W) -> std::io::Result<()> { - self.instruction_data.serialize(writer) - } -} +impl BorshSerialize for LightSystemProgramCpi { + fn serialize<W: borsh::io::Write>(&self, writer: &mut W) -> borsh::io::Result<()> { + self.instruction_data.serialize(writer) + } +}
🧹 Nitpick comments (35)
sdk-libs/macros/src/program.rs (1)
15-15: Consider restoring the Debug derive.The removal of
DebugfromInstructionParamreduces debuggability of the macro without providing clear benefits. Since this is a procedural macro crate that runs at compile-time with fullstdsupport, there's no technical reason to omitDebug. HavingDebugon internal types significantly aids troubleshooting macro transformation issues during development.Apply this diff to restore the Debug derive:
-#[derive(Clone)] +#[derive(Clone, Debug)]program-libs/hasher/tests/data_hasher.rs (3)
2-2: Simplify Vec import.The explicit
use std::vec::Vec;can be simplified touse std::vec;or removed entirely sinceVecis available in the prelude for std environments.Apply this diff:
-use std::vec::Vec; +use std::vec;Or simply remove the import since Vec is in the prelude.
48-55: Simplify hash initialization.The initialization pattern creates the arrays, then immediately shadows them with mutable bindings. This can be simplified by declaring them as mutable directly.
Apply this diff:
- let hash1 = [0u8; 32]; - let hash2 = [0u8; 32]; - - let mut hash1 = hash1; + let mut hash1 = [0u8; 32]; hash1[31] = 1; - let mut hash2 = hash2; + let mut hash2 = [0u8; 32]; hash2[31] = 2;
84-134: Reduce code duplication across match arms.Each match arm contains nearly identical code: construct array → hash → build expected slices → verify. The only differences are the array size and clone count. This duplication makes the test harder to maintain.
Consider extracting the common verification logic:
+ let verify_hash = |arr: &[TestHashable]| { + let expected_slices: Vec<&[u8]> = + expected_inputs.iter().map(|h| h.as_slice()).collect(); + let expected_hash = Poseidon::hashv(&expected_slices).unwrap(); + expected_hash + }; + match size { 1 => { let arr: [TestHashable; 1] = [array_slice[0].clone()]; let hash_result = arr.hash::<Poseidon>().unwrap(); - - let expected_slices: Vec<&[u8]> = - expected_inputs.iter().map(|h| h.as_slice()).collect(); - let expected_hash = Poseidon::hashv(&expected_slices).unwrap(); - - assert_eq!(hash_result, expected_hash); + assert_eq!(hash_result, verify_hash(&arr)); } // Similar for cases 2-4...Alternatively, use a macro to generate the test cases or reconsider whether the match-based approach is necessary.
sdk-libs/sdk-types/src/cpi_accounts/config.rs (1)
30-32: Consider renaming method to avoid field name collision.The method
cpi_signer()returns a[u8; 32]by accessingself.cpi_signer.cpi_signer, wherecpi_signeris both the field name and the method name. This creates ambiguity and makes the code harder to read. Consider renaming the method tocpi_signer_pubkey()orget_cpi_signer()for clarity.- pub fn cpi_signer(&self) -> [u8; 32] { + pub fn cpi_signer_pubkey(&self) -> [u8; 32] { self.cpi_signer.cpi_signer }forester-utils/Cargo.toml (1)
21-21: Consider feature-gating std or documenting std requirement.The forester-utils crate unconditionally enables std for light-compressed-account, which limits potential reuse in no_std contexts. If this library is intentionally std-only (for forester binary support), consider documenting this requirement. Otherwise, gate the std feature behind a feature flag for flexibility.
If std is required, consider adding documentation:
[package] name = "forester-utils" # ... # Note: This crate requires std and is designed for use with the forester binaryOr add feature propagation:
[features] -default = ["v2"] +default = ["v2", "std"] +std = ["light-compressed-account/std"] devenv = ["v2", "light-client/devenv", "light-prover-client/devenv"]program-tests/system-cpi-test/src/sdk.rs (1)
46-69: Typo in field name: readonly_adressesConsider renaming readonly_adresses → read_only_addresses to avoid confusion; provide a deprecation alias if public.
Is this field part of any public/test API consumed downstream? If yes, I can propose a minimal deprecation shim.
program-libs/compressed-account/src/instruction_data/zero_copy.rs (1)
222-239: Guard against malformed length to avoid panicsplit_at(u64::from(*len) as usize) can panic on short buffers. Return ZeroCopyError instead.
- let (data, bytes) = bytes.split_at(u64::from(*len) as usize); + let data_len = u64::from(*len) as usize; + if bytes.len() < data_len { + return Err(ZeroCopyError::InvalidConversion); + } + let (data, bytes) = bytes.split_at(data_len);program-libs/hasher/tests/hash_chain.rs (2)
105-118: Redundant cfg gateFile is already gated with #![cfg(feature = "poseidon")]; the inner #[cfg(feature = "poseidon")] block is unnecessary.
- #[cfg(feature = "poseidon")] - { + { // ... }
7-16: Minor: test descriptions and labelsDuplicate “2. Functional” label and verbose comments; simplify to keep tests focused.
Also applies to: 40-58, 60-78
program-libs/hasher/Cargo.toml (2)
17-18: Guard hash features on Solana target.
sha256/keccakmap tosha2/sha3only on non‑Solana targets. Enabling them on BPF will silently drop those deps. Add a compile‑time guard (or an alternative backend) to prevent foot‑guns.In any common module (e.g., lib.rs), add:
#[cfg(all(target_os = "solana", feature = "sha256"))] compile_error!("feature 'sha256' is not supported on target_os=solana. Use the Solana syscall backend or disable this feature."); #[cfg(all(target_os = "solana", feature = "keccak"))] compile_error!("feature 'keccak' is not supported on target_os=solana. Use the Solana syscall backend or disable this feature.");Optionally, wire Solana keccak via a separate feature (e.g.,
solana-keccak) if you have a syscall-based implementation.Also applies to: 35-36
10-13: Consider changingdefault = []for better external library design.Internal workspace dependents are already protected by root-level
default-features = false, but external consumers pullinglight-hasherfrom crates.io without explicitdefault-features = falsewould inadvertently includestd. For a program library, defaulting to empty features is the recommended approach.program-libs/compressed-account/src/instruction_data/compressed_proof.rs (1)
89-94: PreferFrom<ValidityProof> for Option<CompressedProof>overIntoThis removes the clippy allow and is the idiomatic direction.
Apply this diff:
-#[allow(clippy::from_over_into)] -impl Into<Option<CompressedProof>> for ValidityProof { - fn into(self) -> Option<CompressedProof> { - self.0 - } -} +impl From<ValidityProof> for Option<CompressedProof> { + fn from(v: ValidityProof) -> Option<CompressedProof> { + v.0 + } +}Based on learnings.
sdk-libs/client/Cargo.toml (1)
46-46: Light-event feature flags: ensure std/alloc alignmentClient is a std crate. If light-event defaults to alloc/no_std, explicitly enable its
stdfeature (if provided) to avoid missing impls/serde paths.Check light-event’s Cargo.toml features and, if applicable, change to:
light-event = { workspace = true, features = ["std"] }If
stdis default there, this is fine as-is; please confirm.sdk-libs/sdk/src/lib.rs (1)
163-166: Macro re-exports OK; consider disambiguating LightDiscriminatorYou export both a trait alias (LightDiscriminator from light_account_checks) and a derive macro (LightDiscriminator from light_sdk_macros) under the same name at crate root. Legal (separate namespaces) but confusing in docs/imports. Consider renaming the trait re-export (e.g., LightDiscriminatorTrait) or nest traits under a module to reduce ambiguity.
sdk-libs/client/src/rpc/client.rs (2)
658-659: Guard against empty batch vectorIndexing e[0] can panic if the vector is empty. Prefer first().
- let event = parsed_event.map(|(e, signature, slot)| (e[0].event.clone(), signature, slot)); + let event = parsed_event.and_then(|(e, signature, slot)| { + e.first().map(|b| (b.event.clone(), signature, slot)) + });
295-296: Replace println! with tracingUse warn!/debug! for consistency with tracing.
- println!("Parsed instructions are not implemented yet"); + tracing::warn!("Parsed instructions are not implemented yet");sdk-libs/event/src/parse.rs (1)
519-537: Unchecked indexing may panic; add bounds checks
- new_addresses: accounts[x.tree_index as usize]
- create_nullifier_queue_indices: accounts[x.merkle_context.merkle_tree_pubkey_index as usize]
If indexes are malformed, this panics. Prefer get() with a clear ParseIndexerEventError.
- mt_pubkey: associated_instructions.accounts[x.tree_index as usize], + mt_pubkey: *associated_instructions + .accounts + .get(x.tree_index as usize) + .ok_or(ParseIndexerEventError::DeserializeSystemInstructionError)?,And similarly guard merkle_tree_pubkey_index lookups in create_nullifier_queue_indices().
Also applies to: 612-621
program-libs/account-checks/src/packed_accounts.rs (1)
16-16: Reconsider_nameprefix given conditional usage.The underscore prefix conventionally indicates an unused parameter, but
_nameis actually used within thecfg(feature = "std")logging block on Line 23. While the code compiles correctly, this may confuse maintainers who expect underscore-prefixed parameters to be truly unused.Consider one of these alternatives:
- Keep
namewithout the underscore and add#[allow(unused_variables)]when std is disabled- Use a more explicit name like
name_for_loggingto clarify its purposeApply this diff if choosing option 1:
- pub fn get(&self, index: usize, _name: &str) -> Result<&A, AccountError> { + #[cfg_attr(not(feature = "std"), allow(unused_variables))] + pub fn get(&self, index: usize, name: &str) -> Result<&A, AccountError> { if index >= self.accounts.len() { #[cfg(feature = "std")] { let location = Location::caller(); solana_msg::msg!( "ERROR: Not enough accounts. Requested '{}' at index {} but only {} accounts available. {}:{}:{}", - _name, index, self.accounts.len(), location.file(), location.line(), location.column() + name, index, self.accounts.len(), location.file(), location.line(), location.column() ); }sdk-libs/sdk-types/src/address.rs (1)
65-66: Avoidunwrap()in hashing paths (panic in no_std).Replace
unwrap()withexpect()carrying context, or return aResultupstream. This prevents aborts if the slice exceedsN.- hashv_to_bn254_field_size_be_const_array::<2>(input.as_slice()).unwrap() + hashv_to_bn254_field_size_be_const_array::<2>(input.as_slice()) + .expect("derive_address_from_seed: inputs must not exceed N")- AddressSeed(hashv_to_bn254_field_size_be_const_array::<17>(seeds).unwrap()) + AddressSeed( + hashv_to_bn254_field_size_be_const_array::<17>(seeds) + .expect("v2::derive_address_seed: at most 16 seeds + 1 bump"), + )Also applies to: 116-118
program-tests/create-address-test-program/src/lib.rs (1)
143-145: Minor: avoid constructing config just to pass bump.You can use the const directly to reduce work.
- let cpi_config = CpiAccountsConfig::new(crate::LIGHT_CPI_SIGNER); - invoke_light_system_program(&account_infos, instruction, cpi_config.bump())?; + invoke_light_system_program(&account_infos, instruction, crate::LIGHT_CPI_SIGNER.bump)?;program-libs/macros/src/cpi_signer.rs (1)
118-123: Macro output type relies on CpiSigner in scope; enforce with path or clarify docs.Current code emits
CpiSigner { .. }unqualified. Either emit a fully qualified path or make the requirement explicit in docs across all entry points.Consider:
- Document: “CpiSigner must be in scope (e.g.,
use light_sdk_types::CpiSigneroruse light_sdk::cpi::CpiSigner).”- Or emit
::light_sdk_types::CpiSigner { ... }if that’s the canonical type.sdk-libs/sdk-pinocchio/src/cpi/v2/invoke.rs (1)
27-43: Unify ProgramError aliasing and conversion for readability.Use a local alias and
From<LightSdkError> for ProgramErrorto keep conversions consistent.- fn with_light_account<A>( + fn with_light_account<A>( mut self, account: crate::LightAccount<'_, A>, ) -> Result<Self, pinocchio::program_error::ProgramError> @@ - let account_info = account - .to_account_info() - .map_err(|e| pinocchio::program_error::ProgramError::Custom(u64::from(e) as u32))?; + use pinocchio::program_error::ProgramError; + let account_info = account + .to_account_info() + .map_err(|e| ProgramError::from(crate::error::LightSdkError::from(e)))?;Also applies to: 112-116
program-libs/macros/src/lib.rs (1)
68-85: Docs: compute time is compile-time, not runtime.These proc-macros precompute values during compilation. Update wording to avoid confusion.
-/// Derives a Light Protocol CPI signer PDA at runtime +/// Derives a Light Protocol CPI signer PDA at compile time @@ -/// for the given program ID. Uses `solana_pubkey` with `solana` feature, -/// otherwise uses `pinocchio` (default). +/// for the given program ID.program-libs/hasher/src/hash_to_field_size.rs (2)
43-54: Library code: replace unwrap with expect (or return Result) and simplify push loopUnwraps can panic in production. Either (a) keep the infallible signature and use
expect(...)with rationale, or (b) introduce aResult-returning variant and keep this as a thin wrapper. Also, you can replace thefor_eachpush loop withextend_from_slice.Apply this diff:
pub fn hashv_to_bn254_field_size_be(bytes: &[&[u8]]) -> [u8; 32] { let mut slices = Vec::with_capacity(bytes.len() + 1); - bytes.iter().for_each(|x| slices.push(*x)); + slices.extend_from_slice(bytes); let bump_seed = [HASH_TO_FIELD_SIZE_SEED]; slices.push(bump_seed.as_slice()); - // SAFETY: cannot panic Hasher::hashv returns an error because Poseidon can panic. - let mut hashed_value: [u8; 32] = Keccak::hashv(&slices).unwrap(); + let mut hashed_value: [u8; 32] = + Keccak::hashv(&slices).expect("Keccak::hashv is infallible for Keccak backend");If you prefer the Result path, I can propose a
hashv_to_bn254_field_size_be_result(...) -> Result<[u8;32], HasherError>and have this function call.unwrap_or_else(|_| unreachable!()).
57-67: Same unwrap concern here; align with the approach chosen aboveUse
expect(...)(or add a Result variant) instead ofunwrap()to avoid panics.- let mut hashed_value: [u8; 32] = Keccak::hashv(&slices).unwrap(); + let mut hashed_value: [u8; 32] = + Keccak::hashv(&slices).expect("Keccak::hashv is infallible for Keccak backend");program-libs/compressed-account/src/instruction_data/with_readonly.rs (2)
61-81: Avoid panics in From<...>; prefer a fallible conversionTwo
.expect("...")calls will panic ifdatais missing. Replace withTryFromreturningCompressedAccountError(or a dedicated error) to keep invariants enforced without panicking.Example:
-impl From<PackedCompressedAccountWithMerkleContext> for InAccount { - fn from(value: PackedCompressedAccountWithMerkleContext) -> Self { - Self { - discriminator: value.compressed_account.data.as_ref().expect("Into InAccount expected data to exist.").discriminator, - merkle_context: value.merkle_context, - data_hash: value.compressed_account.data.as_ref().expect("Into InAccount expected data to exist.").data_hash, - ... - } - } -} +impl TryFrom<PackedCompressedAccountWithMerkleContext> for InAccount { + type Error = CompressedAccountError; + fn try_from(value: PackedCompressedAccountWithMerkleContext) -> Result<Self, Self::Error> { + let data = value + .compressed_account + .data + .as_ref() + .ok_or(CompressedAccountError::InvalidArgument)?; + Ok(Self { + discriminator: data.discriminator, + merkle_context: value.merkle_context, + data_hash: data.data_hash, + root_index: value.root_index, + lamports: value.compressed_account.lamports, + address: value.compressed_account.address, + }) + } +}
613-627: Gate tests for serializer and std to avoid cfg-mismatch build failuresThese tests use BorshSerialize and std RNG/println. Add
not(feature="anchor")andfeature="std"to the cfg so they don’t compile under Anchor-only or no_std builds.-#[cfg(all(not(feature = "pinocchio"), feature = "new-unique"))] +#[cfg(all( + not(feature = "pinocchio"), + feature = "new-unique", + not(feature = "anchor"), + feature = "std" +))]program-libs/compressed-account/src/address.rs (1)
19-30: Optional: replace unwrap with expect for clearer invariantsIf Keccak path is truly infallible, prefer
expect("...")over a bareunwrap()to document the assumption.- hashv_to_bn254_field_size_be_const_array::<4>(&slices).unwrap() + hashv_to_bn254_field_size_be_const_array::<4>(&slices) + .expect("hashv_to_bn254_field_size_be_const_array::<4> should be infallible for Keccak")program-libs/compressed-account/src/compressed_account.rs (2)
325-339: Avoid pattern-matching parameter to take ownership of PubkeyPrefer borrowing directly to avoid needless copy/readability oddity.
- pub fn hash( - &self, - &merkle_tree_pubkey: &Pubkey, - leaf_index: &u32, - is_batched: bool, - ) -> Result<[u8; 32], CompressedAccountError> { + pub fn hash( + &self, + merkle_tree_pubkey: &Pubkey, + leaf_index: &u32, + is_batched: bool, + ) -> Result<[u8; 32], CompressedAccountError> {- pub fn hash( - &self, - &merkle_tree_pubkey: &[u8; 32], - leaf_index: &u32, - is_batched: bool, - ) -> Result<[u8; 32], CompressedAccountError> { + pub fn hash( + &self, + merkle_tree_pubkey: &[u8; 32], + leaf_index: &u32, + is_batched: bool, + ) -> Result<[u8; 32], CompressedAccountError> {Also applies to: 364-377
148-157: Consider also deriving ZeroCopy for PackedReadOnlyCompressedAccountAll fields look POD/ZeroCopy-compatible (repr(C), fixed-size fields, PackedMerkleContext is ZeroCopy). Adding ZeroCopy would allow read-only views without mutation APIs.
-#[derive(Debug, PartialEq, Default, Clone, ZeroCopyMut)] +#[derive(Debug, PartialEq, Default, Clone, ZeroCopy, ZeroCopyMut)] pub struct PackedReadOnlyCompressedAccount {program-libs/compressed-account/src/lib.rs (1)
27-30: Re-export hash_to_bn254_field_size_be without requiring allochash_to_bn254_field_size_be doesn’t allocate; gate only the vector-based variant behind alloc.
-pub use light_hasher::hash_to_field_size::{ - hash_to_bn254_field_size_be, hashv_to_bn254_field_size_be, -}; +pub use light_hasher::hash_to_field_size::hash_to_bn254_field_size_be; +#[cfg(feature = "alloc")] +pub use light_hasher::hash_to_field_size::hashv_to_bn254_field_size_be;program-tests/utils/src/compressed_account_pack.rs (3)
70-75: Avoid building a Vec just to take [0]Pack single MerkleContext without allocating an intermediate Vec.
- merkle_context: pack_merkle_context(&[merkle_context], remaining_accounts)[0], + merkle_context: { + let m = merkle_context; + PackedMerkleContext { + leaf_index: m.leaf_index, + merkle_tree_pubkey_index: pack_account(&m.merkle_tree_pubkey, remaining_accounts), + queue_pubkey_index: pack_account(&m.queue_pubkey, remaining_accounts), + prove_by_index: m.prove_by_index, + } + },- merkle_context: pack_merkle_context(&[x.merkle_context], remaining_accounts)[0], + merkle_context: { + let m = x.merkle_context; + PackedMerkleContext { + leaf_index: m.leaf_index, + merkle_tree_pubkey_index: pack_account(&m.merkle_tree_pubkey, remaining_accounts), + queue_pubkey_index: pack_account(&m.queue_pubkey, remaining_accounts), + prove_by_index: m.prove_by_index, + } + },- .map(|merkle_context| PackedMerkleContext { + .map(|merkle_context| PackedMerkleContext { leaf_index: merkle_context.leaf_index, merkle_tree_pubkey_index: pack_account( &merkle_context.merkle_tree_pubkey, remaining_accounts, ), queue_pubkey_index: pack_account(&merkle_context.queue_pubkey, remaining_accounts), prove_by_index: merkle_context.prove_by_index, })Also applies to: 236-241, 95-111
43-51: Deduplicate pack_account and pack_pubkey_usizeBoth do the same. Keep one and call it everywhere to avoid divergence.
-pub fn pack_pubkey_usize(pubkey: &Pubkey, hash_set: &mut HashMap<Pubkey, usize>) -> u8 { - match hash_set.get(pubkey) { - Some(index) => (*index) as u8, - None => { - let index = hash_set.len(); - hash_set.insert(*pubkey, index); - index as u8 - } - } -} +pub fn pack_pubkey_usize(pubkey: &Pubkey, map: &mut HashMap<Pubkey, usize>) -> u8 { + pack_account(pubkey, map) +}Also applies to: 219-228
244-326: Tests are good; add cases for length mismatch and index overflowAdd tests that assert length mismatch panics and that inserting 256+ unique pubkeys trips the overflow guard.
| light-hasher = { path = "program-libs/hasher", version = "4.0.0", default-features = false } | ||
| light-macros = { path = "program-libs/macros", version = "2.1.0" } | ||
| light-merkle-tree-reference = { path = "program-tests/merkle-tree", version = "3.0.1" } | ||
| light-heap = { path = "program-libs/heap", version = "2.0.0" } | ||
| light-prover-client = { path = "prover/client", version = "3.0.0" } | ||
| light-sdk = { path = "sdk-libs/sdk", version = "0.15.0" } | ||
| light-sdk-pinocchio = { path = "sdk-libs/sdk-pinocchio", version = "0.13.0" } | ||
| light-sdk-macros = { path = "sdk-libs/macros", version = "0.15.0" } | ||
| light-sdk-types = { path = "sdk-libs/sdk-types", version = "0.15.0" } | ||
| light-compressed-account = { path = "program-libs/compressed-account", version = "0.5.0" } | ||
| light-sdk-types = { path = "sdk-libs/sdk-types", version = "0.15.0", default-features = false } | ||
| light-compressed-account = { path = "program-libs/compressed-account", version = "0.5.0", default-features = false } | ||
| light-compressible = { path = "program-libs/compressible", version = "0.1.0" } | ||
| light-ctoken-types = { path = "program-libs/ctoken-types", version = "0.1.0" } | ||
| light-account-checks = { path = "program-libs/account-checks", version = "0.4.0" } | ||
| light-account-checks = { path = "program-libs/account-checks", version = "0.4.0", default-features = false } |
There was a problem hiding this comment.
🧩 Analysis chain
Major refactoring: default-features = false for 4 core crates.
Disabling default features for light-hasher, light-sdk-types, light-compressed-account, and light-account-checks is a significant architectural change enabling no_std support. This requires ALL downstream crate dependencies to explicitly enable needed features or risk compilation failure.
Verify that every workspace member depending on these 4 crates has explicit feature enablement where required. The 7 provided files show this pattern, but a comprehensive audit of all workspace members is needed to prevent regressions.
🏁 Script executed:
#!/bin/bash
# Description: Verify all workspace members properly handle default-features = false for core crates.
# Find all workspace members and check if they depend on the 4 affected crates without explicit features.
affected_crates="light-hasher|light-sdk-types|light-compressed-account|light-account-checks"
echo "=== Checking for potential unhandled feature defaults in workspace members ==="
fd 'Cargo.toml' --type f | while read -r toml; do
# Skip the root Cargo.toml (already reviewed)
if [[ "$toml" == "Cargo.toml" ]]; then
continue
fi
# Search for dependencies on the 4 affected crates
if grep -E "^\s*(light-hasher|light-sdk-types|light-compressed-account|light-account-checks)\s*=" "$toml" > /dev/null; then
# Check if the line has explicit features
if grep -E "^\s*(light-hasher|light-sdk-types|light-compressed-account|light-account-checks)\s*=.*features\s*=" "$toml" > /dev/null; then
# Has explicit features, OK
:
else
# Depends on affected crate WITHOUT explicit features - potential issue
echo "⚠️ $toml depends on affected crate(s) without explicit features:"
grep -E "^\s*(light-hasher|light-sdk-types|light-compressed-account|light-account-checks)\s*=" "$toml"
fi
fi
doneLength of output: 914
I've identified a critical issue. The script found 5 workspace members that depend on the 4 affected crates using { workspace = true } without explicit feature enablement. These will inherit default-features = false from the root workspace.
Let me verify if the 4 core crates are actually designed to compile with no features enabled:
Now I need to verify if the 4 core crates are designed to compile with no features. Let me check their feature definitions:Let me check the actual Cargo.toml files of the 4 core crates to see their feature definitions:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check feature definitions in the 4 affected core crates
echo "=== Checking feature definitions in core crates ==="
for crate_dir in "program-libs/hasher" "program-libs/account-checks" "program-libs/compressed-account" "sdk-libs/sdk-types"; do
if [ -f "$crate_dir/Cargo.toml" ]; then
echo ""
echo "--- $crate_dir/Cargo.toml ---"
# Extract [features] section and show what features are defined
sed -n '/^\[features\]/,/^\[/p' "$crate_dir/Cargo.toml" | head -20
else
echo "⚠️ $crate_dir/Cargo.toml not found"
fi
done
Length of output: 1493
Now I need to check if the 5 downstream crates can actually compile with the 4 core crates having default-features = false. Let me inspect whether they have feature gates or conditional code that requires those features:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the 5 affected workspace members for code that depends on features from core crates
echo "=== Checking if affected crates can compile with default-features = false ==="
# The 5 affected crates
affected_members=(
"program-libs/bloom-filter"
"program-libs/compressible"
"program-libs/hash-set"
"program-tests/merkle-tree"
"sdk-libs/sdk-types"
)
for member in "${affected_members[@]}"; do
if [ -d "$member" ]; then
echo ""
echo "--- $member ---"
# Check for feature-gated code that references the core crates
echo "Checking for conditional imports or usage patterns..."
rg -n 'use light_(hasher|account_checks|compressed_account)' "$member/src" --type rust -A2 -B2 2>/dev/null | head -15 || echo " (no matches)"
# Check for #[cfg(feature = ...)] patterns
if rg -n '#\[cfg\(feature' "$member/src" --type rust 2>/dev/null | grep -qE 'solana|std|alloc'; then
echo "⚠️ Found feature-gated code:"
rg -n '#\[cfg\(feature' "$member/src" --type rust 2>/dev/null | head -10
else
echo " (no feature-gated patterns found)"
fi
fi
done
Length of output: 4630
Based on the investigation, I've identified critical compilation issues. The workspace change sets default-features = false for 4 core crates, but those crates still have default = ["std"] or default = ["alloc"] defined. Additionally, 5 workspace members depend on these crates via { workspace = true } without explicit feature enablement, meaning they inherit the default-features = false setting.
The critical problems:
-
program-tests/merkle-tree uses
light_hasherAPIs in main code (lib.rs line 5, indexed.rs line 3) but won't have access to the std feature required to compile those APIs. -
program-libs/compressible uses
light_account_checksin main code (config.rs line 2) without feature gates—no guarantee it works with no features enabled. -
sdk-libs/sdk-types imports from
light_compressed_accountin main code with conditionally-gated std/alloc code, but won't have those features enabled.
The 4 affected crates (light-hasher, light-account-checks, light-compressed-account, sdk-types) still have default = ["std"] or default = ["alloc"]. When default-features = false is applied at workspace level, these defaults are ignored entirely. The crates will compile with NO features unless their code is refactored to not require std/alloc internally.
Critical fixes required:
- Either remove the
default = [...]lines from the 4 core crates' Cargo.toml files (change todefault = []), OR - Explicitly enable required features in each of the 5 affected workspace members' dependencies (change
{ workspace = true }to{ workspace = true, features = ["std"] }etc.)
The refactoring is incomplete—the PR must ensure all affected crates either compile without defaults or explicitly re-enable needed features.
🤖 Prompt for AI Agents
Cargo.toml lines 171-183: the workspace sets default-features = false so crates
that still declare default = ["std"] or ["alloc"] (light-hasher,
light-account-checks, light-compressed-account, sdk-types) end up built with NO
features, breaking dependents; fix by either removing or clearing the default
feature declarations in those four crates' Cargo.toml (change default =
["std"/"alloc"] to default = []) OR update each workspace-dependent entry that
uses { workspace = true } to explicitly enable the required features (e.g., {
workspace = true, features = ["std"] } or ["alloc"] as appropriate) for all
affected members (including program-tests/merkle-tree,
program-libs/compressible, sdk-libs/sdk-types and the other two workspace
dependents), and ensure any conditional code is either refactored or properly
feature-gated.
| default = ["std"] | ||
| std = [] |
There was a problem hiding this comment.
Violates coding guideline: default build should enable none.
The coding guideline explicitly states for program-libs/account-checks/**/Cargo.toml: "Define features solana, pinocchio, and test-only in Cargo.toml; default build should enable none." Setting default = ["std"] contradicts this requirement.
Apply this diff to align with the guideline:
-default = ["std"]
+default = []
std = []Based on coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| default = ["std"] | |
| std = [] | |
| default = [] | |
| std = [] |
🤖 Prompt for AI Agents
In program-libs/account-checks/Cargo.toml around lines 10 to 11, the Cargo.toml
sets default = ["std"] which violates the guideline that the default build
should enable none; update the file to set default = [] and ensure the features
section explicitly defines the features "solana", "pinocchio", and "test-only"
(with any optional dependencies mapped under those feature names) so the default
build enables no features.
| #[cfg_attr( | ||
| all(feature = "std", feature = "anchor"), | ||
| derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize) | ||
| )] | ||
| #[cfg_attr( | ||
| not(feature = "anchor"), | ||
| derive(borsh::BorshDeserialize, borsh::BorshSerialize) | ||
| )] |
There was a problem hiding this comment.
🧩 Analysis chain
Serialization derives disappear when anchor is enabled without std
Current cfg gating applies Anchor derives only with all(std, anchor) and Borsh derives when not(anchor). If anchor is enabled on a no_std target (typical for BPF), neither derive applies, leaving these types without any serialization traits. Fix the gating to choose by anchor alone, or explicitly forbid the unsupported combo.
Apply this diff to both structs:
-#[cfg_attr(
- all(feature = "std", feature = "anchor"),
- derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
-)]
-#[cfg_attr(
- not(feature = "anchor"),
- derive(borsh::BorshDeserialize, borsh::BorshSerialize)
-)]
+#[cfg_attr(
+ feature = "anchor",
+ derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
+)]
+#[cfg_attr(
+ not(feature = "anchor"),
+ derive(borsh::BorshDeserialize, borsh::BorshSerialize)
+)]Optionally, if anchor && !std truly isn’t supported, add a guard near the top of this file:
#[cfg(all(feature = "anchor", not(feature = "std")))]
compile_error!("Enabling `anchor` without `std` is not supported for this crate configuration. Enable `std` or disable `anchor`.");Based on learnings.
Please confirm which crate targets require anchor in no_std so we align gating across the workspace (Anchor/Pinocchio/Borsh interop).
Also applies to: 49-56
I'll verify this serialization derives issue by examining the file and checking the cfg patterns in the workspace.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the specific file and lines in question
echo "=== File: compressed_proof.rs (lines 1-60) ==="
fd -t f compressed_proof.rs --exec head -60 {}
Length of output: 2072
🏁 Script executed:
#!/bin/bash
# Search for other cfg patterns with anchor and std in the codebase
echo "=== Searching for cfg patterns with 'anchor' and 'std' ==="
rg -n "cfg_attr.*anchor.*std|cfg_attr.*std.*anchor" -C 2 --type rustLength of output: 58
🏁 Script executed:
#!/bin/bash
# Check Cargo.toml files for feature definitions
echo "=== Feature definitions in Cargo.toml files ==="
fd -t f "Cargo.toml" | head -20 | xargs -I {} sh -c 'echo "=== {} ===" && grep -A 20 "\[features\]" {} | head -25'Length of output: 9508
🏁 Script executed:
#!/bin/bash
# Search for usage of anchor feature with no_std context
echo "=== Checking for no_std usage context ==="
rg -n "no_std|std.*=.*false" --type rust | head -20Length of output: 458
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full file to count lines and find all cfg_attr patterns
echo "=== Full file with line numbers ==="
cat -n program-libs/compressed-account/src/instruction_data/compressed_proof.rs
Length of output: 2928
🏁 Script executed:
#!/bin/bash
# Search for anchor feature usage patterns across workspace
echo "=== Checking for anchor feature usage (potentially without std) ==="
rg -n "features.*anchor|anchor.*features" --type rust --type toml | grep -v "anchor-lang" | head -30Length of output: 3375
🏁 Script executed:
#!/bin/bash
# Check if there are test configs using anchor without std
echo "=== Workspace tests or configs with anchor feature ==="
find . -name "Cargo.toml" -exec grep -l "anchor" {} \; | xargs -I {} sh -c 'echo "=== {} ===" && grep -B 2 -A 2 "\[features\]" {} | grep -A 15 "anchor"'Length of output: 2012
Fix serialization cfg gating to prevent derives from disappearing when anchor is enabled without std
The cfg conditions create a serialization gap: when anchor feature is enabled without std, neither all(feature = "std", feature = "anchor") nor not(feature = "anchor") matches, leaving these types without any serialization traits. This breaks any anchor+no_std usage (e.g., no_std BPF programs using Anchor types).
The suggested fix correctly changes the condition to feature = "anchor" alone. Apply to both structs at lines 5-12 and 49-56:
-#[cfg_attr(
- all(feature = "std", feature = "anchor"),
- derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
-)]
-#[cfg_attr(
- not(feature = "anchor"),
- derive(borsh::BorshDeserialize, borsh::BorshSerialize)
-)]
+#[cfg_attr(
+ feature = "anchor",
+ derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
+)]
+#[cfg_attr(
+ not(feature = "anchor"),
+ derive(borsh::BorshDeserialize, borsh::BorshSerialize)
+)]Optionally add a guard to forbid anchor+!std if unsupported:
#[cfg(all(feature = "anchor", not(feature = "std")))]
compile_error!("Enabling `anchor` without `std` is not supported for this crate. Enable `std` or disable `anchor`.");🤖 Prompt for AI Agents
In program-libs/compressed-account/src/instruction_data/compressed_proof.rs
around lines 5-12 and 49-56, the current cfg_attr gating uses all(feature =
"std", feature = "anchor") and not(feature = "anchor") which leaves a gap when
anchor is enabled without std; change the derive gating to check feature =
"anchor" alone (i.e., #[cfg_attr(feature = "anchor",
derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize))]) for both
structs so they keep Anchor derives when anchor is enabled, and keep the borsh
derives under not(feature = "anchor"); optionally add a compile_error guard for
the unsupported combination (anchor enabled but std disabled) using
cfg(all(feature = "anchor", not(feature = "std"))) to surface a clear build-time
error.
| #[cfg_attr( | ||
| all(feature = "std", feature = "anchor"), | ||
| derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize) | ||
| )] | ||
| #[cfg_attr( | ||
| not(feature = "anchor"), | ||
| derive(borsh::BorshDeserialize, borsh::BorshSerialize) | ||
| )] | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, ZeroCopyMut)] |
There was a problem hiding this comment.
🧩 Analysis chain
Fix derive gating: Anchor should not depend on std.
cfg_attr(all(feature="std", feature="anchor"), …) drops derives when anchor is enabled without std. Align with no_std goals by deriving Anchor on feature="anchor" alone.
-#[cfg_attr(
- all(feature = "std", feature = "anchor"),
- derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
-)]
+#[cfg_attr(
+ feature = "anchor",
+ derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
+)]
#[cfg_attr(
- not(feature = "anchor"),
+ not(feature = "anchor"),
derive(borsh::BorshDeserialize, borsh::BorshSerialize)
)]Search for similar gating elsewhere (should be synchronized with other instruction_data types).
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify the specific file and find similar patterns
echo "=== Inspecting cpi_context.rs ==="
cat program-libs/compressed-account/src/instruction_data/cpi_context.rs | head -20
echo -e "\n=== Searching for similar cfg_attr patterns with all(feature=\"std\" ==="
rg -n 'all\(feature\s*=\s*"std".*feature\s*=\s*"anchor"' program-libs/compressed-account/
echo -e "\n=== Searching for any cfg_attr with both std and anchor features ==="
rg -nA2 'cfg_attr.*all.*"std"' program-libs/compressed-account/ | head -50
echo -e "\n=== Finding all instruction_data types ==="
fd -t f 'instruction_data' program-libs/compressed-account/src/ --exec ls -la {} \;
echo -e "\n=== List files in instruction_data directory ==="
find program-libs/compressed-account/src/instruction_data -type f -name "*.rs"
Length of output: 5416
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify the problematic pattern in another file and check fix scope
echo "=== Inspecting data.rs (first occurrence) ==="
sed -n '8,20p' program-libs/compressed-account/src/instruction_data/data.rs
echo -e "\n=== Inspecting with_account_info.rs (first occurrence) ==="
sed -n '32,44p' program-libs/compressed-account/src/instruction_data/with_account_info.rs
echo -e "\n=== Check if review is scoped to single file or entire codebase ==="
# Count total instances in instruction_data directory only
echo "Instances in instruction_data/:"
rg -c 'all\(feature\s*=\s*"std".*feature\s*=\s*"anchor"' program-libs/compressed-account/src/instruction_data/ | grep -v ':0$'
Length of output: 1335
Fix derive gating pattern across all instruction_data types: Anchor should not depend on std.
The problematic pattern all(feature = "std", feature = "anchor") prevents Anchor derives from being applied when the anchor feature is enabled without std, breaking no_std support. This pattern appears 19 times across 6 instruction_data files:
cpi_context.rs(1 instance)data.rs(9 instances)with_account_info.rs(4 instances)with_readonly.rs(2 instances)compressed_proof.rs(2 instances)invoke_cpi.rs(1 instance)
Apply the fix to all instances by changing the condition to feature = "anchor" alone:
#[cfg_attr(
- all(feature = "std", feature = "anchor"),
+ feature = "anchor",
derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
)]This synchronizes the derive gating across all instruction_data types and aligns with the no_std design goal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg_attr( | |
| all(feature = "std", feature = "anchor"), | |
| derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize) | |
| )] | |
| #[cfg_attr( | |
| not(feature = "anchor"), | |
| derive(borsh::BorshDeserialize, borsh::BorshSerialize) | |
| )] | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, ZeroCopyMut)] | |
| #[cfg_attr( | |
| feature = "anchor", | |
| derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize) | |
| )] | |
| #[cfg_attr( | |
| not(feature = "anchor"), | |
| derive(borsh::BorshDeserialize, borsh::BorshSerialize) | |
| )] | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, ZeroCopyMut)] |
🤖 Prompt for AI Agents
In program-libs/compressed-account/src/instruction_data/cpi_context.rs around
lines 8 to 16, the derive gating uses all(feature = "std", feature = "anchor")
which prevents Anchor derives when the crate is built no_std; change the
cfg_attr to only check feature = "anchor" (i.e., cfg_attr(feature = "anchor",
derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize))) so Anchor
derives are enabled regardless of std, and apply the same change consistently
across the other listed instruction_data files.
| pub fn pack_account(pubkey: &Pubkey, remaining_accounts: &mut HashMap<Pubkey, usize>) -> u8 { | ||
| match remaining_accounts.get(pubkey) { | ||
| Some(index) => *index as u8, | ||
| None => { | ||
| let next_index = remaining_accounts.len(); | ||
| remaining_accounts.insert(*pubkey, next_index); | ||
| next_index as u8 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
u8 index overflow risk when >255 remaining accounts
Casting usize→u8 will wrap. Add a guard (assert in tests) to fail loudly.
- match remaining_accounts.get(pubkey) {
+ match remaining_accounts.get(pubkey) {
Some(index) => *index as u8,
None => {
let next_index = remaining_accounts.len();
+ assert!(next_index <= u8::MAX as usize, "remaining_accounts index overflow");
remaining_accounts.insert(*pubkey, next_index);
next_index as u8
}
}- match hash_set.get(pubkey) {
+ match hash_set.get(pubkey) {
Some(index) => (*index) as u8,
None => {
let index = hash_set.len();
+ assert!(index <= u8::MAX as usize, "remaining_accounts index overflow");
hash_set.insert(*pubkey, index);
index as u8
}
}- vec.push(*remaining_accounts.get(pubkey).unwrap() as u8);
+ let idx = *remaining_accounts.get(pubkey).unwrap();
+ assert!(idx <= u8::MAX as usize, "remaining_accounts index overflow");
+ vec.push(idx as u8);Also applies to: 43-51, 158-175
🤖 Prompt for AI Agents
In program-tests/utils/src/compressed_account_pack.rs around lines 219-228 (and
likewise at lines 43-51 and 158-175), the code casts usize→u8 which will wrap
when there are >255 accounts; add an explicit guard before casting: compute
next_index, assert or panic with a clear message if next_index > u8::MAX (or use
an if returning a Result with Err), then perform the cast only when safe so the
failure is loud and deterministic (also update tests to assert the guard
triggers for overflow).
| pub mod assert_token_tx; | ||
| pub mod assert_transfer2; | ||
| pub mod batched_address_tree; | ||
| pub mod compressed_account_pack; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify no duplication with existing pack module.
A new compressed_account_pack module is being added alongside the existing pack module (line 43). Please confirm that these modules serve distinct purposes and that functionality is not duplicated.
🏁 Script executed:
#!/bin/bash
# Description: Compare exported symbols from pack and compressed_account_pack modules
echo "=== Checking pack module exports ==="
rg -n "^pub (fn|struct|enum|type|const)" program-tests/utils/src/pack.rs -A 1
echo "=== Checking compressed_account_pack module exports ==="
rg -n "^pub (fn|struct|enum|type|const)" program-tests/utils/src/compressed_account_pack.rs -A 1Length of output: 2319
I need to verify if these are true duplicates by examining the actual implementations:
I need to verify if the implementations are actually duplicated by examining the code:
---
🏁 Script executed:
```shell
#!/bin/bash
# Compare implementations of overlapping functions
echo "=== pack.rs: pack_pubkey_usize implementation ==="
sed -n '211,220p' program-tests/utils/src/pack.rs
echo ""
echo "=== compressed_account_pack.rs: pack_pubkey_usize implementation ==="
sed -n '43,52p' program-tests/utils/src/compressed_account_pack.rs
echo ""
echo "=== pack.rs: pack_compressed_accounts implementation ==="
sed -n '159,183p' program-tests/utils/src/pack.rs
echo ""
echo "=== compressed_account_pack.rs: pack_compressed_accounts implementation ==="
sed -n '53,79p' program-tests/utils/src/compressed_account_pack.rs
echo ""
echo "=== Check lib.rs module declarations ==="
sed -n '36,50p' program-tests/utils/src/lib.rs
Length of output: 3149
Let me check if there's documentation or comments explaining this duplication:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for comments or documentation explaining module separation
echo "=== Check compressed_account_pack.rs header/comments ==="
head -20 program-tests/utils/src/compressed_account_pack.rs
echo ""
echo "=== Check pack.rs header/comments ==="
head -20 program-tests/utils/src/pack.rs
echo ""
echo "=== Search for usages of both modules in tests ==="
rg "use.*pack::" program-tests/ -c
rg "use.*compressed_account_pack::" program-tests/ -c
Length of output: 2276
Let me check which specific functions from each module are actually being used:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check specific usages of functions from each module
echo "=== Functions from pack module used in tests ==="
rg "pack::" program-tests/ -n | head -20
echo ""
echo "=== Functions from compressed_account_pack module used in tests ==="
rg "compressed_account_pack::" program-tests/ -n | head -20
Length of output: 2436
Consolidate duplicate functions between pack and compressed_account_pack modules.
Verification confirms significant code duplication: 10 out of 11 functions in compressed_account_pack.rs are identical duplicates of functions in pack.rs, including pack_pubkey_usize, pack_compressed_accounts, pack_merkle_context, pack_new_address_params, and others. The pack module is more widely used (wildcard imports in e2e_test_env.rs and multiple direct imports), while compressed_account_pack has limited usage.
Options:
- Consolidate functions into a single module and deprecate the other
- Clarify if the separation serves a distinct architectural purpose (not apparent from current code)
- Re-export overlapping functions from one module to avoid duplication
🤖 Prompt for AI Agents
In program-tests/utils/src/lib.rs around line 36, the compressed_account_pack
module duplicates most functions from pack; consolidate by moving the shared
implementations into the primary pack module (pack.rs) or a new common
submodule, then have compressed_account_pack either re-export those functions
(pub use crate::pack::{...};) or be removed and replaced with a thin deprecated
wrapper (add #[deprecated] to the wrapper module). Update all imports across the
repo (e2e_test_env.rs and others) to reference the consolidated module, run
cargo check/tests to ensure no breakage, and remove the duplicate
implementations from compressed_account_pack.rs.
| #[cfg(feature = "light-account")] | ||
| fn with_light_account<A>( | ||
| mut self, | ||
| account: crate::LightAccount<'_, A>, | ||
| ) -> Result<Self, pinocchio::program_error::ProgramError> | ||
| where | ||
| A: BorshSerialize | ||
| + BorshDeserialize | ||
| + LightDiscriminator | ||
| A: crate::BorshSerialize | ||
| + borsh::BorshDeserialize | ||
| + crate::LightDiscriminator | ||
| + light_hasher::DataHasher | ||
| + Default, | ||
| { |
There was a problem hiding this comment.
Inconsistent trait bounds may break feature-gated builds.
First impl uses crate::BorshDeserialize; second uses borsh::BorshDeserialize. Use the same alias to respect crate feature gating.
- A: crate::BorshSerialize
- + borsh::BorshDeserialize
+ A: crate::BorshSerialize
+ + crate::BorshDeserialize
+ crate::LightDiscriminator
+ light_hasher::DataHasher
+ Default,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(feature = "light-account")] | |
| fn with_light_account<A>( | |
| mut self, | |
| account: crate::LightAccount<'_, A>, | |
| ) -> Result<Self, pinocchio::program_error::ProgramError> | |
| where | |
| A: BorshSerialize | |
| + BorshDeserialize | |
| + LightDiscriminator | |
| A: crate::BorshSerialize | |
| + borsh::BorshDeserialize | |
| + crate::LightDiscriminator | |
| + light_hasher::DataHasher | |
| + Default, | |
| { | |
| #[cfg(feature = "light-account")] | |
| fn with_light_account<A>( | |
| mut self, | |
| account: crate::LightAccount<'_, A>, | |
| ) -> Result<Self, pinocchio::program_error::ProgramError> | |
| where | |
| A: crate::BorshSerialize | |
| crate::BorshDeserialize | |
| crate::LightDiscriminator | |
| light_hasher::DataHasher | |
| Default, | |
| { |
🤖 Prompt for AI Agents
In sdk-libs/sdk-pinocchio/src/cpi/v2/invoke.rs around lines 98 to 109, the trait
bounds are inconsistent between implementations: one uses
crate::BorshDeserialize while the other uses borsh::BorshDeserialize; to respect
feature-gating via the crate alias, change the borsh::BorshDeserialize reference
to crate::BorshDeserialize so both impls use the same alias and compile
correctly under feature flags.
| let mut inputs: [&[u8]; 16] = [&[]; 16]; | ||
|
|
||
| inputs.push(program_id.as_slice()); | ||
| inputs[0] = program_id.as_slice(); | ||
|
|
||
| inputs.extend(seeds); | ||
| for (i, seed) in seeds.iter().enumerate() { | ||
| inputs[i + 1] = seed; | ||
| } |
There was a problem hiding this comment.
Guard against too many seeds (panic risk).
Fixed array has 16 slots (program_id + up to 15 seeds). If seeds.len() > 15, the inputs[i + 1] write will panic at runtime.
Apply a hard precondition before filling:
pub fn derive_address_seed(seeds: &[&[u8]], program_id: &[u8; 32]) -> AddressSeed {
- let mut inputs: [&[u8]; 16] = [&[]; 16];
+ assert!(
+ seeds.len() <= 15,
+ "v1::derive_address_seed supports at most 15 seeds (program_id + 15)"
+ );
+ let mut inputs: [&[u8]; 16] = [&[]; 16];
inputs[0] = program_id.as_slice();
for (i, seed) in seeds.iter().enumerate() {
inputs[i + 1] = seed;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut inputs: [&[u8]; 16] = [&[]; 16]; | |
| inputs.push(program_id.as_slice()); | |
| inputs[0] = program_id.as_slice(); | |
| inputs.extend(seeds); | |
| for (i, seed) in seeds.iter().enumerate() { | |
| inputs[i + 1] = seed; | |
| } | |
| assert!( | |
| seeds.len() <= 15, | |
| "v1::derive_address_seed supports at most 15 seeds (program_id + 15)" | |
| ); | |
| let mut inputs: [&[u8]; 16] = [&[]; 16]; | |
| inputs[0] = program_id.as_slice(); | |
| for (i, seed) in seeds.iter().enumerate() { | |
| inputs[i + 1] = seed; | |
| } |
🤖 Prompt for AI Agents
In sdk-libs/sdk-types/src/address.rs around lines 38 to 44, add an explicit
precondition that seeds.len() must be <= 15 before writing into the fixed
16-element inputs array to avoid a runtime panic; check if seeds.len() > 15 and
handle it immediately (prefer returning a descriptive Result::Err if the
surrounding function returns Result, otherwise panic with a clear message)
instead of letting the loop attempt out-of-bounds writes.
| #[cfg(all(not(feature = "std"), feature = "alloc"))] | ||
| use alloc::vec::Vec; | ||
| #[cfg(feature = "std")] | ||
| use std::vec::Vec; |
There was a problem hiding this comment.
Feature-gate methods that depend on Vec allocation.
The conditional Vec imports are correct, but methods to_account_infos() (line 204) and tree_pubkeys() (line 224) use Vec without being feature-gated. If neither "std" nor "alloc" feature is enabled, these methods will fail to compile because Vec won't be in scope.
Add feature gates to the methods:
+ #[cfg(any(feature = "std", feature = "alloc"))]
pub fn to_account_infos(&self) -> Vec<T> {+ #[cfg(any(feature = "std", feature = "alloc"))]
pub fn tree_pubkeys(&self) -> Result<Vec<T::Pubkey>> {Based on learnings.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
sdk-libs/sdk-types/src/cpi_accounts/v2.rs around lines 204 to 224: the methods
to_account_infos() (line ~204) and tree_pubkeys() (line ~224) use Vec but are
not feature-gated, so compilation fails when neither "std" nor "alloc" is
enabled; add a feature gate to each method such as #[cfg(any(feature = "std",
feature = "alloc"))] (or the equivalent project convention) above their
definitions so they are only compiled when Vec is available, ensuring the crate
builds when Vec is not in scope.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
program-libs/account-checks/Cargo.toml (1)
10-11: Violates coding guideline: default build should enable none.This issue was already flagged in a previous review. The coding guideline explicitly states: "Define features solana, pinocchio, and test-only in Cargo.toml; default build should enable none." Setting
default = ["std"]contradicts this requirement.Apply this diff to align with the guideline:
-default = ["std"] +default = [] std = []Based on coding guidelines.
program-libs/ctoken-types/Cargo.toml (1)
21-21: Unconditional std feature may conflict with no_std goals (unresolved from prior review).The crate declares
default = [](line 9) to support no_std, but unconditionally enables std on light-compressed-account. This contradicts the stated no_std support and the PR's "refactor: libs to nostd" objective.Consider gating the std feature behind a feature flag (e.g.,
std = ["light-compressed-account/std"]) and removing the unconditional feature from line 21.
🧹 Nitpick comments (1)
sdk-libs/sdk-pinocchio/src/cpi/traits.rs (1)
112-115: Consider more granular error mapping.The error mapping collapses all
CompressedAccountErrorvariants exceptInputTooLargeintoInvalidArgument. This loses diagnostic information compared to theinvokemethod's comprehensive error handling. For better debuggability, consider mapping additional error variants or using the error code directly.For example:
- let data = self.data_array::<N>().map_err(|e| match e { - CompressedAccountError::InputTooLarge(_) => ProgramError::InvalidInstructionData, - _ => ProgramError::InvalidArgument, - })?; + let data = self.data_array::<N>().map_err(|e| { + let error_code: u32 = e.into(); + ProgramError::Custom(error_code) + })?;This preserves the full error context by using the error code conversion defined in
compressed-account/src/lib.rs.
📜 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)
program-libs/account-checks/Cargo.toml(2 hunks)program-libs/account-checks/src/account_info/test_account_info.rs(2 hunks)program-libs/compressed-account/Cargo.toml(3 hunks)program-libs/ctoken-types/Cargo.toml(2 hunks)program-libs/hasher/Cargo.toml(2 hunks)program-libs/zero-copy/Cargo.toml(1 hunks)sdk-libs/macros/Cargo.toml(1 hunks)sdk-libs/sdk-pinocchio/src/cpi/traits.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
program-libs/account-checks/**/Cargo.toml
📄 CodeRabbit inference engine (program-libs/account-checks/CLAUDE.md)
Define features solana, pinocchio, and test-only in Cargo.toml; default build should enable none
Files:
program-libs/account-checks/Cargo.toml
program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs}
📄 CodeRabbit inference engine (program-libs/account-checks/CLAUDE.md)
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}
Gate SDK-specific implementations with #[cfg(feature = "solana"|"pinocchio"|"test-only")]
Files:
program-libs/account-checks/src/account_info/test_account_info.rs
program-libs/account-checks/src/**/*.rs
📄 CodeRabbit inference engine (program-libs/account-checks/CLAUDE.md)
program-libs/account-checks/src/**/*.rs: On account initialization, call account_info_init to set the 8-byte discriminator
Validate account type with 8-byte discriminators using check_discriminator before deserialization
Return AccountError variants (codes 12006–12021) and rely on automatic ProgramError conversions; avoid returning raw ProgramError directly
Files:
program-libs/account-checks/src/account_info/test_account_info.rs
program-libs/account-checks/src/account_info/test_account_info.rs
📄 CodeRabbit inference engine (program-libs/account-checks/CLAUDE.md)
Use the mock AccountInfo implementation under the test-only feature for unit tests
Files:
program-libs/account-checks/src/account_info/test_account_info.rs
🧠 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/**/Cargo.toml : Define features solana, pinocchio, and test-only in Cargo.toml; default build should enable none
Applied to files:
program-libs/account-checks/Cargo.tomlsdk-libs/macros/Cargo.tomlprogram-libs/hasher/Cargo.tomlprogram-libs/ctoken-types/Cargo.tomlprogram-libs/compressed-account/Cargo.toml
📚 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/account_info_trait.rs : Ensure the crate compiles with no features enabled by keeping trait definitions free of SDK-specific dependencies
Applied to files:
program-libs/account-checks/Cargo.tomlprogram-libs/account-checks/src/account_info/test_account_info.rssdk-libs/sdk-pinocchio/src/cpi/traits.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-libs/account-checks/Cargo.tomlsdk-libs/macros/Cargo.tomlprogram-libs/account-checks/src/account_info/test_account_info.rsprogram-libs/hasher/Cargo.tomlprogram-libs/ctoken-types/Cargo.tomlsdk-libs/sdk-pinocchio/src/cpi/traits.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:
program-libs/account-checks/src/account_info/test_account_info.rsprogram-libs/ctoken-types/Cargo.toml
📚 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:
program-libs/account-checks/src/account_info/test_account_info.rsprogram-libs/ctoken-types/Cargo.tomlsdk-libs/sdk-pinocchio/src/cpi/traits.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/test_account_info.rs : Use the mock AccountInfo implementation under the test-only feature for unit tests
Applied to files:
program-libs/account-checks/src/account_info/test_account_info.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-libs/account-checks/src/account_info/test_account_info.rssdk-libs/sdk-pinocchio/src/cpi/traits.rs
📚 Learning: 2025-10-16T06:33:55.362Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/compressed-token/program/CLAUDE.md:0-0
Timestamp: 2025-10-16T06:33:55.362Z
Learning: Applies to programs/compressed-token/program/program-libs/ctoken-types/** : Define all state and instruction data structures in the light-ctoken-types crate (program-libs/ctoken-types), including state/, instructions/, and state/extensions/
Applied to files:
program-libs/ctoken-types/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 : 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:
sdk-libs/sdk-pinocchio/src/cpi/traits.rs
🧬 Code graph analysis (1)
sdk-libs/sdk-pinocchio/src/cpi/traits.rs (5)
sdk-libs/sdk-pinocchio/src/cpi/v2/invoke.rs (2)
with_light_account(28-76)with_light_account(99-118)sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs (2)
with_light_account(70-117)data(143-148)program-libs/compressed-account/src/instruction_data/invoke_cpi.rs (1)
data(38-48)program-libs/compressed-account/src/lib.rs (5)
from(84-108)from(113-115)from(120-122)from(150-159)from(204-212)sdk-libs/sdk-pinocchio/src/error.rs (5)
from(90-92)from(96-98)from(102-104)from(108-135)from(139-180)
⏰ 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-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- 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 (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-token-test-program, ["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: stateless-js-v2
- GitHub Check: lint
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: cli-v2
🔇 Additional comments (15)
program-libs/zero-copy/Cargo.toml (1)
29-29: Verify the intent of the self-referential dev-dependency.While valid in Rust, having
light-zero-copydepend on itself as a dev-dependency is unusual. This pattern works because dev-dependencies don't create circular runtime dependencies, but it's worth confirming this is intentional.Could you clarify the purpose? For example:
- Are tests/examples using the crate's public API directly (e.g., to validate derive macros work with
stdenabled)?- Could this be replaced with direct imports or example code instead?
program-libs/compressed-account/Cargo.toml (4)
10-12: Verify default feature alignment with no_std migration goals.The default feature now includes
["alloc"], but the PR objective is to refactor tono_std. Related guidance suggests workspace defaults should enable no features. This may create an implicit alloc dependency for users who simply depend on this crate without explicitly requesting it.Clarify: Is
allocintended to be the default, or shoulddefault = []be used with alloc as an opt-in feature?
27-27: Light-zero-copy's std feature is always enabled unconditionally.Line 27 hard-codes
features = ["std", "mut", "derive"]on thelight-zero-copydependency. This contradicts the no_std goal by always pulling in the std feature. Should this feature enablement be conditional (e.g., behind thestdfeature of this crate)?
39-39: Verify tinyvec is truly unconditional or should be feature-gated.TinyVec is added as an unconditional workspace dependency (line 39). If it is only used in certain code paths (e.g., only in std code, or only behind the alloc feature), it should be moved into the appropriate feature gate to reduce the dependency footprint in minimal no_std configurations.
31-31: Good: solana-msg is now optional.Making
solana-msgoptional and tying it to thestdfeature (line 12) aligns well with the no_std migration, allowing no_std builds to avoid the dependency.program-libs/account-checks/src/account_info/test_account_info.rs (2)
61-64: LGTM! Appropriate migration to core for no_std compatibility.The migration from
std::mem::forgetandstd::mem::transmuteto theircore::equivalents is correct for no_std support. These intrinsic operations are available in core and do not require the standard library.
106-109: LGTM! Proper feature gating for std-dependent code.The enhanced feature gate
#[cfg(all(feature = "solana", feature = "std"))]correctly restricts this module to builds where both features are enabled, since it requires std features likeRc,RefCell, andVec. The explicitextern crate std;declaration makes the std dependency clear in a no_std context.program-libs/ctoken-types/Cargo.toml (1)
52-52: Clarify self-referential dev-dependency.Line 52 adds
light-ctoken-typesas a dev-dependency to itself (the package name islight-ctoken-typeson line 2). This appears to be a copy-paste error or unintentional.Verify whether this line was intended or should reference a different crate.
program-libs/hasher/Cargo.toml (4)
10-18: Feature architecture aligns with no_std patterns.The feature structure correctly enables granular control:
stdfeature enablesalloc, which is the standard no_std patternsolana,pinocchio,poseidon, and the newsha256/keccakfeatures are independent- Default includes
std, allowing users to opt-in to no_std by disabling default featuresThis design is solid for the nostd refactor objective.
50-51: No issue found — self-dependency is intentional and actively used.The "circular" dev-dependency is actually a standard self-dependency pattern. The package
light-hashercorrectly declares itself as a dev-dependency to test its functionality with specific feature combinations. All five test files (bigint.rs, data_hasher.rs, hash_chain.rs, hash_to_field_size.rs, to_byte_array.rs) actively import and uselight_hasher, confirming the dev-dependency is necessary and intentional.Likely an incorrect or invalid review comment.
31-31: Feature gating of borsh is correct and properly implemented.Verification confirms:
borshis configured asoptional = truein Cargo.toml- The
allocfeature properly gates borsh viaalloc = ["dep:borsh"]- All borsh usage in the codebase is protected:
hash_to_field_size.rs:2hasuse borsh::BorshSerialize;correctly guarded with#[cfg(feature = "alloc")]The implementation matches the review comment's assessment exactly.
24-24: tinyvec is actively used and correctly configured as an unconditional dependency.The crate uses
ArrayVecfrom tinyvec in thehashv_to_bn254_field_size_be_const_arrayfunction (line 67) to create a bounded, stack-allocated vector. This function and its callerhash_to_bn254_field_size_beare both unconditionally compiled, making tinyvec a legitimate unconditional dependency.sdk-libs/sdk-pinocchio/src/cpi/traits.rs (3)
1-20: LGTM: Clean no_std feature gates.The conditional imports follow standard Rust no_std patterns, properly gating
Vec,Pubkey, andLightSdkErrorbased on available allocator features.
43-67: LGTM: Trait signatures updated for no_std compatibility.The changes properly:
- Feature-gate
with_light_accountandinvoke- Add
invoke_arraywithout feature gates, enabling no_std CPI calls- Standardize error handling with
ProgramError- Update serialization traits to use external
borsh::paths
93-93: ****The concern about a dangling reference is incorrect.
Pubkeyimplements theCopytrait, which means that creating a reference to a temporaryPubkeyvalue (via&Pubkey::from(...)) is safe and valid in Rust. The compiler ensures the copied value's lifetime is sufficient for the reference. No changes are needed at line 93.Likely an incorrect or invalid review comment.
| solana-pubkey = { workspace = true, features = ["curve25519", "sha2"] } | ||
|
|
||
| light-hasher = { workspace = true } | ||
| light-hasher = { workspace = true, features = ["sha256"] } |
There was a problem hiding this comment.
🧩 Analysis chain
Feature selection looks aligned with no_std refactor.
The explicit feature selections are consistent with the PR goal:
- Main dependencies (line 19) pin
light-hashertosha256, suitable for no_std builds. - Dev dependencies (line 22) enable
stdforlight-compressed-account, which is appropriate for testing. - Test-specific features (line 29:
poseidon,keccak) are isolated in dev-dependencies.
This follows the existing pattern of explicit feature declarations (e.g., solana-pubkey at line 17).
Please verify that the selected features (sha256, poseidon, keccak) are no_std-compatible and that enabling std on light-compressed-account in dev-dependencies does not inadvertently pull in std dependencies into the main build. You can verify this by checking the manifest for each dependency:
Also applies to: 22-22, 29-29
🏁 Script executed:
#!/bin/bash
# Verify feature definitions in workspace crates
echo "=== Checking light-hasher features ==="
fd 'light-hasher' --type d -x sh -c 'test -f "{}/Cargo.toml" && echo "File: {}/Cargo.toml" && rg -A 20 "\[features\]" "{}/Cargo.toml"'
echo ""
echo "=== Checking light-compressed-account features ==="
fd 'light-compressed-account' --type d -x sh -c 'test -f "{}/Cargo.toml" && echo "File: {}/Cargo.toml" && rg -A 20 "\[features\]" "{}/Cargo.toml"'
echo ""
echo "=== Verifying no_std references in light-sdk-macros dependencies ==="
rg 'default\s*=' --type toml | grep -A 3 -B 3 'light-hasher\|light-compressed-account'Length of output: 160
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the actual Cargo.toml file in question
echo "=== Contents of sdk-libs/macros/Cargo.toml ==="
cat sdk-libs/macros/Cargo.toml
echo ""
echo "=== Checking workspace members definition ==="
fd Cargo.toml --type f | head -5 | xargs -I {} sh -c 'echo "File: {}" && rg -A 50 "\[workspace\]" "{}" | head -20'
Length of output: 1865
🏁 Script executed:
#!/bin/bash
# Search for light-hasher and light-compressed-account in all Cargo.toml files
echo "=== Searching for light-hasher across all Cargo.toml files ==="
rg 'light-hasher' --type toml -B 2 -A 2
echo ""
echo "=== Searching for light-compressed-account across all Cargo.toml files ==="
rg 'light-compressed-account' --type toml -B 2 -A 2Length of output: 67922
🏁 Script executed:
#!/bin/bash
# Check if these are external crates by looking at their presence in workspace
echo "=== Checking for workspace members that might contain these crates ==="
find . -name "Cargo.toml" -type f -exec grep -l "name.*light" {} \;
echo ""
echo "=== Listing all workspace members ==="
fd Cargo.toml --type f -x grep -H "^\[package\]" {} | head -20Length of output: 2403
🏁 Script executed:
#!/bin/bash
# Verify feature definitions in the workspace crates
echo "=== program-libs/hasher/Cargo.toml features ==="
cat program-libs/hasher/Cargo.toml | sed -n '/\[features\]/,/\[/p' | head -30
echo ""
echo "=== program-libs/compressed-account/Cargo.toml features ==="
cat program-libs/compressed-account/Cargo.toml | sed -n '/\[features\]/,/\[/p' | head -40
echo ""
echo "=== program-libs/hasher/Cargo.toml default features ==="
rg 'default\s*=' program-libs/hasher/Cargo.toml
echo ""
echo "=== program-libs/compressed-account/Cargo.toml default features ==="
rg 'default\s*=' program-libs/compressed-account/Cargo.tomlLength of output: 332
🏁 Script executed:
#!/bin/bash
# Verify that dev-dependencies don't affect main build by checking Cargo.lock or workspace resolution
echo "=== Checking if light-hasher main dependency has default-features setting ==="
rg 'light-hasher.*workspace.*true' sdk-libs/macros/Cargo.toml -A 1 -B 1
echo ""
echo "=== Checking if light-compressed-account main dependency avoids std ==="
rg 'light-compressed-account' sdk-libs/macros/Cargo.toml -B 1 -A 1Length of output: 493
🏁 Script executed:
#!/bin/bash
# Check the complete dependencies section with all flags
echo "=== Full [dependencies] section in sdk-libs/macros/Cargo.toml ==="
cat sdk-libs/macros/Cargo.toml | sed -n '/^\[dependencies\]/,/^\[/p'
echo ""
echo "=== Check if light-hasher uses default-features = false anywhere ==="
rg 'light-hasher.*default-features' sdk-libs/macros/Cargo.toml
echo ""
echo "=== Compare with how other SDK libs handle light-hasher ==="
rg 'light-hasher.*workspace.*true' sdk-libs/sdk/Cargo.toml -A 1 -B 1Length of output: 645
🏁 Script executed:
#!/bin/bash
# Verify the entire file structure to ensure complete context
echo "=== Complete sdk-libs/macros/Cargo.toml ==="
cat sdk-libs/macros/Cargo.tomlLength of output: 1008
🏁 Script executed:
#!/bin/bash
# Check git history or PR diff to see if default-features = false was removed
echo "=== Searching for default-features = false patterns in comparable crates ==="
rg 'default-features\s*=\s*false' program-libs/hasher/Cargo.toml -B 2 -A 2
echo ""
echo "=== Check how other program-libs handle light-hasher deps ==="
rg 'light-hasher.*workspace' program-libs/*/Cargo.toml | head -20
echo ""
echo "=== Verify light-hasher can be compiled without std ==="
cat program-libs/hasher/Cargo.toml | sed -n '/\[features\]/,/\[dependencies\]/p'Length of output: 1572
Add default-features = false to light-hasher main dependency (line 19).
The current configuration will inadvertently include the std feature despite the no_std refactor goal. In Cargo, specifying features = [...] without default-features = false enables both the default features AND the explicitly listed features.
Since light-hasher has default = ["std"], line 19 will activate both std and sha256. To maintain no_std compatibility, use:
light-hasher = { workspace = true, default-features = false, features = ["sha256"] }Also applies to line 29 (dev-dependencies) for consistency:
light-hasher = { workspace = true, default-features = false, features = ["poseidon", "keccak"] }Dev-dependencies won't affect the main build, but explicit default-features = false ensures the intent is clear and prevents accidental std inclusion during testing.
🤖 Prompt for AI Agents
In sdk-libs/macros/Cargo.toml around line 19 (and also update line 29 in the
[dev-dependencies] section), the light-hasher dependency currently specifies
features but does not disable default features which causes the crate's default
"std" feature to be enabled; update the dependency entries to add
default-features = false so the dependency entries explicitly disable defaults
and only enable the listed features (for line 19 set default-features = false
with features = ["sha256"], and for line 29 set default-features = false with
features = ["poseidon","keccak"]).
| use light_compressed_account::CompressedAccountError; | ||
| // Validate mode consistency | ||
| if accounts.get_mode() != self.get_mode() { | ||
| return Err(ProgramError::InvalidArgument); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling for mode mismatch.
Line 108 returns ProgramError::InvalidArgument for mode mismatch, while line 78 in the invoke method returns ProgramError::from(LightSdkError::ModeMismatch). These methods should handle the same validation error consistently.
Apply this diff to align the error handling:
- if accounts.get_mode() != self.get_mode() {
- return Err(ProgramError::InvalidArgument);
- }
+ if accounts.get_mode() != self.get_mode() {
+ return Err(ProgramError::Custom(16035)); // LightSdkError::ModeMismatch
+ }Note: Error code 16035 corresponds to ModeMismatch based on the error mappings in error.rs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use light_compressed_account::CompressedAccountError; | |
| // Validate mode consistency | |
| if accounts.get_mode() != self.get_mode() { | |
| return Err(ProgramError::InvalidArgument); | |
| } | |
| use light_compressed_account::CompressedAccountError; | |
| // Validate mode consistency | |
| if accounts.get_mode() != self.get_mode() { | |
| return Err(ProgramError::Custom(16035)); // LightSdkError::ModeMismatch | |
| } |
🤖 Prompt for AI Agents
In sdk-libs/sdk-pinocchio/src/cpi/traits.rs around lines 105 to 109, the
mode-mismatch branch returns ProgramError::InvalidArgument which is inconsistent
with the invoke method; change the return to
ProgramError::from(LightSdkError::ModeMismatch) so both places use the same
LightSdkError mapping (alternatively use ProgramError::Custom(16035) if the
LightSdkError type isn't in scope), and ensure the LightSdkError import is
present or add it.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
program-libs/zero-copy/src/traits/zero_copy_at.rs (1)
169-171: Missing alloc feature gate causes compilation error.The
ZeroCopyStructInnerimpl forVec<T>usesVec, which is only imported under#[cfg(feature = "alloc")](Lines 1-2). Without gating this impl, the code will fail to compile when the alloc feature is disabled.Apply this fix to add the missing feature gate:
+#[cfg(feature = "alloc")] impl<T: ZeroCopyStructInner + Copy> ZeroCopyStructInner for Vec<T> { type ZeroCopyInner = Vec<T::ZeroCopyInner>; }Note: The equivalent impl in
zero_copy_at_mut.rs(Lines 187-190) is correctly gated, demonstrating the expected pattern.program-libs/zero-copy/src/traits/vec_u8.rs (1)
38-74: VecU8 uses u8 length encoding while Vec uses U32—this is a real inconsistency requiring clarification or remediation.I've verified the claims in the review comment against the actual codebase:
Confirmed findings:
Length encoding mismatch: VecU8 (lines 44, 64 in
vec_u8.rs) usesRef::<&[u8], u8>::from_prefix, limiting capacity to 255 elements. Vec (line 122 inzero_copy_at.rs) usesRef::<&[u8], U32>::from_prefix, supporting ~4 billion elements.Heap exhaustion check present in Vec: Lines 124–130 in
zero_copy_at.rsinclude an explicit heap exhaustion defense with the comment "Prevent heap exhaustion attacks by checking if num_slices is reasonable."Heap exhaustion check absent in VecU8: Neither
VecU8::zero_copy_at()norVecU8::zero_copy_at_mut()implements this check.No documented design rationale: The codebase contains no comments or documentation explaining why VecU8 intentionally deviates from Vec's approach.
Recommendation: This inconsistency requires action—either align VecU8 with Vec by using U32 and adding the heap exhaustion check, or document the intentional constraint in VecU8's type documentation explaining its limited capacity for small-vector use cases.
program-libs/compressed-account/src/instruction_data/invoke_cpi.rs (1)
37-48: <!-- [scratchpad_update]
[task verify_divergence 1/2 examine_usages 1/1]
[observations_added]
- Standard Solana wire format: discriminator (8 bytes) + serialized args (variable)
- The trait default matches Solana standard format (no length prefix)
- invoke_cpi.rs adds non-standard 4-byte length: discriminator + u32_len + args
- From web search: Anchor CPI standard does NOT include length prefix for inline args in instruction data
- The pattern of adding 4-byte length is seen in some SDK code (transfer/system) but those are NOT using LightInstructionData trait
[analysis_update]
- Review comment is identifying a genuine non-compliance with standard Solana wire format
- invoke_cpi.rs deviates from both trait default AND Solana standard
- The 4-byte length seen elsewhere (transfer/system) is for different purposes and not part of instruction CPI wire format standard
- This is a critical compatibility issue
[pending]
- Verify if deserialization code on CPI consumer expects this 4-byte length or follows standard format
- Check if removing the length field would break any actual CPI consumers
[actions]
Search for CPI consumers/deserializers that process InstructionDataInvokeCpi data output
[scratchpad_end] --><!-- [scratchpad_update]
[task verify_divergence 1/2 examine_usages 2/2][observations_added]
- Borsh format: "The size of dynamic containers (such as hash maps and hash sets) is written as a 32-bit unsigned integer before the values" (18-11)
- Strings in Borsh have 4-byte length prefix (18-2)
- When serializing a structure containing Vec, Borsh DOES include the length prefix
- The manual 4-byte length in invoke_cpi.rs appears to be attempting to match this Borsh behavior
[analysis_update]
- The 4-byte length is NOT for the instruction wire format, it's for the payload (inputs) serialization
- invoke_cpi.rs is manually adding Vec length prefix that Borsh would automatically add
- BUT: the payload
inputsis already Borsh-serialized bytry_to_vec()at line 42- When you call
try_to_vec()on a BorshSerialize type, it already contains its own structure- The manual length field at line 45 is DOUBLE-WRAPPING the payload length
Key insight: If
inputsis already Borsh-serialized data fromself.try_to_vec(), adding another 4-byte length prefix creates: discriminator + length_of_borsh_data + borsh_data, which is incorrectNeed to verify: what exactly is in
inputsand what does the trait default do?
From traits.rs:29-33, the default impl just does: discriminator + inputs (no extra length)The review comment is correct: this is a divergence that will cause incompatibility.
[pending]
- Verify if anything on the CPI consumer side expects and parses this extra 4-byte length field
[actions]
Search codebase for deserialization/parsing of LightInstructionData instruction output
[scratchpad_end] -->Now let me search the codebase to verify if any CPI consumers parse this 4-byte length field:<!-- [scratchpad_update]
[task verify_divergence - conclusion 2/2][observations_final]
- Light protocol uses CPI to call system program
- Standard Solana CPI format: instruction discriminator + serialized args (no length prefix)
- invoke_cpi.rs adds: discriminator + 4-byte-length + serialized-args
- traits.rs default adds: discriminator + serialized-args (correct format)
- No evidence of any deserializer expecting the 4-byte length prefix in Light protocol
- The learnings note emphasizes serialization compatibility across feature combinations
- Multiple trait impls use the default (no length), only InstructionDataInvokeCpi adds it
[analysis_final]
The review comment is CORRECT and identifies a critical compatibility issue:
- InstructionDataInvokeCpi diverges from the trait's standard wire format
- This diverges from Solana standard CPI instruction data format
- The 4-byte length appears to be an accidental double-wrapping of Borsh serialized data
- This will break CPI compatibility between programs built with different features
The suggested fix (remove the 4-byte length) is correct and matches:
- The trait default implementation
- The Solana standard CPI wire format
- Other implementations using the default trait
[done]
Review comment is verified as correct. The 4-byte length field in invoke_cpi.rs lines 45 creates a wire-format divergence that will cause compatibility issues across feature combinations.
[scratchpad_end] -->Based on my verification of the codebase and investigation of the underlying standards, I can confirm the review comment is accurate. The issue is a genuine wire-format divergence that requires correction.
Remove the 4-byte length field to match the trait's standard CPI wire format.
The
InstructionDataInvokeCpi::data()implementation at lines 43-47 incorrectly adds a u32 length prefix that diverges from both the trait's default implementation and the Solana standard CPI instruction data format (which is: discriminator + serialized args, with no additional length field).The trait default (
traits.rslines 27-33) correctly writes: discriminator + serialized args. This matches the Solana CPI standard and is used by otherLightInstructionDataimplementations. The extra 4-byte prefix at line 45 will cause deserialization failures when CPI consumers expect the standard format.Apply the suggested fix:
#[cfg(all(feature = "alloc", not(feature = "anchor")))] fn data(&self) -> Result<Vec<u8>, crate::CompressedAccountError> { use borsh::BorshSerialize; let inputs = self .try_to_vec() .map_err(|_| crate::CompressedAccountError::InvalidArgument)?; - let mut data = Vec::with_capacity(12 + inputs.len()); + let mut data = Vec::with_capacity(8 + inputs.len()); data.extend_from_slice(self.discriminator()); - data.extend_from_slice(&(inputs.len() as u32).to_le_bytes()); data.extend_from_slice(inputs.as_slice()); Ok(data) }program-libs/hasher/src/hash_to_field_size.rs (3)
42-54: Avoid panics: gate infallible helpers to keccak/solana or make them fallible.Both functions unwrap
Keccak::hashv. Withoutfeature="keccak"on non-Solana targets,hashvreturns an error → panic.Non-breaking option: compile only where keccak/solana is available.
-#[cfg(feature = "alloc")] +#[cfg(all(feature = "alloc", any(target_os = "solana", feature = "keccak")))] pub fn hashv_to_bn254_field_size_be(bytes: &[&[u8]]) -> [u8; 32] { @@ - let mut hashed_value: [u8; 32] = Keccak::hashv(&slices).unwrap(); + let mut hashed_value: [u8; 32] = Keccak::hashv(&slices).unwrap(); @@ } -#[cfg(feature = "alloc")] +#[cfg(all(feature = "alloc", any(target_os = "solana", feature = "keccak")))] pub fn hashv_to_bn254_field_size_be_array(bytes: &[[u8; 32]]) -> [u8; 32] { @@ - let mut hashed_value: [u8; 32] = Keccak::hashv(&slices).unwrap(); + let mut hashed_value: [u8; 32] = Keccak::hashv(&slices).unwrap();Alternative (breaking but safer): change return types to
Result<[u8;32], HasherError>and use?.Also applies to: 56-67
69-85: Fix potential underflow in MAX_SLICES check.
MAX_SLICES - 1underflows whenMAX_SLICES == 0(debug panic, release wrap). Compare againstbytes.len() + 1instead.- if bytes.len() > MAX_SLICES - 1 { + if bytes.len() + 1 > MAX_SLICES { return Err(HasherError::InvalidInputLength(MAX_SLICES, bytes.len())); }
93-101: Also gate the convenience wrapper or make it fallible.This unwrap will panic without
keccakon non-Solana targets. Align gating with helpers.-/// ``` -/// # #[cfg(feature = "keccak")] +/// ``` +/// # #[cfg(feature = "keccak")] /// # use light_hasher::hash_to_field_size::hash_to_bn254_field_size_be; @@ -pub fn hash_to_bn254_field_size_be(bytes: &[u8]) -> [u8; 32] { +#[cfg(any(target_os = "solana", feature = "keccak"))] +pub fn hash_to_bn254_field_size_be(bytes: &[u8]) -> [u8; 32] { hashv_to_bn254_field_size_be_const_array::<2>(&[bytes]).unwrap() }
♻️ Duplicate comments (3)
program-libs/ctoken-types/Cargo.toml (1)
21-21: Unconditional std feature contradicts no_std PR objective—still unresolved from prior review.Line 21 still unconditionally enables
features = ["std"]onlight-compressed-account, despite the past review flagging this as conflicting with the no_std refactoring goal. The crate definesdefault = [](line 9) to support no_std environments, but this dependency forces std transitively.Add a
stdfeature to this crate and gate the dependency's feature:[features] anchor = ["light-compressed-account/anchor", "dep:anchor-lang", "light-compressible/anchor"] solana = ["dep:solana-program-error", "dep:solana-sysvar"] default = [] +std = ["light-compressed-account/std"] profile-program = []-light-compressed-account = { workspace = true, features = ["std"] } +light-compressed-account = { workspace = true }sdk-libs/sdk-types/src/address.rs (2)
38-44: Buffer overflow risk remains unaddressed (duplicate concern).The previous review flagged that if
seeds.len() > 15, writinginputs[i + 1]will panic at runtime. This critical issue has not been resolved in the current changes.
116-119: Guard against too many seeds (panic risk).The function calls
hashv_to_bn254_field_size_be_const_array::<17>with unwrap but doesn't validate thatseeds.len() <= 17. If more than 17 seeds are passed, this will panic at runtime.Add a precondition check:
pub fn derive_address_seed(seeds: &[&[u8]]) -> AddressSeed { + assert!( + seeds.len() <= 17, + "v2::derive_address_seed supports at most 17 seeds (16 seeds + 1 for bump)" + ); // Max 16 seeds + 1 for bump AddressSeed(hashv_to_bn254_field_size_be_const_array::<17>(seeds).unwrap()) }
🧹 Nitpick comments (5)
program-libs/compressed-account/Cargo.toml (1)
39-39: Specify default-features for tinyvec to support no_std builds.Line 39 adds
tinyvecwithoutdefault-features = falseor explicit features. Per learnings, tinyvec may have default-enabled heap allocation behaviors. For consistency with the no_std refactoring goal and the explicit feature gating applied to other dependencies (e.g.,light-zero-copy,borsh), explicitly disable defaults and/or gate allocation-related features:-tinyvec = { workspace = true } +tinyvec = { workspace = true, default-features = false, features = ["alloc"] }(Adjust feature set based on actual tinyvec usage and no_std requirements.)
sdk-libs/client/src/rpc/client.rs (1)
686-686: Optional: Remove redundant import.The
use light_compressed_account::TreeType;statement is redundant sinceTreeTypeis already imported at the module level (line 9).Apply this diff to remove the redundant import:
- use light_compressed_account::TreeType; use solana_pubkey::pubkey;sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs (1)
91-92: Consider simplifying error conversion chain.The two-step error conversion (
.map_err(crate::error::LightSdkError::from).map_err(|e| ProgramError::Custom(u32::from(e)))) could potentially be simplified if there's a directFrom<OriginalError> for ProgramErrorimplementation available.Based on the relevant code snippets,
CompressedAccountErroralready implementsFrom<CompressedAccountError> for ProgramError. If the intermediateLightSdkErrorconversion isn't adding value, consider using the direct conversion.Also applies to: 108-109
program-libs/compressed-account/src/lib.rs (2)
131-139: Serialization derives: confirm borsh derive feature and std defaults.Good switch to cfg_attr. Please ensure:
borshhasfeatures = ["derive"]anddefault-features = falsein this crate.anchorderives only compile underall(feature = "std", feature = "anchor").You can add a brief module-level doc explaining the feature matrix for Anchor/Borsh to prevent misuse. Based on learnings.
Also applies to: 167-176
215-221: Prefer Pubkey fields (and consider serde/borsh derives) for CpiSigner.Using
[u8; 32]loses type intent. Suggest:
- Use
Pubkeyforprogram_idandcpi_signer.- If serialized across boundaries, gate derives:
#[cfg_attr(all(feature="std", feature="anchor"), derive(anchor_lang::AnchorSerialize, anchor_lang::AnchorDeserialize))]#[cfg_attr(not(feature="anchor"), derive(borsh::BorshSerialize, borsh::BorshDeserialize))]-pub struct CpiSigner { - pub program_id: [u8; 32], - pub cpi_signer: [u8; 32], - pub bump: u8, -} +pub struct CpiSigner { + pub program_id: Pubkey, + pub cpi_signer: Pubkey, + pub bump: u8, +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
program-libs/account-checks/src/account_info/test_account_info.rs(3 hunks)program-libs/compressed-account/Cargo.toml(3 hunks)program-libs/compressed-account/src/address.rs(2 hunks)program-libs/compressed-account/src/instruction_data/invoke_cpi.rs(2 hunks)program-libs/compressed-account/src/instruction_data/zero_copy.rs(4 hunks)program-libs/compressed-account/src/lib.rs(8 hunks)program-libs/ctoken-types/Cargo.toml(2 hunks)program-libs/hasher/src/hash_to_field_size.rs(6 hunks)program-libs/zero-copy/Cargo.toml(2 hunks)program-libs/zero-copy/src/lib.rs(2 hunks)program-libs/zero-copy/src/traits/mod.rs(1 hunks)program-libs/zero-copy/src/traits/vec_u8.rs(3 hunks)program-libs/zero-copy/src/traits/zero_copy_at.rs(2 hunks)program-libs/zero-copy/src/traits/zero_copy_at_mut.rs(3 hunks)program-libs/zero-copy/src/traits/zero_copy_new.rs(2 hunks)program-tests/create-address-test-program/src/lib.rs(6 hunks)sdk-libs/client/src/rpc/client.rs(2 hunks)sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs(5 hunks)sdk-libs/sdk-types/src/address.rs(5 hunks)sdk-libs/sdk-types/src/instruction/account_info.rs(4 hunks)sdk-libs/sdk/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- program-libs/compressed-account/src/instruction_data/zero_copy.rs
- program-libs/zero-copy/Cargo.toml
- sdk-libs/sdk-types/src/instruction/account_info.rs
🧰 Additional context used
📓 Path-based instructions (3)
program-libs/account-checks/src/account_info/{solana.rs,pinocchio.rs,test_account_info.rs}
📄 CodeRabbit inference engine (program-libs/account-checks/CLAUDE.md)
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}
Gate SDK-specific implementations with #[cfg(feature = "solana"|"pinocchio"|"test-only")]
Files:
program-libs/account-checks/src/account_info/test_account_info.rs
program-libs/account-checks/src/**/*.rs
📄 CodeRabbit inference engine (program-libs/account-checks/CLAUDE.md)
program-libs/account-checks/src/**/*.rs: On account initialization, call account_info_init to set the 8-byte discriminator
Validate account type with 8-byte discriminators using check_discriminator before deserialization
Return AccountError variants (codes 12006–12021) and rely on automatic ProgramError conversions; avoid returning raw ProgramError directly
Files:
program-libs/account-checks/src/account_info/test_account_info.rs
program-libs/account-checks/src/account_info/test_account_info.rs
📄 CodeRabbit inference engine (program-libs/account-checks/CLAUDE.md)
Use the mock AccountInfo implementation under the test-only feature for unit tests
Files:
program-libs/account-checks/src/account_info/test_account_info.rs
🧠 Learnings (15)
📚 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:
program-libs/account-checks/src/account_info/test_account_info.rsprogram-libs/ctoken-types/Cargo.toml
📚 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:
program-libs/account-checks/src/account_info/test_account_info.rsprogram-libs/ctoken-types/Cargo.toml
📚 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-libs/account-checks/src/account_info/test_account_info.rsprogram-libs/ctoken-types/Cargo.toml
📚 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/account_info_trait.rs : Ensure the crate compiles with no features enabled by keeping trait definitions free of SDK-specific dependencies
Applied to files:
program-libs/account-checks/src/account_info/test_account_info.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/test_account_info.rs : Use the mock AccountInfo implementation under the test-only feature for unit tests
Applied to files:
program-libs/account-checks/src/account_info/test_account_info.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-libs/account-checks/src/account_info/test_account_info.rsprogram-tests/create-address-test-program/src/lib.rsprogram-libs/compressed-account/src/lib.rsprogram-libs/compressed-account/src/instruction_data/invoke_cpi.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:
program-libs/account-checks/src/account_info/test_account_info.rsprogram-libs/ctoken-types/Cargo.tomlprogram-libs/compressed-account/Cargo.tomlsdk-libs/sdk/Cargo.toml
📚 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:
program-libs/account-checks/src/account_info/test_account_info.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/client/src/rpc/client.rs
📚 Learning: 2025-10-16T06:33:55.362Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/compressed-token/program/CLAUDE.md:0-0
Timestamp: 2025-10-16T06:33:55.362Z
Learning: Applies to programs/compressed-token/program/program-libs/ctoken-types/** : Define all state and instruction data structures in the light-ctoken-types crate (program-libs/ctoken-types), including state/, instructions/, and state/extensions/
Applied to files:
program-libs/ctoken-types/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 : 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:
program-tests/create-address-test-program/src/lib.rssdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rsprogram-libs/compressed-account/src/instruction_data/invoke_cpi.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/create-address-test-program/src/lib.rsprogram-libs/compressed-account/src/instruction_data/invoke_cpi.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 : 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:
program-tests/create-address-test-program/src/lib.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 : Pass the data Vec<u8> through unchanged from the wrapper to the target program CPI; the target program performs deserialization.
Applied to files:
sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rsprogram-libs/compressed-account/src/instruction_data/invoke_cpi.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/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:
program-libs/compressed-account/src/lib.rs
🧬 Code graph analysis (9)
sdk-libs/client/src/rpc/client.rs (1)
sdk-libs/event/src/parse.rs (1)
event_from_light_transaction(83-134)
sdk-libs/sdk-types/src/address.rs (1)
program-libs/compressed-account/src/address.rs (2)
hashv_to_bn254_field_size_be_const_array(10-10)hashv_to_bn254_field_size_be_const_array(24-24)
program-tests/create-address-test-program/src/lib.rs (3)
sdk-libs/sdk-types/src/cpi_accounts/v1.rs (4)
config(205-207)new_with_config(72-78)fee_payer(110-112)account_infos(223-225)sdk-libs/sdk-types/src/cpi_accounts/v2.rs (4)
config(157-159)new_with_config(79-85)fee_payer(87-89)account_infos(175-177)sdk-libs/sdk-pinocchio/src/cpi/traits.rs (1)
to_account_metas(25-25)
program-libs/zero-copy/src/traits/mod.rs (3)
program-libs/zero-copy/src/traits/vec_u8.rs (2)
zero_copy_at(43-53)zero_copy_at_mut(61-73)program-libs/zero-copy/src/traits/zero_copy_at.rs (6)
zero_copy_at(18-18)zero_copy_at(25-28)zero_copy_at(34-47)zero_copy_at(54-60)zero_copy_at(67-73)borsh_vec_u8_as_slice(182-189)program-libs/zero-copy/src/traits/zero_copy_at_mut.rs (16)
zero_copy_at_mut(18-20)zero_copy_at_mut(28-33)zero_copy_at_mut(40-55)zero_copy_at_mut(62-66)zero_copy_at_mut(73-77)zero_copy_at_mut(85-90)zero_copy_at_mut(97-117)zero_copy_at_mut(335-340)zero_copy_at_mut(398-410)zero_copy_at_mut(463-470)zero_copy_at_mut(501-506)zero_copy_at_mut(559-578)zero_copy_at_mut(632-639)zero_copy_at_mut(673-678)zero_copy_at_mut(753-759)borsh_vec_u8_as_slice_mut(150-159)
sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs (4)
sdk-libs/sdk-pinocchio/src/cpi/traits.rs (1)
with_light_account(44-53)sdk-libs/sdk-pinocchio/src/cpi/v2/invoke.rs (2)
with_light_account(28-76)with_light_account(99-118)program-libs/compressed-account/src/lib.rs (5)
from(84-108)from(113-115)from(120-122)from(150-159)from(204-212)program-libs/compressed-account/src/instruction_data/traits.rs (3)
data(27-34)data(87-87)data(108-108)
program-libs/compressed-account/src/lib.rs (2)
program-libs/hasher/src/bigint.rs (1)
bigint_to_be_bytes_array(23-36)program-libs/hasher/src/hash_to_field_size.rs (4)
hash_to_field_size(12-12)hash_to_field_size(20-39)hash_to_bn254_field_size_be(99-101)hashv_to_bn254_field_size_be(43-54)
program-libs/compressed-account/src/instruction_data/invoke_cpi.rs (2)
sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs (1)
data(146-151)program-libs/compressed-account/src/instruction_data/traits.rs (3)
data(27-34)data(87-87)data(108-108)
program-libs/compressed-account/src/address.rs (3)
program-libs/compressed-account/src/compressed_account.rs (3)
hash(103-109)hash(324-339)hash(364-377)program-libs/indexed-array/src/array.rs (1)
hash(86-96)program-tests/system-cpi-test/src/create_pda.rs (1)
hash(542-545)
program-libs/hasher/src/hash_to_field_size.rs (2)
program-libs/hasher/src/keccak.rs (1)
hashv(18-48)program-libs/hasher/src/lib.rs (1)
hashv(41-41)
⏰ 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 (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 (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "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-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: lint
- GitHub Check: Forester e2e test
- GitHub Check: stateless-js-v1
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: cli-v1
- GitHub Check: cli-v2
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-token-test-program, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
🔇 Additional comments (38)
program-libs/ctoken-types/Cargo.toml (2)
22-22: Verify light-hasher feature gating for no_std compatibility.
light-hasheris pulled in without explicit features. Given the no_std PR objective, confirm whetherallocorstdshould be conditionally enabled via a feature or if this dependency is intentionally no_std-compatible by default.
48-49: Clarify whether dev-dependency std/solana features should be conditional.Lines 48–49 add
"std"and"solana"tolight-account-checksunconditionally in dev-dependencies. Based on learnings, this crate should definesolana,pinocchio, andtest-onlyfeatures withdefault = []. Verify whether these should be gated behind a dev feature (e.g.,dev-feature = ["light-account-checks/test-only", "light-account-checks/std", "light-account-checks/solana"]) or are appropriately unconditional for tests.program-libs/compressed-account/Cargo.toml (3)
10-12: Verify whether alloc-by-default aligns with no_std refactoring goal.Line 10 sets
default = ["alloc"], meaning this crate enables heap allocation by default. While the newallocandstdfeature hierarchy (lines 11–12) is well-structured for no_std builds, enabling alloc by default may allow consumers to accidentally pull in alloc when they intended a fully no_std build.Verify whether this is intentional (e.g., if consumers are expected to always need alloc) or if
default = []would better align with the no_std refactoring objectives, leaving it to consumers to opt-in viafeatures = ["alloc"].
27-27: Excellent feature gating for no_std support.Lines 27 and 35 correctly disable default features on
light-zero-copyandborshrespectively, with explicit opt-in for required features. This approach properly supports no_std builds while maintaining explicit control over alloc/std dependencies.Also applies to: 35-35
49-49: Dev-dependency std feature properly gated.Line 49 appropriately enables
stdon the dev-dependencylight-compressed-account, allowing tests to use full std features without requiring std for the library build itself.sdk-libs/client/src/rpc/client.rs (2)
9-13: LGTM! Import paths updated for new crate structure.The import paths have been correctly updated to use the new
light_eventcrate for event types, andTreeTypeis now imported directly fromlight_compressed_account. These changes align with the PR's objective to refactor event-related types into a dedicated workspace crate.
301-303: Excellent error handling improvement!Replacing
unwrap()withmap_errprovides much better error propagation and debugging information. The formatted error message clearly indicates the failure point and includes the underlying error details.sdk-libs/sdk/Cargo.toml (1)
43-46: All feature definitions are correct; the original review concern about feature mismatch is unfounded.The verification confirms that all referenced features are properly defined:
light-sdk-typesdefinesstd = ["alloc", "light-compressed-account/std", "solana-msg"]light-compressed-accountdefinesstd = ["alloc", "borsh/std", "dep:solana-msg", "light-zero-copy/std"]light-hasherdefines bothstd = ["alloc"]andkeccak = ["dep:sha3"]Regarding the dev-dependencies concern: when Cargo processes
light-hasher = { workspace = true, features = ["keccak"] }without explicitly disabling default features, the default features are automatically included. Since light-hasher specifiesdefault = ["std"], the dev build will have bothstdandkeccakenabled. There is no feature mismatch or compilation issue.The code changes are correct and follow proper no_std refactoring patterns.
Likely an incorrect or invalid review comment.
program-libs/account-checks/src/account_info/test_account_info.rs (4)
5-6: LGTM: Explicit std import for Vec usage.The explicit
extern crate std;anduse std::vec::Vec;are correct since this test helper module requires Vec and related macros. The pattern of usingcore::memoperations (lines 64-67) while importingstdfor collections is appropriate.
64-67: LGTM: Appropriate use of core::mem operations.Correctly shifted from
std::memtocore::memforforgetandtransmuteoperations. These functions are available incore, reducing unnecessarystddependency. The intentional memory leak pattern (line 63 comment) is acceptable for test-only mocking.
111-112: LGTM: Explicit std imports align with feature gate.The explicit
extern crate std;and consolidated imports correctly declare thestddependency now required by the feature gate (line 109). All imported items (RefCell,Rc,vec,Vec) are used in the module implementation.
109-109: Feature gate change verified—no breaking code dependencies found.The grep results confirm no code or tests in the
account-checkscrate expect thesolana_programsubmodule (at line 109) to be available withfeature = "solana"alone. The explicitstdrequirement is accurate since the implementation usesRc,RefCell, andVec, which mandate std support.sdk-libs/sdk-types/src/address.rs (1)
65-66: ****The const generic parameter
<3>is correctly sized. The functionhashv_to_bn254_field_size_be_const_arrayinternally appends abump_seedslice to the input (line 79 ofhash_to_field_size.rs), soMAX_SLICESmust account for input slices plus this additional element. For 2 input slices,<3>is correct (2 inputs + 1 bump_seed = 3 total capacity).Likely an incorrect or invalid review comment.
program-tests/create-address-test-program/src/lib.rs (2)
31-55: LGTM! Clean separation of serializable config.The
CpiAccountsConfigLocalpattern effectively separates the serializable configuration fields from the runtimecpi_signer, which is always reconstructed as the constantLIGHT_CPI_SIGNER. The bidirectional conversions are correct and maintain consistency.
106-109: LGTM! Proper bounds check added.The runtime guard correctly prevents out-of-bounds access to
remaining_accounts[1]andremaining_accounts[2]in the subsequent code (lines 117, 121). This improves safety whenwrite_cpi_contextis enabled.program-libs/zero-copy/src/traits/zero_copy_at_mut.rs (3)
1-2: LGTM! Correct alloc import for no_std support.The conditional import of
Vecfromallocis correctly gated and aligns with the no_std refactoring objectives.
93-118: LGTM! Vec deserialization with heap exhaustion protection.The implementation correctly:
- Gates Vec support behind the alloc feature
- Includes a heap exhaustion check (Line 104-108) that ensures the buffer has at least
num_slicesbytes- Uses
u32_to_usizefor safe size conversionThe check assumes each element requires at least 1 byte when serialized, which is reasonable for most use cases.
187-190: LGTM! Correctly gated trait implementation.The
ZeroCopyStructInnerMutimplementation forVec<T>is properly feature-gated and consistent with other implementations in the module.program-libs/zero-copy/src/traits/mod.rs (1)
1-13: LGTM! Consistent alloc feature gating.All new alloc-dependent modules and re-exports are correctly gated with
#[cfg(feature = "alloc")], ensuring the crate can build in no_std environments without heap allocation support.program-libs/zero-copy/src/lib.rs (2)
2-3: LGTM! Correct extern crate declaration for no_std + alloc.The conditional
extern crate allocdeclaration is properly gated and necessary for using heap-allocated types in a no_std environment.
13-22: LGTM! Feature gate migration from std to alloc.The change from
stdtoallocfeature gating for the traits module and related re-exports is correct, enabling the crate to work in no_std environments with allocator support while still supporting std environments.program-libs/zero-copy/src/traits/zero_copy_at.rs (2)
1-2: LGTM! Correct alloc import for no_std support.The conditional import of
Vecfromallocis properly gated and consistent with the no_std refactoring.
117-140: LGTM! Vec deserialization with heap exhaustion protection.The
ZeroCopyAtimplementation forVec<T>correctly:
- Gates support behind the alloc feature
- Includes heap exhaustion protection (Lines 126-130)
- Uses safe size conversion with
u32_to_usizeprogram-libs/zero-copy/src/traits/zero_copy_new.rs (2)
1-2: LGTM! Correct alloc import for no_std support.The conditional import is properly gated for the no_std refactoring.
238-274: LGTM! Vec initialization with overflow protection.The
ZeroCopyNewimplementation correctly:
- Gates Vec support behind the alloc feature
- Uses
checked_addto prevent overflow when calculating byte length (Line 248)- Writes a 4-byte U32 length prefix
- Initializes each element with its configuration
The implementation is consistent with other Vec-based zero-copy traits.
program-libs/zero-copy/src/traits/vec_u8.rs (2)
1-2: LGTM! Correct alloc import for no_std support.The conditional import is properly gated.
12-21: LGTM! VecU8 wrapper type is correctly gated.The wrapper type and constructor are properly feature-gated and provide a clean API.
sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs (5)
4-5: LGTM: Feature-gated import.The conditional import of
CompressedAccountInfoTraitis correctly feature-gated and aligns with the PR's no_std refactoring goals.
129-136: LGTM: BorshSerialize implementation updated for no_std.The explicit use of
borsh::maybestd::io::Writeis correct for no_std compatibility, asmaybestdis Borsh's compatibility layer that works in both std and no_std environments.
145-151: LGTM: Correctly feature-gated for alloc.The
data()method is appropriately gated behind theallocfeature since it returnsVec<u8>, which requires heap allocation. This aligns with the PR's no_std refactoring objectives.Note that callers without the
allocfeature will need to use alternative serialization methods.
69-79: Feature gate and signature updates look good.The method signature changes are consistent with:
- The trait definition in
sdk-pinocchio/src/cpi/traits.rs(shown in relevant snippets)- The v2 implementation in
sdk-pinocchio/src/cpi/v2/invoke.rs- The PR's goal of feature-gated no_std support
The use of
crate::prefixes for trait bounds improves scoping clarity.
84-86: Fix incorrect error conversion at line 86 that will not compile.The conversion
u64::from(e) as u32is invalid—there is noFrom<ProgramError> for u64implementation in the codebase. This pattern is inconsistent with the correct error handling at lines 92 and 109, which properly convert viaLightSdkError::from()thenu32::from(e).Align line 86 with the pattern at lines 92 and 109:
.map_err(crate::error::LightSdkError::from) .map_err(|e| ProgramError::Custom(u32::from(e)))?Alternatively, consider whether the error should return
CompressedAccountErrordirectly (which has properFrom<CompressedAccountError> for u32implementations matching error codes 12006–12021).⛔ Skipped due to learnings
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 : Return AccountError variants (codes 12006–12021) and rely on automatic ProgramError conversions; avoid returning raw ProgramError directlyLearnt 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.rsLearnt 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 discriminatorprogram-libs/compressed-account/src/instruction_data/invoke_cpi.rs (1)
11-11: LGTM on no_std aliasing.Importing crate::Vec and InstructionDiscriminator here keeps the module no_std‑friendly and consistent.
program-libs/compressed-account/src/lib.rs (2)
185-193: LGTM on core-only Display/Default.Using
core::fmtandcore::default::Defaultkeeps no_std clean.Also applies to: 196-200
41-44: LGTM on error text clarifications.Wording updates improve diagnostics; numeric mapping below remains consistent.
program-libs/hasher/src/hash_to_field_size.rs (2)
15-40: LGTM on alloc-gated blanket impl and debug gating.Correctly propagates Borsh errors and restricts println! to
all(debug_assertions, std).
42-54: Verify workspace features ensure keccak is enabled where hash_to_bn254_field_size_be is used.The functions
hash_to_bn254_field_size_be(),hashv_to_bn254_field_size_be()and related variants useKeccak::hashv().unwrap()but are not gated on thekeccakfeature. On non-Solana platforms withoutkeccakenabled, they will panic withKeccakFeatureNotEnabled.Current usage is safe:
- Solana programs use the syscall path (always available)
- Tests explicitly enable
keccakHowever,
sdk-libs/sdk-types(which exports public address derivation functions) imports light-hasher with workspace defaults (default-features = false), making keccak unavailable. These public APIs could be called from non-Solana contexts without the feature, causing panics.Recommendation: Either gate these functions on
#[cfg(feature = "keccak")]or explicitly enablekeccakin packages that export them publicly (e.g., sdk-types). Alternatively, adopt fallible variants and propagate errors instead of unwrapping.program-libs/compressed-account/src/address.rs (1)
9-11: LGTM on const-array helper usage.No allocation; correct error propagation.
| hashv_to_bn254_field_size_be_const_array::<4>(&slices) | ||
| .expect("hashv_to_bn254_field_size_be_const_array::<4> should be infallible for Keccak") | ||
| } |
There was a problem hiding this comment.
Eliminate expect; return Result or gate to keccak/solana.
expect will panic when Keccak::hashv returns an error (e.g., feature="keccak" disabled). Prefer fallible API.
Option A (preferred; safer API):
-pub fn derive_address(
+pub fn derive_address(
seed: &[u8; 32],
merkle_tree_pubkey: &[u8; 32],
program_id_bytes: &[u8; 32],
-) -> [u8; 32] {
+) -> Result<[u8; 32], CompressedAccountError> {
@@
- hashv_to_bn254_field_size_be_const_array::<4>(&slices)
- .expect("hashv_to_bn254_field_size_be_const_array::<4> should be infallible for Keccak")
+ Ok(
+ hashv_to_bn254_field_size_be_const_array::<4>(&slices)
+ .map_err(CompressedAccountError::from)?
+ )
}Option B (non-breaking): add #[cfg(any(target_os = "solana", feature = "keccak"))] to this function as well.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In program-libs/compressed-account/src/address.rs around lines 24 to 26, remove
the expect call that can panic when Keccak is unavailable and make this function
fallible (or gate it to Keccak/Solana); specifically change the function to
return Result<_, _> and propagate the error from
hashv_to_bn254_field_size_be_const_array::<4>(&slices) using ? (or map_err to
the crate's error type), update all callers to handle the Result, and if you
prefer the non-breaking alternative instead add #[cfg(any(target_os = "solana",
feature = "keccak"))] to this function so it is only compiled when Keccak is
available.
| #[cfg_attr( | ||
| all(feature = "std", feature = "anchor"), | ||
| derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize) | ||
| )] | ||
| #[cfg_attr( | ||
| not(feature = "anchor"), | ||
| derive(borsh::BorshDeserialize, borsh::BorshSerialize) | ||
| )] | ||
| #[derive(Debug, PartialEq, Default, Clone, ZeroCopyMut)] |
There was a problem hiding this comment.
🧩 Analysis chain
Anchor derive gated on std → anchor+no_std loses derives (likely compile break).
If feature="anchor" but not "std" (typical BPF/no_std), neither Anchor nor Borsh derives apply. Drop the std requirement and derive Anchor when anchor is enabled; otherwise derive Borsh.
Apply:
#[cfg_attr(
- all(feature = "std", feature = "anchor"),
+ feature = "anchor",
derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
)]
#[cfg_attr(
not(feature = "anchor"),
derive(borsh::BorshDeserialize, borsh::BorshSerialize)
)]Also ensure Cargo enables borsh with features = ["derive"] when feature = "anchor" is off.
I'll verify this review comment by checking the actual code and Cargo configuration.
</script>🏁 Script executed:
#!/bin/bash
# Verify the file and feature gate configuration
echo "=== Checking invoke_cpi.rs lines 15-23 ==="
cat program-libs/compressed-account/src/instruction_data/invoke_cpi.rs | head -30 | tail -20
echo -e "\n=== Checking Cargo.toml for borsh configuration ==="
cat program-libs/compressed-account/Cargo.toml
echo -e "\n=== Searching for any conditional compilation on this struct ==="
rg -nB5 -A15 "ZeroCopyMut" program-libs/compressed-account/src/instruction_data/invoke_cpi.rsLength of output: 4253
Feature gate requires both std and anchor, breaking no_std+anchor builds.
The condition all(feature = "std", feature = "anchor") means Anchor derives apply only when both features are enabled. In typical BPF/no_std scenarios where feature = "anchor" is set but feature = "std" is not, neither the Anchor nor Borsh derives are applied, resulting in a compile break.
Additionally, the borsh dependency in Cargo.toml lacks the "derive" feature, so Borsh macros will fail when anchor is disabled.
Fixes required:
- Remove the std requirement from the Anchor derive gate (lines 15–17):
#[cfg_attr(
- all(feature = "std", feature = "anchor"),
+ feature = "anchor",
derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize)
)]- Add
features = ["derive"]to borsh in Cargo.toml:
-borsh = { workspace = true, default-features = false }
+borsh = { workspace = true, default-features = false, features = ["derive"] }🤖 Prompt for AI Agents
In program-libs/compressed-account/src/instruction_data/invoke_cpi.rs around
lines 15 to 23, the cfg_attr currently requires both feature = "std" and feature
= "anchor" which prevents Anchor derives in no_std+anchor builds; change the
cfg_attr to require only feature = "anchor" so the
AnchorDeserialize/AnchorSerialize derives apply when anchor is enabled without
std, and separately update Cargo.toml to add features = ["derive"] to the borsh
dependency so borsh derive macros are available when anchor is disabled.
| pub use light_hasher::bigint::bigint_to_be_bytes_array; | ||
| #[cfg(feature = "alloc")] | ||
| pub use light_hasher::hash_to_field_size::{ | ||
| hash_to_bn254_field_size_be, hashv_to_bn254_field_size_be, | ||
| }; |
There was a problem hiding this comment.
Gate bigint re-export behind alloc to avoid no_std build breaks.
bigint_to_be_bytes_array depends on alloc (BigUint::to_bytes_be). Unconditional re-export can fail in no_std without alloc.
Apply:
- pub use light_hasher::bigint::bigint_to_be_bytes_array;
+#[cfg(feature = "alloc")]
+pub use light_hasher::bigint::bigint_to_be_bytes_array;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub use light_hasher::bigint::bigint_to_be_bytes_array; | |
| #[cfg(feature = "alloc")] | |
| pub use light_hasher::hash_to_field_size::{ | |
| hash_to_bn254_field_size_be, hashv_to_bn254_field_size_be, | |
| }; | |
| #[cfg(feature = "alloc")] | |
| pub use light_hasher::bigint::bigint_to_be_bytes_array; | |
| #[cfg(feature = "alloc")] | |
| pub use light_hasher::hash_to_field_size::{ | |
| hash_to_bn254_field_size_be, hashv_to_bn254_field_size_be, | |
| }; |
🤖 Prompt for AI Agents
In program-libs/compressed-account/src/lib.rs around lines 26–30, the public
re-export of bigint_to_be_bytes_array is unconditional but it relies on alloc
(BigUint::to_bytes_be) and breaks no_std builds; guard that re-export with the
same #[cfg(feature = "alloc")] used for the other hash_to_field_size re-exports
so the line becomes conditional, and ensure the crate's Cargo features expose
"alloc" where appropriate.
| #[cfg(feature = "alloc")] | ||
| impl<T> Deref for VecU8<T> { | ||
| type Target = Vec<T>; | ||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "alloc")] | ||
| impl DerefMut for VecU8<u8> { | ||
| fn deref_mut(&mut self) -> &mut Self::Target { | ||
| &mut self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
DerefMut implementation is inconsistent with Deref.
Deref is implemented for all VecU8<T> (Line 24), but DerefMut is only implemented for VecU8<u8> (Line 32). This asymmetry means users cannot mutate VecU8<T> where T is not u8, which is unexpected and inconsistent.
Apply this fix to make DerefMut generic:
-impl DerefMut for VecU8<u8> {
+impl<T> DerefMut for VecU8<T> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(feature = "alloc")] | |
| impl<T> Deref for VecU8<T> { | |
| type Target = Vec<T>; | |
| fn deref(&self) -> &Self::Target { | |
| &self.0 | |
| } | |
| } | |
| #[cfg(feature = "alloc")] | |
| impl DerefMut for VecU8<u8> { | |
| fn deref_mut(&mut self) -> &mut Self::Target { | |
| &mut self.0 | |
| } | |
| } | |
| #[cfg(feature = "alloc")] | |
| impl<T> Deref for VecU8<T> { | |
| type Target = Vec<T>; | |
| fn deref(&self) -> &Self::Target { | |
| &self.0 | |
| } | |
| } | |
| #[cfg(feature = "alloc")] | |
| impl<T> DerefMut for VecU8<T> { | |
| fn deref_mut(&mut self) -> &mut Self::Target { | |
| &mut self.0 | |
| } | |
| } |
🤖 Prompt for AI Agents
In program-libs/zero-copy/src/traits/vec_u8.rs around lines 23 to 36, the
DerefMut impl is only for VecU8<u8> causing asymmetry with the generic Deref
impl; change the DerefMut to be generic as well by implementing impl<T> DerefMut
for VecU8<T> (guarded with #[cfg(feature = "alloc")]) and make deref_mut return
&mut self.0 with the same Target type as Deref so all VecU8<T> types can be
mutably dereferenced.
| let extra = cpi_accounts | ||
| .account_infos() | ||
| .len() | ||
| .saturating_sub(PROGRAM_ACCOUNTS_LEN); | ||
| let mut account_metas = Vec::with_capacity(1 + extra); |
There was a problem hiding this comment.
Under-allocated capacity causes unnecessary reallocations.
The capacity calculation is incorrect. The function adds approximately 6 fixed AccountMeta entries (lines 296–328) plus optional/tree accounts from account_infos, totaling roughly account_infos().len() entries. However, the current formula allocates only 1 + (len - PROGRAM_ACCOUNTS_LEN) capacity, which under-allocates by about 5 slots, forcing the Vec to reallocate during growth.
Apply this diff to fix the capacity:
- let extra = cpi_accounts
- .account_infos()
- .len()
- .saturating_sub(PROGRAM_ACCOUNTS_LEN);
- let mut account_metas = Vec::with_capacity(1 + extra);
+ let mut account_metas = Vec::with_capacity(cpi_accounts.account_infos().len());Alternatively, if you prefer expressing it as fixed + extra:
let extra = cpi_accounts
.account_infos()
.len()
.saturating_sub(PROGRAM_ACCOUNTS_LEN);
- let mut account_metas = Vec::with_capacity(1 + extra);
+ let mut account_metas = Vec::with_capacity(6 + extra);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let extra = cpi_accounts | |
| .account_infos() | |
| .len() | |
| .saturating_sub(PROGRAM_ACCOUNTS_LEN); | |
| let mut account_metas = Vec::with_capacity(1 + extra); | |
| let mut account_metas = Vec::with_capacity(cpi_accounts.account_infos().len()); |
🤖 Prompt for AI Agents
In program-tests/create-address-test-program/src/lib.rs around lines 290 to 294,
the Vec capacity is under-allocated (currently set to 1 + extra) which causes
reallocations because there are roughly PROGRAM_ACCOUNTS_LEN fixed entries plus
the optional/tree accounts; change the allocation to reserve the full expected
size (fixed entries + extra) — e.g. use PROGRAM_ACCOUNTS_LEN + extra (or
directly account_infos().len()) as the with_capacity argument so the Vec is
sized correctly up front.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
program-libs/compressed-account/src/instruction_data/traits.rs (2)
3-7: Previous conditional import issue resolved.The conditional
AnchorSerializeimport now correctly uses#[cfg(all(feature = "std", feature = "anchor"))]at line 3, which addresses the gap identified in the previous review where enablinganchorwithoutstdwould leave no import available. The fallback toborsh::BorshSerializeat line 7 now covers all other feature combinations.
35-44: Critical:data_arrayoverwrites discriminator and fails to track length.The
self.serialize(&mut data.as_mut_slice())call at line 40 writes over the discriminator bytes added at line 39, becauseas_mut_slice()returns a mutable slice of the ArrayVec's current contents. Additionally,serialize()writes to the slice but doesn't update the ArrayVec's internal length tracker, leavingdata.len()at just the discriminator length.Based on the pattern in
program-libs/compressed-account/src/instruction_data/invoke_cpi.rs(lines 37-47), serialize to a temporary buffer first, then extend:fn data_array<const N: usize>(&self) -> Result<ArrayVec<[u8; N]>, CompressedAccountError> { let mut data = ArrayVec::new(); - // Add discriminator data.extend_from_slice(self.discriminator()); - self.serialize(&mut data.as_mut_slice()) - .map_err(|_e| CompressedAccountError::InputTooLarge(data.len().saturating_sub(N)))?; - + + // Serialize into a temporary buffer + let mut temp_buf = [0u8; N]; + let remaining = N.saturating_sub(data.len()); + let mut cursor = &mut temp_buf[..remaining]; + self.serialize(&mut cursor) + .map_err(|_| CompressedAccountError::InvalidArgument)?; + let written = remaining - cursor.len(); + data.try_extend_from_slice(&temp_buf[..written]) + .map_err(|_| CompressedAccountError::InputTooLarge(written))?; + Ok(data) }Cargo.toml (1)
171-171: Explicitly declare required features for downstream crates—multiple workspace members are missing critical feature enablement.The script confirms the core concern: setting
default-features = falsefor 4 of the 5 affected crates (light-hasher,light-account-checks,light-compressed-account,light-sdk-types) breaks downstream crates using{ workspace = true }without explicit features, since they inheritdefault-features = falseand won't compile if code requiresstdoralloc.Spot-checking reveals the issue is widespread:
- ✓ Some crates have proper features (e.g.,
sdk-libs/sdk/Cargo.tomldeclares features for most affected crates;forester-utils/Cargo.tomlhas features forlight-hasherandlight-compressed-account)- ✗ Many crates are still missing feature declarations, including:
program-libs/bloom-filter/Cargo.toml:light-hasher = { workspace = true }(needs features)program-libs/compressible/Cargo.toml:light-hasherandlight-account-checkslack feature declarationsanchor-programs/system/Cargo.toml:light-zero-copy = { workspace = true }(needs features)forester-utils/Cargo.toml:light-account-checks = { workspace = true }(needs features)Audit all 51+ workspace members and add explicit feature declarations (e.g.,
features = ["std"]orfeatures = ["solana"]depending on context). Verify the workspace compiles with these changes before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Cargo.toml(3 hunks)program-libs/account-checks/Cargo.toml(2 hunks)program-libs/batched-merkle-tree/src/errors.rs(2 hunks)program-libs/compressed-account/Cargo.toml(3 hunks)program-libs/compressed-account/src/instruction_data/traits.rs(1 hunks)program-libs/compressible/Cargo.toml(1 hunks)program-libs/hasher/tests/hash_to_field_size.rs(1 hunks)program-libs/zero-copy/tests/borsh.rs(1 hunks)scripts/lint.sh(1 hunks)sdk-libs/compressed-token-sdk/Cargo.toml(2 hunks)sdk-libs/sdk/Cargo.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/lint.sh
- program-libs/account-checks/Cargo.toml
- sdk-libs/compressed-token-sdk/Cargo.toml
🧰 Additional context used
🧠 Learnings (9)
📚 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-libs/compressible/Cargo.tomlprogram-libs/compressed-account/Cargo.tomlprogram-libs/compressed-account/src/instruction_data/traits.rsCargo.toml
📚 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:
program-libs/compressible/Cargo.tomlprogram-libs/compressed-account/Cargo.tomlsdk-libs/sdk/Cargo.toml
📚 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:
program-libs/batched-merkle-tree/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:
program-libs/batched-merkle-tree/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/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-libs/batched-merkle-tree/src/errors.rssdk-libs/sdk/Cargo.toml
📚 Learning: 2025-10-16T06:33:55.362Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: programs/compressed-token/program/CLAUDE.md:0-0
Timestamp: 2025-10-16T06:33:55.362Z
Learning: Applies to programs/compressed-token/program/programs/compressed-token/anchor/src/lib.rs : Define all program-specific error codes in anchor_compressed_token::ErrorCode in programs/compressed-token/anchor/src/lib.rs; return errors as ProgramError::Custom(error_code as u32)
Applied to files:
program-libs/batched-merkle-tree/src/errors.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-libs/compressed-account/src/instruction_data/traits.rs
📚 Learning: 2025-10-11T21:59:52.712Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: program-libs/account-checks/docs/CLAUDE.md:0-0
Timestamp: 2025-10-11T21:59:52.712Z
Learning: Applies to program-libs/account-checks/docs/**/DISCRIMINATOR.md : DISCRIMINATOR.md must document 8-byte discriminators, the Discriminator trait, constant arrays for compile-time checks, and integration with account initialization
Applied to files:
program-libs/compressed-account/src/instruction_data/traits.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:
program-libs/compressed-account/src/instruction_data/traits.rs
🧬 Code graph analysis (2)
program-libs/hasher/tests/hash_to_field_size.rs (2)
program-libs/hasher/src/hash_to_field_size.rs (5)
hash_to_field_size(12-12)hash_to_field_size(20-39)hashv_to_bn254_field_size_be(43-54)is_smaller_than_bn254_field_size_be(104-109)hash_to_bn254_field_size_be(99-101)program-libs/hasher/src/bigint.rs (1)
bigint_to_be_bytes_array(23-36)
program-libs/compressed-account/src/instruction_data/traits.rs (3)
program-libs/compressed-account/src/instruction_data/invoke_cpi.rs (2)
discriminator(126-128)data(38-48)sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs (2)
discriminator(139-141)data(146-151)sdk-libs/sdk-pinocchio/src/cpi/traits.rs (1)
data_array(112-112)
⏰ 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-v2
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- 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-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- 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-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: stateless-js-v1
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: stateless-js-v2
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: lint
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (sdk-token-test-program, ["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: cli-v1
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
🔇 Additional comments (21)
program-libs/zero-copy/tests/borsh.rs (1)
288-294: LGTM! Improves consistency across test suite.The change from
Result<(), Box<dyn std::error::Error>>to a plain unit return type with.unwrap()alignstest_array_structwith the pattern used by all other tests in this file. Using.unwrap()in tests is appropriate since test failures should panic rather than propagate errors.program-libs/compressed-account/src/instruction_data/traits.rs (3)
1-1: LGTM: Correct no_std migration.Switching from
std::fmt::Debugtocore::fmt::Debugcorrectly supports no_std environments while maintaining functionality.
9-9: LGTM: Appropriate no_std vector choice.The
tinyvec::ArrayVecimport supports stack-allocated, fixed-capacity vectors suitable for no_std environments, aligning with the newdata_arraymethod.
24-33: LGTM: Correct allocation-aware implementation.The
data()method is properly gated with#[cfg(feature = "alloc")]sincetry_to_vecrequires allocation. The use ofcrate::Vecand the serialization pattern (discriminator followed by serialized inputs) align with established patterns in the codebase.program-libs/hasher/tests/hash_to_field_size.rs (1)
31-44: LGTM!The test correctly validates that
hash_to_field_size()produces the same result as manually serializing with Borsh and callinghash_to_bn254_field_size_be().program-libs/compressible/Cargo.toml (1)
9-9: LGTM! Anchor feature correctly enables std.Adding
light-compressed-account/stdto the anchor feature is appropriate, as Anchor programs operate in a std environment. This ensures proper feature propagation given that the workspace now setsdefault-features = falseforlight-compressed-account.sdk-libs/sdk/Cargo.toml (3)
14-14: Breaking change: borsh is now always included.The removal of
borshfrom the default feature set (line 14) combined with making it a required dependency (line 38) means borsh is now unconditionally compiled into all builds. Previously, users could opt out viadefault-features = false.Is this intentional? If certain build configurations should exclude borsh, consider keeping it optional and adding it to appropriate feature sets.
Also applies to: 38-38
42-42: Good—explicit std feature enablement.These changes correctly address the workspace-level
default-features = falseby explicitly enabling thestdfeature forlight-sdk-types,light-compressed-account, andlight-hasher. This prevents compilation failures when default features are disabled.Also applies to: 44-45
52-52: LGTM! Keccak feature added for testing.Adding the
keccakfeature tolight-hasherin dev-dependencies is appropriate for testing hash functionality.Cargo.toml (3)
27-27: New workspace member added.The addition of
sdk-libs/eventintroduces a new crate to the workspace, likely for event handling functionality mentioned in the PR summary.
170-170: New dependency for event handling.The
light-eventcrate is added to support the new workspace member atsdk-libs/event.
129-129: Borsh default features disabled workspace-wide.Setting
default-features = falsefor borsh aligns with the no_std refactoring goals. Ensure all crates that require borsh features (likederive) explicitly enable them.Verify that all workspace members using borsh have explicit feature declarations where needed (e.g.,
features = ["derive"]).program-libs/batched-merkle-tree/src/errors.rs (2)
39-41: Good—prevents duplicate ProgramError variants.Refining the feature gate to
#[cfg(all(feature = "solana", not(feature = "pinocchio")))]ensures that only oneProgramErrorvariant exists when both features are enabled. Line 36 already defines aProgramError(u64)variant for pinocchio, so this change prevents a conflict.
79-81: Correct—conversion arm supports both harnesses.Updating the feature gate to
#[cfg(any(feature = "pinocchio", feature = "solana"))]ensures theProgramErrorconversion arm is available for both Pinocchio and Solana builds, aligning with the variant definitions above.program-libs/compressed-account/Cargo.toml (7)
10-12: Well-structured no_std support with alloc tier.The new feature hierarchy (default → alloc → std) enables flexible compilation:
default = ["alloc"]: reasonable baseline supporting heap allocations without full stdalloc: enables allocator-dependent code vialight-hasher/allocstd: adds standard library support including borsh/std and solana-msgThis aligns with the PR's no_std refactoring goals and addresses the workspace-level
default-features = falseby explicitly defining feature tiers. Based on learnings.
14-14: Correct—anchor requires std.Adding
stdto the anchor feature is appropriate since Anchor programs run in a std environment and require standard library functionality.
27-27: Good—explicit feature control for light-zero-copy.Setting
default-features = falsewith explicitfeatures = ["alloc", "mut", "derive"]gives precise control over light-zero-copy's capabilities, aligning with the workspace-wide default-features strategy.
31-31: Making solana-msg optional supports no_std builds.Marking
solana-msgas optional and gating it behind thestdfeature (line 12) enables no_std compilation paths where logging may not be needed.
35-35: Borsh default-features = false aligns with no_std.Disabling borsh's default features is correct for no_std support. The
stdfeature on line 12 explicitly enablesborsh/stdwhen needed. Based on learnings: borsh 1.5.7 (current workspace version per line 129 of Cargo.toml at 0.10.4) supports no_std with optional std feature.
49-49: Self-dependency in dev-dependencies is appropriate.Adding
light-compressed-accountas a dev-dependency withfeatures = ["new-unique", "std"]is a common pattern for testing feature combinations and ensuring feature-gated code paths work correctly.
39-39: tinyvec usage is correct—noallocfeature needed.Verification confirms that only
ArrayVec(stack-only) is used in the codebase:
program-libs/compressed-account/src/compressed_account.rsline 259: Fixed-capacity stack vectorprogram-libs/compressed-account/src/instruction_data/traits.rsline 36-37: Generic ArrayVec return type
TinyVec(which requires theallocfeature) is not used. The current dependency configuration withoutallocfeature is appropriate for the stack-only usage pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
program-libs/compressed-account/src/instruction_data/invoke_cpi.rs (1)
15-22: Critical issue from previous review remains unfixed.The feature gate
all(feature = "std", feature = "anchor")on line 16 still breaks no_std+anchor builds. Whenanchoris enabled withoutstd(typical for BPF programs), the struct derives neither Anchor nor Borsh traits, causing compilation failure.This is especially problematic because lines 39-42 in the
data()method assumefeature = "anchor"enables Anchor traits, but the derives won't apply withoutstd.The fix from the previous review is still required: remove the
stdrequirement from the Anchor derive gate.program-libs/macros/src/lib.rs (1)
88-88: Fix: "at runtime" should be "at compile time".The macro performs PDA derivation during macro expansion (compile time), not at runtime. This was previously flagged but remains unaddressed.
Apply this diff:
-/// Derives a complete Light Protocol CPI configuration at runtime +/// Derives a complete Light Protocol CPI configuration at compile timeprogram-libs/hasher/tests/hash_to_field_size.rs (4)
1-11: Add missingark_bn254import and review unnecessary import.Line 14 references
ark_bn254::Fr::MODULUS, butark_bn254is not explicitly imported. Additionally,ark_ff::PrimeField(line 1) is imported but not directly used in this test file—it's only used withinis_smaller_than_bn254_field_size_be.Apply this diff:
-use ark_ff::PrimeField; +use ark_bn254; use borsh::{BorshDeserialize, BorshSerialize};
12-25: Add required feature gate to prevent compilation failure.This test uses
ark_bn254::Fr::MODULUSand callsis_smaller_than_bn254_field_size_be, which are only available when theposeidonfeature is enabled and not targeting Solana. Without the proper feature gate, this test will fail to compile on incompatible targets.Apply this diff:
+#[cfg(all(not(target_os = "solana"), feature = "poseidon"))] #[test] fn test_is_smaller_than_bn254_field_size_be() {
42-60: Add required feature gate for test compatibility.This test calls
is_smaller_than_bn254_field_size_be(lines 49, 57), which is only available undercfg(all(not(target_os = "solana"), feature = "poseidon")). The test also usessolana_pubkey::Pubkey, which should be available on non-Solana targets for testing. Gate the test to match the function's availability.Apply this diff:
+#[cfg(all(not(target_os = "solana"), feature = "poseidon"))] #[test] fn test_hash_to_bn254_field_size_be() {Based on learnings: gate SDK-specific implementations appropriately as done in program-libs/account-checks.
62-82: Add required feature gate for test compatibility.Same issue as
test_hash_to_bn254_field_size_be: this test callsis_smaller_than_bn254_field_size_be(lines 70, 79), which requirescfg(all(not(target_os = "solana"), feature = "poseidon")). Gate the test appropriately.Apply this diff:
+#[cfg(all(not(target_os = "solana"), feature = "poseidon"))] #[test] fn test_hashv_to_bn254_field_size_be() {program-libs/hasher/tests/data_hasher.rs (1)
62-139: Sizes 5–12 are never asserted; cover all sizes without duplication.This repeats an earlier warning; the current code silently skips 5–12. Replace the match with a const‑generic helper + macro to test 1..=12 uniformly.
#[test] fn test_data_hasher_array_multiple_sizes() { - // Test arrays of each implemented size - for size in 1..=12 { - let mut array = Vec::with_capacity(size); - for i in 0..size { - array.push(TestHashable::new(i as u8)); - } - - // Convert the Vec to an array of the appropriate size - let array_slice = array.as_slice(); - - // Create expected inputs (hashes of each TestHashable) - let mut expected_inputs = Vec::with_capacity(size); - for i in 0..size { - let mut hash = [0u8; 32]; - hash[31] = i as u8; - expected_inputs.push(hash); - } - - // Dynamically test each array size - match size { - 1 => { - let arr: [TestHashable; 1] = [array_slice[0].clone()]; - let hash_result = arr.hash::<Poseidon>().unwrap(); - - let expected_slices: Vec<&[u8]> = - expected_inputs.iter().map(|h| h.as_slice()).collect(); - let expected_hash = Poseidon::hashv(&expected_slices).unwrap(); - - assert_eq!(hash_result, expected_hash); - } - 2 => { - let arr: [TestHashable; 2] = [array_slice[0].clone(), array_slice[1].clone()]; - let hash_result = arr.hash::<Poseidon>().unwrap(); - - let expected_slices: Vec<&[u8]> = - expected_inputs.iter().map(|h| h.as_slice()).collect(); - let expected_hash = Poseidon::hashv(&expected_slices).unwrap(); - - assert_eq!(hash_result, expected_hash); - } - 3 => { - let arr: [TestHashable; 3] = [ - array_slice[0].clone(), - array_slice[1].clone(), - array_slice[2].clone(), - ]; - let hash_result = arr.hash::<Poseidon>().unwrap(); - - let expected_slices: Vec<&[u8]> = - expected_inputs.iter().map(|h| h.as_slice()).collect(); - let expected_hash = Poseidon::hashv(&expected_slices).unwrap(); - - assert_eq!(hash_result, expected_hash); - } - // We test one more size (4) to confirm the pattern works - 4 => { - let arr: [TestHashable; 4] = [ - array_slice[0].clone(), - array_slice[1].clone(), - array_slice[2].clone(), - array_slice[3].clone(), - ]; - let hash_result = arr.hash::<Poseidon>().unwrap(); - - let expected_slices: Vec<&[u8]> = - expected_inputs.iter().map(|h| h.as_slice()).collect(); - let expected_hash = Poseidon::hashv(&expected_slices).unwrap(); - - assert_eq!(hash_result, expected_hash); - } - _ => { - // For sizes 5-12, we've verified the pattern with tests for sizes 1-4 - // We could add more tests here if needed - } - } - } + // Helper that checks an exact array length using const generics. + fn check<const N: usize>() { + let arr: [TestHashable; N] = core::array::from_fn(|i| TestHashable::new(i as u8)); + let hash_result = arr.hash::<Poseidon>().unwrap(); + let expected_inputs: Vec<[u8; 32]> = (0..N) + .map(|i| { + let mut h = [0u8; 32]; + h[31] = i as u8; + h + }) + .collect(); + let expected_slices: Vec<&[u8]> = expected_inputs.iter().map(|h| h.as_slice()).collect(); + let expected_hash = Poseidon::hashv(&expected_slices).unwrap(); + assert_eq!(hash_result, expected_hash); + } + + macro_rules! for_sizes { + ($($n:expr),* $(,)?) => { $(check::<$n>();)* }; + } + for_sizes!(1,2,3,4,5,6,7,8,9,10,11,12); }#!/bin/bash set -euo pipefail echo "Checking MSRV / toolchain (array::from_fn requires Rust 1.63+):" rg -nP '^\s*rust-version\s*=\s*".+?"' -S -g 'Cargo.toml' -C1 || true fd -a 'rust-toolchain*' | while read -r f; do echo "== $f =="; cat "$f"; echo; done echo echo "Verify dev-deps enable poseidon (and std if required) for hasher tests:" if [ -f program-libs/hasher/Cargo.toml ]; then echo "== program-libs/hasher/Cargo.toml ==" rg -n -C2 '^\[dev-dependencies\]|\[dev-dependencies\..*\]|light-?hasher|features\s*=' program-libs/hasher/Cargo.toml || true else echo "program-libs/hasher/Cargo.toml not found; scanning workspace for hasher Cargo.toml..." fd -a Cargo.toml | while read -r f; do if rg -q '^\s*name\s*=\s*"light-?hasher"' "$f"; then echo "== $f =="; rg -n -C2 '^\[dev-dependencies\]|\[dev-dependencies\..*\]|light-?hasher|features\s*=' "$f" fi done fi
🧹 Nitpick comments (2)
program-libs/hasher/tests/data_hasher.rs (2)
31-37: Use hashv consistently (even for 1 element).Align the single‑element expectation with the multi‑element path to avoid relying on an implementation detail where
hashv([x]) == hash(x)may diverge for other hashers.- let expected_hash = Poseidon::hash(&expected_input).unwrap(); + let expected_hash = Poseidon::hashv(&[&expected_input]).unwrap();
46-55: Remove variable shadowing; initialize directly.Reduces noise and improves clarity.
- let hash1 = [0u8; 32]; - let hash2 = [0u8; 32]; - - let mut hash1 = hash1; - hash1[31] = 1; - - let mut hash2 = hash2; - hash2[31] = 2; + let mut hash1 = [0u8; 32]; + hash1[31] = 1; + let mut hash2 = [0u8; 32]; + hash2[31] = 2;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
program-libs/compressed-account/src/instruction_data/invoke_cpi.rs(2 hunks)program-libs/hasher/tests/data_hasher.rs(1 hunks)program-libs/hasher/tests/hash_chain.rs(1 hunks)program-libs/hasher/tests/hash_to_field_size.rs(1 hunks)program-libs/macros/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- program-libs/hasher/tests/hash_chain.rs
🧰 Additional context used
🧠 Learnings (6)
📚 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-libs/hasher/tests/hash_to_field_size.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 : Pass the data Vec<u8> through unchanged from the wrapper to the target program CPI; the target program performs deserialization.
Applied to files:
program-libs/compressed-account/src/instruction_data/invoke_cpi.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-libs/compressed-account/src/instruction_data/invoke_cpi.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:
program-libs/compressed-account/src/instruction_data/invoke_cpi.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-libs/compressed-account/src/instruction_data/invoke_cpi.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 : 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:
program-libs/macros/src/lib.rs
🧬 Code graph analysis (4)
program-libs/hasher/tests/hash_to_field_size.rs (2)
program-libs/hasher/src/bigint.rs (1)
bigint_to_be_bytes_array(23-36)program-libs/hasher/src/hash_to_field_size.rs (5)
hash_to_field_size(12-12)hash_to_field_size(20-39)hash_to_bn254_field_size_be(99-101)hashv_to_bn254_field_size_be(43-54)is_smaller_than_bn254_field_size_be(104-109)
program-libs/hasher/tests/data_hasher.rs (2)
program-libs/hasher/src/data_hasher.rs (1)
hash(4-4)program-libs/hasher/src/lib.rs (2)
hash(40-40)hashv(41-41)
program-libs/compressed-account/src/instruction_data/invoke_cpi.rs (2)
program-libs/compressed-account/src/instruction_data/traits.rs (3)
data(26-33)data(86-86)data(107-107)sdk-libs/sdk-pinocchio/src/cpi/v1/invoke.rs (1)
data(146-151)
program-libs/macros/src/lib.rs (1)
program-libs/macros/src/cpi_signer.rs (2)
derive_light_cpi_signer_pda(23-58)derive_light_cpi_signer(83-127)
⏰ 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-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- 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 (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- 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 (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: cli-v2
- GitHub Check: system-programs (sdk-token-test-program, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: Forester e2e test
- GitHub Check: cli-v1
- GitHub Check: lint
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
🔇 Additional comments (3)
program-libs/compressed-account/src/instruction_data/invoke_cpi.rs (1)
37-52: Implementation is sound once derive issue is resolved.The
data()method implementation correctly:
- Gates allocation behind the
allocfeature for no_std support- Uses conditional trait imports that align with the intended feature flags
- Adds a 4-byte length prefix (line 49) beyond the standard discriminator, which is appropriate for this instruction format
The trait aliasing pattern (lines 39-42) is unusual but works because both
AnchorSerializeandBorshSerializeprovidetry_to_vec()with identical signatures.However, this implementation will only compile correctly once the derive feature gate issue (lines 15-22) is fixed.
program-libs/macros/src/lib.rs (1)
61-66: LGTM: Clean no-op derive implementation.The
Noopderive macro is correctly implemented as a placeholder for conditional serialization. This pattern is useful for feature-gated derives.program-libs/hasher/tests/hash_to_field_size.rs (1)
27-40: Now let me query for the actual Cargo.toml and test file structure in the codebase:Based on my investigation of the code and Rust feature gate mechanics:
The test will fail to compile unless the "alloc" feature is enabled.
The
HashToFieldSizetrait implementation at line 16 ofhash_to_field_size.rsis gated with#[cfg(feature = "alloc")], meaning the predicate is true if the option is set, and false if it is unset. Since the test at lines 27-40 callstest_struct.hash_to_field_size(), which depends on this feature-gated implementation, the test code will not compile without that feature enabled.By default, tests, docs, and other tooling such as Clippy will only run with the default set of features. If "alloc" is not in the crate's default features list, this test will be silently excluded from compilation when running
cargo testwithout explicit feature flags.Verify whether "alloc" is included in the default features in
program-libs/hasher/Cargo.toml. If it is not, the test should either be gated with#[cfg(feature = "alloc")]or the feature should be added to defaults.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
program-libs/compressed-account/src/compressed_account.rs (1)
9-16: Feature gate condition is correct but contains a redundant clause.The
anchorfeature definition explicitly includesstd(anchor = ["anchor-lang", "std"]), makingall(feature = "std", feature = "anchor")redundant. The condition can be simplified to justfeature = "anchor"without changing behavior.The serialization strategy is sound: Anchor derives when
anchoris enabled, Borsh derives otherwise. There is no valid no_std + anchor scenario per the feature configuration.Recommendation: Simplify the condition to
feature = "anchor"across all occurrences (lines 9–16, 26–33, 53–60, 88–95, 133–140, 149–156, 164–171, 182–189, 199–206, 215–222, 233–240) for clarity, though the current code functions correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/rust.yml(0 hunks)program-libs/compressed-account/src/compressed_account.rs(11 hunks)program-libs/macros/src/lib.rs(2 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/rust.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- program-libs/macros/src/lib.rs
🧰 Additional context used
🧠 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/config.rs : Ensure serialization compatibility across Anchor, Pinocchio, and Borsh for core account types used by dependent programs
Applied to files:
program-libs/compressed-account/src/compressed_account.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-libs/compressed-account/src/compressed_account.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/packed_accounts.rs : For dynamic account sets, use PackedAccounts for index-based access with bounds checks instead of manual indexing
Applied to files:
program-libs/compressed-account/src/compressed_account.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:
program-libs/compressed-account/src/compressed_account.rs
🧬 Code graph analysis (1)
program-libs/compressed-account/src/compressed_account.rs (1)
program-libs/hasher/src/hash_to_field_size.rs (3)
hash_to_field_size(12-12)hash_to_field_size(20-39)hash_to_bn254_field_size_be(99-101)
⏰ 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-v2
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: cli-v1
- GitHub Check: system-programs (sdk-token-test-program, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: stateless-js-v2
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Forester e2e test
- GitHub Check: lint
- 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 (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- 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: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: stateless-js-v1
- 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...
🔇 Additional comments (5)
program-libs/compressed-account/src/compressed_account.rs (5)
330-330: LGTM: Scoped imports improve clarity.The explicit function-scoped imports of
hash_to_bn254_field_size_beare appropriate and improve code clarity by making the dependency explicit at the point of use.Also applies to: 370-370, 383-383
133-146: LGTM: Read-only account variants properly structured.The new
ReadOnlyCompressedAccountandPackedReadOnlyCompressedAccounttypes are well-defined:
- Feature-gated derives are consistent with other types
PackedReadOnlyCompressedAccountcorrectly uses#[repr(C)]andZeroCopyMutfor zero-copy semantics- Field layouts are appropriate for their use cases
Also applies to: 148-162
380-785: LGTM: Test coverage is comprehensive.The test module properly:
- Imports necessary serialization traits
- Tests hash computation with various field combinations
- Verifies equivalency between legacy and new implementations
- Includes reference values for regression testing
5-5: No action required—Vecis properly configured for no_std.The crate's prelude correctly handles
Vecwith conditional compilation: it re-exports fromallocwhenstdis disabled (line 7-8 of lib.rs) and fromstdwhen available (line 11-12). The import from the crate root in line 5 of compressed_account.rs follows this established pattern and is correct.
257-299: Based on my verification, the review comment contains inaccurate concerns. Let me check the workspace Cargo.toml to confirm the tinyvec configuration:Code changes are correct; no configuration issues or refactoring needed.
The ArrayVec usage with capacity 7 is appropriate for no_std support. tinyvec is no_std by default, and ArrayVec has no dependencies other than tinyvec and core, so the current workspace dependency configuration is correct.
The fixed capacity of 7 properly matches the maximum possible elements: 3 mandatory pushes (owner_hashed, leaf_index, merkle_tree) plus up to 4 conditional pushes (lamports, address, discriminator, data_hash). When ArrayVec capacity is exceeded, it causes a panic, but the code logic ensures this never occurs—all conditional pushes are properly guarded by their respective conditions (lines 280-296).
ananas-block
force-pushed
the
jorrit/refactor-compressed-account-nostd
branch
from
596d168 to
6551559
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
program-libs/compressed-account/src/compressed_account.rs (2)
112-131: into_read_only may leave prove_by_index stale when root_index is Some.You only set prove_by_index = true when root_index is None; when root_index is Some, the existing value is preserved, which could carry over an unintended true from prior state.
Make the intent explicit:
- let merkle_context = if root_index.is_none() { - let mut merkle_context = self.merkle_context; - merkle_context.prove_by_index = true; - merkle_context - } else { - self.merkle_context - }; + let mut merkle_context = self.merkle_context; + merkle_context.prove_by_index = root_index.is_none();This prevents stale values and documents the invariant.
248-299: Public hash_with_hashed_values accepts unsized inputs; enforce canonical lengths.As a public API, taking address: Option<&[u8]>, data: Option<(&[u8], &[u8])> permits non-32/8/32 byte inputs, producing non-canonical hashes if misused downstream. Given this is consensus/verification-critical, prefer typed arrays (and/or explicit validation).
Options:
- API hardening (breaking change):
-pub fn hash_with_hashed_values( - lamports: &u64, - address: Option<&[u8]>, - data: Option<(&[u8], &[u8])>, +pub fn hash_with_hashed_values( + lamports: &u64, + address: Option<&[u8; 32]>, + data: Option<(&[u8; 8], &[u8; 32])>,
- Backward-compatible guardrails: add debug assertions now and follow up with a typed API.
if let Some(address) = address { + debug_assert_eq!(address.len(), 32, "address must be 32 bytes"); vec.push(address); } @@ if let Some((discriminator, data_hash)) = data { + debug_assert_eq!(discriminator.len(), 8, "discriminator must be 8 bytes"); + debug_assert_eq!(data_hash.len(), 32, "data_hash must be 32 bytes");Additionally, to avoid future panics if inputs expand, prefer fallible pushes:
- let mut vec: tinyvec::ArrayVec<[&[u8]; 7]> = tinyvec::ArrayVec::new(); + let mut vec: tinyvec::ArrayVec<[&[u8]; 7]> = tinyvec::ArrayVec::new(); + // Invariant: at most 7 elements; use try_push to avoid panics if modified later.and replace push with try_push().map_err(|_| CompressedAccountError::InternalCapacityExceeded)? (or similar error variant).
These tighten invariants around canonical hashing. Based on learnings.
♻️ Duplicate comments (1)
program-libs/macros/src/lib.rs (1)
88-88: Fix: "at runtime" should be "at compile time".The macro performs PDA derivation during macro expansion (compile time), not at runtime. The generated code contains precomputed byte arrays, as evidenced by the implementation in
cpi_signer.rsand the example showingconstinitialization.Apply this diff:
-/// Derives a complete Light Protocol CPI configuration at runtime +/// Derives a complete Light Protocol CPI configuration at compile time
🧹 Nitpick comments (3)
program-libs/compressed-account/src/compressed_account.rs (3)
9-17: Feature-gating gap: Anchor derives disabled when anchor=true and std=false.Current cfg_attr applies Anchor derives only with all(feature="std", feature="anchor"), and Borsh derives only with not(feature="anchor"). When anchor=true but std=false, neither path applies, dropping (de)serialization derives and potentially breaking no_std anchor builds or consumers.
Two options:
- If Anchor is intentionally disallowed without std: enforce that explicitly once per crate with a compile_error! to avoid silent misconfigurations.
- If Anchor should work in no_std: drop the std requirement or add an alternative cfg path.
Suggested guard (crate-level or once in this module):
#[cfg(all(feature = "anchor", not(feature = "std")))] compile_error!("`feature=\"anchor\"` requires `feature=\"std\"` in this crate. Enable `std` or disable `anchor`.");Or relax the gating:
#[cfg_attr(feature = "anchor", derive(anchor_lang::AnchorDeserialize, anchor_lang::AnchorSerialize))] #[cfg_attr(not(feature = "anchor"), derive(borsh::BorshDeserialize, borsh::BorshSerialize))]Please confirm the intended support matrix and apply consistently across all structs. Based on learnings.
Also applies to: 26-34, 53-61, 88-96, 133-141, 149-157, 164-172, 182-191, 199-207, 215-223, 233-241
324-339: Avoid pattern-binding on references in API; pass references directly.
&merkle_tree_pubkey: &Pubkeycopies the value unnecessarily and is unconventional. Prefer a plain reference for clarity and fewer moves:- pub fn hash( - &self, - &merkle_tree_pubkey: &Pubkey, + pub fn hash( + &self, + merkle_tree_pubkey: &Pubkey,Use
merkle_tree_pubkey.as_ref()as before. Same for leaf_index if desired.
364-377: Same param-pattern simplification for ZCompressedAccount::hash.Mirror the simplification:
- pub fn hash( - &self, - &merkle_tree_pubkey: &[u8; 32], + pub fn hash( + &self, + merkle_tree_pubkey: &[u8; 32],Then use
merkle_tree_pubkey.as_slice().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/rust.yml(0 hunks)program-libs/batched-merkle-tree/Cargo.toml(1 hunks)program-libs/compressed-account/src/compressed_account.rs(11 hunks)program-libs/macros/src/lib.rs(2 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/rust.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- program-libs/batched-merkle-tree/Cargo.toml
🧰 Additional context used
🧠 Learnings (5)
📚 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:
program-libs/macros/src/lib.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-libs/compressed-account/src/compressed_account.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-libs/compressed-account/src/compressed_account.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/packed_accounts.rs : For dynamic account sets, use PackedAccounts for index-based access with bounds checks instead of manual indexing
Applied to files:
program-libs/compressed-account/src/compressed_account.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:
program-libs/compressed-account/src/compressed_account.rs
🧬 Code graph analysis (2)
program-libs/macros/src/lib.rs (4)
sdk-libs/sdk-types/src/cpi_accounts/config.rs (2)
cpi_signer(30-32)new(12-19)sdk-libs/sdk-types/src/cpi_accounts/v2.rs (1)
new(71-77)sdk-libs/sdk-types/src/cpi_accounts/v1.rs (1)
new(53-59)program-libs/macros/src/cpi_signer.rs (2)
derive_light_cpi_signer_pda(23-58)derive_light_cpi_signer(83-127)
program-libs/compressed-account/src/compressed_account.rs (1)
program-libs/hasher/src/hash_to_field_size.rs (3)
hash_to_field_size(12-12)hash_to_field_size(20-39)hash_to_bn254_field_size_be(99-101)
⏰ 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: Forester e2e 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 (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- 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 (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: system-programs (sdk-token-test-program, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: cli-v2
- GitHub Check: lint
- GitHub Check: stateless-js-v2
🔇 Additional comments (5)
program-libs/macros/src/lib.rs (3)
7-7: LGTM: Internal module for CPI signer macro logic.The module declaration appropriately encapsulates the compile-time PDA derivation logic used by the new macros.
61-66: LGTM: Clean pattern for conditional serialization derives.The no-op derive macro enables feature-gated compilation without syntax errors when serialization derives are not needed. This aligns well with the PR's no_std refactoring objectives.
68-86: LGTM: Compile-time PDA derivation macro.The macro correctly documents that PDA computation happens at compile time, and the delegation to the internal module is appropriate. The documentation and example clearly describe the return type and usage.
program-libs/compressed-account/src/compressed_account.rs (2)
133-147: New read-only account types look consistent with mutable/packed counterparts.Layouts and derives align; ZeroCopyMut on packed variant with repr(C) is correct. LGTM.
Also applies to: 148-162
380-784: Strong test coverage for hashing variants and legacy parity.The tests clearly validate domain separation, endianness toggle (is_batched), ZC path, and legacy equivalence. Thanks for the thoroughness. LGTM.
ananas-block
force-pushed
the
jorrit/refactor-compressed-account-nostd
branch
from
6551559 to
00f3ced
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/rust.yml(0 hunks)program-libs/batched-merkle-tree/Cargo.toml(1 hunks)program-libs/compressed-account/src/compressed_account.rs(11 hunks)program-libs/concurrent-merkle-tree/Cargo.toml(1 hunks)program-libs/hash-set/Cargo.toml(1 hunks)program-libs/indexed-array/Cargo.toml(1 hunks)program-libs/indexed-merkle-tree/Cargo.toml(1 hunks)program-libs/macros/src/lib.rs(2 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/rust.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- program-libs/batched-merkle-tree/Cargo.toml
🧰 Additional context used
🧠 Learnings (5)
📚 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-libs/compressed-account/src/compressed_account.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-libs/compressed-account/src/compressed_account.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/packed_accounts.rs : For dynamic account sets, use PackedAccounts for index-based access with bounds checks instead of manual indexing
Applied to files:
program-libs/compressed-account/src/compressed_account.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:
program-libs/compressed-account/src/compressed_account.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 : 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:
program-libs/macros/src/lib.rs
🧬 Code graph analysis (2)
program-libs/compressed-account/src/compressed_account.rs (1)
program-libs/hasher/src/hash_to_field_size.rs (3)
hash_to_field_size(12-12)hash_to_field_size(20-39)hash_to_bn254_field_size_be(99-101)
program-libs/macros/src/lib.rs (1)
program-libs/macros/src/cpi_signer.rs (2)
derive_light_cpi_signer_pda(23-58)derive_light_cpi_signer(83-127)
⏰ 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). (22)
- GitHub Check: stateless-js-v2
- GitHub Check: lint
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v1
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: cli-v2
- GitHub Check: Forester e2e test
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: system-programs (sdk-token-test-program, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- 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 (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: cli-v1
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
program-libs/indexed-merkle-tree/Cargo.toml (1)
37-37: Expand dev-dependency hashing algorithms for comprehensive testing.Adding
light-hasheras a dev-dependency with extended features ["keccak", "sha256", "poseidon"] is a solid pattern. This allows tests and examples to validate multiple hashing algorithms while the production code remains minimal (poseidon-only on line 25).Both dependencies correctly use
{ workspace = true }, ensuring version consistency across the workspace.Please verify that tests/examples in this crate actually exercise the additional
keccakandsha256features. If they're unused, consider removing them to keep dev-dependencies lean.program-libs/hash-set/Cargo.toml (1)
17-17: Verify that main dependencylight-hasher(line 17) has sufficient baseline functionality without explicit features.The dev-dependency now explicitly enables
["keccak", "sha256", "poseidon"]features (line 22), while the main dependency relies only on workspace defaults (line 17). For no_std/alloc refactors with feature-gating, this pattern is sound: the library compiles minimally, and tests gain access to specific algorithms.However, ensure that the code path in the library itself does not require at least one of these hash algorithms at compile time or runtime when features are absent (e.g., as a fallback). If the library does need a baseline hasher, consider conditionally enabling one feature in the main dependency, or documenting the requirement clearly.
To verify this pattern is safe, confirm:
- Whether any code in
program-libs/hash-setunconditionally calls functions from light-hasher that depend on these features.- Whether light-hasher's workspace default (now disabled per the learnings) previously provided a default algorithm that is now required by tests only.
Consider running the library's doctests and unit tests without the dev-dependency to ensure no breakage occurs when building the library in isolation.
Also applies to: 22-22
program-libs/concurrent-merkle-tree/Cargo.toml (1)
28-28: Verified: all hash function features are valid.The dev-dependency correctly expands the hash function features for comprehensive testing. All three features—keccak, sha256, and poseidon—are properly defined in light-hasher and available for use. This follows best practice: keeping the runtime dependency minimal (poseidon only) while enabling full test coverage with multiple hash algorithms in dev-dependencies.
program-libs/indexed-array/Cargo.toml (1)
16-16: Features align with workspace pattern; verify consistency rationale with maintainers.The expanded dev-dependency features (
keccak,sha256,poseidon) match the pattern used across related crates (indexed-merkle-tree,hash-set,concurrent-merkle-tree). However, indexed-array's test suite does not directly reference these hashers. Confirm whether this is:
- Preparatory for future feature-gated tests
- Required for transitive test dependencies
- Maintained for consistency across similar library crates
If this is intentional consistency, no action needed. If not, consider whether dev-dependencies should be trimmed to only required features.
program-libs/macros/src/lib.rs (2)
61-66: LGTM: Clean no-op derive placeholder.The implementation correctly returns an empty
TokenStream, and the documentation clearly explains its intended use as a serialization derive placeholder.
68-86: LGTM: Documentation and delegation are correct.The macro correctly documents compile-time PDA derivation, and the example matches the implementation's tuple return type.
program-libs/compressed-account/src/compressed_account.rs (4)
257-299: LGTM: Excellent no_std optimization with ArrayVec.The replacement of
Vecwithtinyvec::ArrayVec<[&[u8]; 7]>is well-suited for no_std environments. The capacity of 7 correctly accounts for all possible elements (3 mandatory + 4 optional), avoiding heap allocation in the typical case while maintaining the same hashing logic.
330-330: LGTM: Function-scoped imports improve clarity.The explicit
usestatements at function scope make dependencies clear and are consistent with no_std best practices.Also applies to: 370-370
382-383: LGTM: Test imports align with Borsh serialization path.The change to
borsh::BorshSerializeis consistent with tests using the non-Anchor feature configuration, properly exercising the Borsh serialization path.
9-16: ****The feature flag concern is unfounded. The
Cargo.tomlexplicitly definesanchor = ["anchor-lang", "std"], meaning theanchorfeature automatically enablesstd. There is no "anchor without std" scenario in this codebase—the conditionall(feature = "std", feature = "anchor")will always be true whenanchoris enabled.The current conditional derives pattern is correct:
- When
anchoris enabled:stdis also enabled (by feature definition) → Anchor derives applied ✓- When
anchoris disabled: Borsh derives applied ✓No changes to lines 9-16 or related derives are needed.
Likely an incorrect or invalid review comment.
| cpi_signer::derive_light_cpi_signer_pda(input) | ||
| } | ||
|
|
||
| /// Derives a complete Light Protocol CPI configuration at runtime |
There was a problem hiding this comment.
Fix: "at runtime" should be "at compile time".
The macro performs PDA derivation during macro expansion (compile time), not at runtime. The implementation precomputes all byte arrays and the bump seed, as evidenced by the const usage in the example at line 100.
Apply this diff:
-/// Derives a complete Light Protocol CPI configuration at runtime
+/// Derives a complete Light Protocol CPI configuration at compile time📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Derives a complete Light Protocol CPI configuration at runtime | |
| /// Derives a complete Light Protocol CPI configuration at compile time |
🤖 Prompt for AI Agents
In program-libs/macros/src/lib.rs around line 88, the doc comment mistakenly
says "at runtime" but the macro actually performs PDA derivation during macro
expansion (compile time); update the comment text to "at compile time" (and
adjust any adjacent wording to reflect compile-time behavior) so the
documentation correctly states that derivation and precomputation occur during
macro expansion.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Cargo.toml (1)
172-172: Duplicate: default-features = false compilation risk already flagged.This change was previously reviewed and flagged as a critical issue. The concern is that workspace members using
{ workspace = true }will inheritdefault-features = false, potentially causing compilation failures if they don't explicitly enable required features.See the previous review comment on lines 172-184 that identified 5 workspace members (program-libs/bloom-filter, program-libs/compressible, program-libs/hash-set, program-tests/merkle-tree, sdk-libs/sdk-types) depending on these crates without explicit feature enablement.
The fix requires either:
- Removing/clearing
default = [...]in the affected crates' Cargo.toml files, OR- Updating all workspace dependents to explicitly enable required features
Also applies to: 180-181, 184-184, 186-186
🧹 Nitpick comments (2)
sdk-libs/sdk-pinocchio/Cargo.toml (1)
27-27: Consider disabling default features for optional light-sdk.The
light-sdkdependency is marked as optional but doesn't specifydefault-features = false, unlike other light-* dependencies in this file (lines 19-23). This inconsistency could inadvertently enable std features when thelight-accountfeature is active.Apply this diff to align with the no_std pattern:
-light-sdk = { workspace = true, optional = true } +light-sdk = { workspace = true, default-features = false, optional = true }program-tests/pinocchio-nostd-test/src/lib.rs (1)
28-37: panic! in no_std TryFrom implementation.Line 34 uses
panic!for invalid discriminators, which can increase program size and may not be the most efficient error handling in a no_std Solana program. Pinocchio and Solana programs typically prefer returning errors.Consider returning an error instead of panicking:
fn try_from(value: u8) -> Result<Self, Self::Error> { match value { 0 => Ok(InstructionType::TestBasic), - _ => panic!("Invalid instruction discriminator."), + _ => Err(LightSdkError::InvalidDiscriminator), } }Note: This requires
LightSdkErrorto have anInvalidDiscriminatorvariant. Verify this variant exists or add it.
📜 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 (7)
.github/workflows/sdk-tests.yml(1 hunks)Cargo.toml(4 hunks)program-tests/pinocchio-nostd-test/Cargo.toml(1 hunks)program-tests/pinocchio-nostd-test/build.rs(1 hunks)program-tests/pinocchio-nostd-test/src/lib.rs(1 hunks)program-tests/pinocchio-nostd-test/tests/basic.rs(1 hunks)sdk-libs/sdk-pinocchio/Cargo.toml(1 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/**/Cargo.toml : Define features solana, pinocchio, and test-only in Cargo.toml; default build should enable none
Applied to files:
program-tests/pinocchio-nostd-test/build.rsprogram-tests/pinocchio-nostd-test/Cargo.tomlsdk-libs/sdk-pinocchio/Cargo.tomlprogram-tests/pinocchio-nostd-test/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/{solana.rs,pinocchio.rs,test_account_info.rs} : Gate SDK-specific implementations with #[cfg(feature = "solana"|"pinocchio"|"test-only")]
Applied to files:
program-tests/pinocchio-nostd-test/Cargo.tomlsdk-libs/sdk-pinocchio/Cargo.tomlprogram-tests/pinocchio-nostd-test/src/lib.rs
📚 Learning: 2025-10-15T03:46:43.242Z
Learnt from: CR
PR: Lightprotocol/light-protocol#0
File: sdk-tests/sdk-token-test/CLAUDE.md:0-0
Timestamp: 2025-10-15T03:46:43.242Z
Learning: Run tests with: cargo test-sbf -p sdk-token-test --test <test-file-name>
Applied to files:
.github/workflows/sdk-tests.yml
📚 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:
Cargo.toml
🧬 Code graph analysis (1)
program-tests/pinocchio-nostd-test/tests/basic.rs (1)
program-tests/pinocchio-nostd-test/src/lib.rs (1)
get_program_id(71-73)
⏰ 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 (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: 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: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v2
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: Test program-libs-slow
- 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 (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
- GitHub Check: lint
- GitHub Check: Forester e2e test
🔇 Additional comments (15)
.github/workflows/sdk-tests.yml (2)
50-55: Matrix reorganization aligns well with PR objectives.The consolidation of CI matrix entries—renaming generic labels and grouping anchor & pinocchio tests—is a clean reorganization. The addition of
pinocchio-nostd-test(line 53) directly supports the no_std refactoring goal outlined in the PR.
51-51: Verify command format consistency:cargo test-sbfvscargo-test-sbf.Line 51 contains
"cargo test-sbf -p client-test"(with a space), whereas other similar entries usecargo-test-sbf(hyphenated, e.g., line 53). Based on the learnings provided ("Run tests with:cargo test-sbf -p ..."), the correct form likely uses a hyphen. However, this line is not marked as changed in the diff, so it may be pre-existing. If this is indeed an error, the shell eval on line 90 will fail when executing this command.Please verify that the command format in line 51 is correct and will execute successfully. If
cargo test-sbfshould becargo-test-sbf, apply this fix:- sub-tests: '["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-test", "cargo test-sbf -p client-test"]' + sub-tests: '["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-test", "cargo-test-sbf -p client-test"]'program-tests/pinocchio-nostd-test/build.rs (1)
14-15: LGTM: strict_nostd cfg flag emission.Emitting the
strict_nostdcfg flag allows downstream code to conditionally compile based on this no_std enforcement mode.Cargo.toml (3)
27-27: LGTM: New workspace members added.The addition of
sdk-libs/eventandprogram-tests/pinocchio-nostd-testto the workspace aligns with the PR objectives to support no_std refactoring and event handling.Also applies to: 48-48
126-127: LGTM: Anchor version normalization.Removing the
=prefix from anchor-lang and anchor-spl versions allows for compatible version updates while maintaining the 0.31.1 constraint. This is a more flexible versioning approach than exact pinning.
130-130: LGTM: Borsh no_std compatibility.Setting
default-features = falsefor borsh enables no_std builds across the workspace. Borsh 0.10.4 supports no_std, so this change is safe.program-tests/pinocchio-nostd-test/src/lib.rs (3)
1-6: LGTM: Proper no_std configuration.The crate-level attributes correctly establish a no_std environment:
#![no_std]disables the standard library#![cfg_attr(target_os = "solana", forbid(unsafe_code))]enforces safety on Solana#![cfg_attr(not(test), no_main)]removes the main function requirement for library builds
54-65: LGTM: Effective compile-time std detection.These compile-time guards (lines 55-56 and 63-64) will cause compilation to fail if the
stdfeature is somehow enabled for Solana targets. This is the effective defense mechanism, unlike the build script check.
67-78: LGTM: Test helpers properly gated.The test helpers module is correctly gated with
#[cfg(not(target_os = "solana"))], ensuring it's only available in test environments and not compiled into the Solana binary.program-tests/pinocchio-nostd-test/tests/basic.rs (1)
1-33: LGTM: Well-structured no_std test.The test properly exercises the no_std program:
- Correctly gated with
#[cfg(feature = "test-sbf")]- Uses test helpers to access program ID
- Creates a minimal transaction with the TestBasic instruction (discriminator 0)
- Verifies successful execution
The test effectively validates that the no_std program compiles and executes correctly in a Solana environment.
program-tests/pinocchio-nostd-test/Cargo.toml (4)
14-17: LGTM: Correct no_std feature configuration.Setting
default = [](line 17) ensures no features are enabled by default, which is correct for a no_std test crate. Theno-entrypointandtest-sbffeatures are appropriately defined for testing scenarios.
19-28: LGTM: Dependencies properly configured for no_std.All dependencies use
{ workspace = true }and rely on workspace-level default-features = false configuration. The absence of poseidon in light-hasher (line 26) is intentional to avoid std dependencies, as documented in the comment.The v2 features on light-sdk-pinocchio and light-sdk-types (lines 23-24) are correctly enabled.
30-36: LGTM: Dev dependencies can safely use std.Dev dependencies (lines 31-36) correctly enable std-dependent features like
solanaandposeidonsince tests run in a std environment. Thedevenvfeature on light-program-test (line 31) is appropriate for test infrastructure.
38-43: LGTM: Lints allow necessary cfg patterns.The
unexpected_cfgsconfiguration correctly allows:
target_os = "solana"for Solana-specific conditional compilation- Features
frozen-abiandno-entrypointthat may be checked but not always definedThis prevents spurious warnings during development.
sdk-libs/sdk-pinocchio/Cargo.toml (1)
10-15: Verified:light-compressed-accountexports theallocfeature; feature configuration is correct.The forwarding of
light-compressed-account/allocon line 11 is valid and properly enables no_std + alloc support by default. The feature hierarchy (std → alloc) and optional feature delegation (poseidon, light-account) are correctly structured.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests