fix: conflicting pinocchio & solana crates compilation errors #1795
Conversation
ananas-block
force-pushed
the
jorrit/chore-improve-crate-feature-robustness
branch
from
15cdd80 to
fd19acc
Compare
ananas-block
requested review from
SwenSchaeferjohann and
sergeytimoshin
as code owners
ananas-block
force-pushed
the
jorrit/chore-improve-crate-feature-robustness
branch
from
fd19acc to
3afda7b
Compare
|
This pr should be merged with rebase and merge intead of squash in merge to persist the commits! |
ananas-block
force-pushed
the
jorrit/chore-improve-crate-feature-robustness
branch
from
3afda7b to
7f94448
Compare
|
consider to move |
| /// Checks that the account balance is greater or eqal to rent exemption. | ||
| pub fn check_account_balance_is_rent_exempt( | ||
| account_info: &AccountInfo, | ||
| #[cfg(any(feature = "pinocchio", feature = "solana"))] |
There was a problem hiding this comment.
any reason why we use conditional compilation explicitly here but not for the other functions?
There was a problem hiding this comment.
good catch its a letftover
| use pinocchio::sysvars::{clock::Clock, Sysvar}; | ||
| self.rolledover_slot = Clock::get().unwrap().slot; | ||
| } | ||
| #[cfg(not(feature = "pinocchio"))] |
There was a problem hiding this comment.
same as?
| #[cfg(not(feature = "pinocchio"))] | |
| #[cfg(feature = "solana")] |
There was a problem hiding this comment.
in this case we need this condition else if we compile with all features we get an import conflict because we are importing this twice `use pinocchio::sysvars::{clock::Clock, Sysvar};' and we only need it once.
| (pubkey, bump) | ||
| } | ||
| // Pinocchio does not support find_program_address outside of target_os solana. | ||
| // That is annoying for rust unit tests. |
There was a problem hiding this comment.
| // That is annoying for rust unit tests. |
WalkthroughThis update introduces a comprehensive abstraction layer for account information handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AccountInfo (Solana/Pinocchio)
participant AccountInfoTrait
participant ValidationFn
participant ErrorType
User->>AccountInfo: Provides account info (backend-specific)
AccountInfo->>AccountInfoTrait: Uses trait interface
AccountInfoTrait->>ValidationFn: Calls validation/check function
ValidationFn->>AccountInfoTrait: Accesses metadata via trait methods
ValidationFn->>ErrorType: Returns unified error on failure
ValidationFn-->>User: Returns success or error
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (9)
programs/system/src/lib.rs (1)
24-34:⚠️ Potential issue
pubkey_array!returns[u8; 32]– currentIDdefinition does not compile
pub const ID: Pubkey = pubkey_array!(...)assigns a plain byte-array to aPubkey.
pinocchio::pubkey::Pubkeydoes not have aconstFrom<[u8;32]>impl, so this will fail at compile-time.- pub const ID: Pubkey = pubkey_array!("SySTEM1eSU2p4BGQfQpimFEWWSC1XDFeun3Nqzz3rT7"); + pub const ID: Pubkey = + Pubkey::new_from_array(pubkey_array!("SySTEM1eSU2p4BGQfQpimFEWWSC1XDFeun3Nqzz3rT7"));This keeps
IDaPubkey, matches the rest of the program API, and remains aconst.
Without this fix the crate will not build.program-tests/sdk-test/src/create_pda.rs (1)
40-46:⚠️ Potential issue
unwrap()in on-chain context can abort the programUsing
unwrap()will panic ifhashv_to_bn254_field_size_be_const_arrayever returns an error, immediately aborting the transaction.
Propagate the error instead:- let address_seed = hashv_to_bn254_field_size_be_const_array::<3>(&[ - b"compressed", - instruction_data.data.as_slice(), - ]) - .unwrap(); + let address_seed = hashv_to_bn254_field_size_be_const_array::<3>(&[ + b"compressed", + instruction_data.data.as_slice(), + ]) + .map_err(|_| LightSdkError::HashToFieldSize)?;This keeps the program fail-safe and avoids unintended panics.
program-libs/compressed-account/src/indexer_event/parse.rs (1)
633-640: 🛠️ Refactor suggestionQuadratic walk over accounts – switch to a HashMap lookup
For every sequence number we iterate over every input account, leading to
O(n*m)behaviour.
Batch transactions with hundreds of inputs/outputs will pay for this-again CPU-wise in the indexer.- internal_input_sequence_numbers.iter_mut().for_each(|seq| { - for (i, merkle_tree_pubkey) in input_merkle_tree_pubkeys.iter().enumerate() { - if *merkle_tree_pubkey == seq.tree_pubkey { - nullifier_queue_indices[i] = seq.seq.into(); - seq.seq += 1; - } - } - }); + let mut map: std::collections::HashMap<Pubkey, &mut u64> = + internal_input_sequence_numbers.iter_mut().map(|s| (s.tree_pubkey, &mut s.seq)).collect(); + for (idx, tree) in input_merkle_tree_pubkeys.iter().enumerate() { + if let Some(seq) = map.get_mut(tree) { + nullifier_queue_indices[idx] = (**seq).into(); + **seq += 1; + } + }Same applies to
create_address_queue_indices.This drops the complexity to linear time and allocates only a small additional map.
program-tests/system-cpi-test/src/create_pda.rs (1)
492-506:⚠️ Potential issue
unwrap()will abort the on-chain program – propagate the error insteadBoth the Poseidon hash and legacy address derivation can fail;
unwrap()converts that into an unrecoverable panic which will consume all compute units and return a generic error to the caller.- data_hash: timelock_compressed_pda.hash::<Poseidon>().unwrap(), + data_hash: timelock_compressed_pda + .hash::<Poseidon>() + .map_err(|e| anchor_lang::error::Error::from(e))?, @@ - ) - .unwrap(), + )?,Define a dedicated
ErrorCode::HashFailed/ErrorCode::AddressDerivationFailedif you need something more descriptive.Failing gracefully keeps the program deterministic and makes debugging far easier.
program-libs/merkle-tree-metadata/src/rollover.rs (1)
105-118:⚠️ Potential issueUnchecked
powmay overflow for tall trees
(rollover_fee * rollover_threshold * (2u64.pow(height)))silently wraps onu64overflow (e.g.height >= 64).
Consideru128math orchecked_pow/checked_multo detect impossible configurations instead of producing undefined economics.forester/tests/batched_state_async_indexer_test.rs (1)
857-862:⚠️ Potential issue
tree_typehard-coded toStateV2even in the V1 branchInside
transfer::<false,…>themerkle_contextsyou build still usetree_type: TreeType::StateV2,leading to inconsistent proofs and root look-ups for V1 trees.
- tree_type: TreeType::StateV2, + tree_type: if V2 { TreeType::StateV2 } else { TreeType::StateV1 },Failing to fix this will surface as “root not found” or mismatched-hash errors once V1 accounts are processed.
program-libs/verifier/src/lib.rs (1)
42-47:⚠️ Potential issueUse correct Solana
ProgramErrorpath
solana_program_error::ProgramErroris not re-exported by the canonical Solana SDK; the type lives undersolana_program::program_error::ProgramError.
Unless you have a bespoke crate namedsolana_program_error, this will not compile once thesolanafeature is enabled.-impl From<VerifierError> for solana_program_error::ProgramError { +use solana_program::program_error::ProgramError; + +impl From<VerifierError> for ProgramError { fn from(e: VerifierError) -> Self { - solana_program_error::ProgramError::Custom(e.into()) + ProgramError::Custom(e.into()) } }programs/compressed-token/src/process_transfer.rs (2)
190-201: 🛠️ Refactor suggestionGeneric slice of
impl AsPubkeyis brittleUsing
&[impl AsPubkey]forpubkeys/amountscreates a monomorphic slice tied to one concrete type.
If the caller mixes differentAsPubkeyimpls (e.g.Pubkeyand[u8;32]) the code won’t compile, defeating the purpose of this abstraction.-pub fn create_output_compressed_accounts( - output_compressed_accounts: &mut [OutputCompressedAccountWithPackedContext], - mint_pubkey: impl AsPubkey, - pubkeys: &[impl AsPubkey], +pub fn create_output_compressed_accounts<P, O>( + output_compressed_accounts: &mut [OutputCompressedAccountWithPackedContext], + mint_pubkey: P, + pubkeys: &[O], ) -> Result<u64> where + P: AsPubkey, + O: AsPubkey,A regular generic parameter keeps the API flexible and avoids monomorph-code bloat when the same function is called with many concrete types.
150-165:⚠️ Potential issueUnsafe overflow on lamport change calculation
change_lamports = input_lamports - output_lamports;can underflow if the outputs exceed the inputs, causing a panic in release builds (checked subtraction is removed).
Usechecked_suband return a domain-specific error instead:- let change_lamports = input_lamports - output_lamports; + let change_lamports = input_lamports + .checked_sub(output_lamports) + .ok_or(ErrorCode::ComputeOutputLamportsFailed)?;
♻️ Duplicate comments (2)
program-libs/batched-merkle-tree/src/batch.rs (2)
188-192: Same logging duplication comment as aboveWhen you refactor, remember to update this block to the wrapper macro too.
204-208: Third occurrence – candidate for macro consolidation
🧹 Nitpick comments (59)
program-libs/hasher/src/hash_to_field_size.rs (1)
142-176: Gating tests onsolanalooks good, but 10 000 iterations is overkillSwitching from
#[cfg(not(feature = "pinocchio"))]to#[cfg(feature = "solana")]removes the mutual-exclusion problem – nice.Minor nit: each test still hashes 10 000 random keys. On CI this costs ~200 ms per test on a fast machine; multiplied across crates it adds up. Consider trimming to 1 000 unless you specifically need that statistical confidence.
- for _ in 0..10_000 { + for _ in 0..1_000 {program-libs/account-checks/Cargo.toml (1)
32-36: Dev-dependencies force Solana + Pinocchio even when features are off
pinocchioandsolana-pubkeyare unconditional in[dev-dependencies], socargo testwill always compile both backends, defeating the purpose of mutually-exclusive features during CI size checks.
Consider gating them:[dev-dependencies] pinocchio = { workspace = true, optional = true } solana-pubkey = { workspace = true, optional = true, features = ["curve25519","sha2"] } [features] test-solana = ["dep:solana-pubkey"] test-pinocchio = ["dep:pinocchio"]This keeps tests flexible without imposing both heavy deps every run.
examples/anchor/token-escrow/Cargo.toml (1)
31-34: Duplicatelight-test-utilsentries may confuse Cargo feature resolutionThe crate is declared both in the target-specific section and again in
[dev-dependencies](lines 38–39). Cargo merges them, but the duplicated specification can become a maintenance foot-gun when versions/features diverge.If both clauses need identical features you can drop the
[dev-dependencies]entry – tests still inherit the target-specific dependency.programs/account-compression/src/instructions/resize_registered_program_account.rs (1)
42-46: Avoid extra allocation by borrowing the program ID bytes directly
Pubkey::to_bytes()allocates a new[u8;32]each call.
Pubkeyalready implementsAsRef<[u8]>, so you can pass a borrowed slice without copying:- light_account_checks::checks::check_owner( - &crate::ID.to_bytes(), - &ctx.accounts.registered_program_pda, - ) + light_account_checks::checks::check_owner( + crate::ID.as_ref(), + &ctx.accounts.registered_program_pda, + )This is cheaper and reads a bit cleaner.
programs/compressed-token/src/freeze.rs (2)
185-189: Minor perf / clarity nit – avoid repeated.into()inside the loop
crate::ID.into()is executed for every iteration. Cache once:- output_compressed_accounts[i] = OutputCompressedAccountWithPackedContext { - compressed_account: CompressedAccount { - owner: crate::ID.into(), + let program_owner: [u8; 32] = crate::ID.into(); + output_compressed_accounts[i] = OutputCompressedAccountWithPackedContext { + compressed_account: CompressedAccount { + owner: program_owner,Pure micro-optimisation; feel free to ignore if size-optimised build is not a concern.
524-527: Same.into()duplication in test helper – consider applying the same caching trick here to keep test code consistent.program-tests/create-address-test-program/src/create_pda.rs (1)
118-120: Redundant conversion
crate::IDis already the program id. Ifownerexpects[u8;32], convert once outside the struct literal to improve readability (same suggestion as infreeze.rs).examples/anchor/counter/tests/test.rs (1)
52-68: Heavy repetition – consider a helper to fetch & decode the counterEvery verification block repeats the same three steps:
get_compressed_account → unwrap → CounterAccount::deserialize.
Extracting this into a small helper (e.g.fetch_counter_value(&rpc, address) -> Result<(CompressedAccount,u64)>) would:
- remove ~25 duplicated lines,
- consolidate the
unwrap()panic into a single?,- make the intent of each test section clearer.
Purely optional but improves readability/maintenance.
Also applies to: 74-86, 88-104
programs/system/src/context.rs (1)
358-365: Minor: avoid extra copy by matching input branchSee comment above – align the
ownerassignment once the expected return type is confirmed.program-tests/utils/src/assert_token_tx.rs (1)
167-172: Prefer comparingPubkeys directly for stronger type-safetyBoth operands are converted to raw bytes before the equality check.
Instead, keep the comparison at thePubkeylevel and convert the constant once.
This avoids repeated allocations/copies and keeps the compiler on your side if either side’s type ever changes again.-assert_eq!( - transfer_recipient_compressed_account - .compressed_account - .owner - .to_bytes(), - light_compressed_token::ID.to_bytes() -); +assert_eq!( + transfer_recipient_compressed_account + .compressed_account + .owner, + light_compressed_token::ID.into() // `From<[u8;32]> for Pubkey` impl +);program-libs/macros/src/lib.rs (1)
18-25: Add doc-tests for the newpubkey_array!macroThe macro implementation mirrors
pubkey!, but there is no usage example or compile-time doctest yet.
Including a minimal doctest ensures the macro continues to expand correctly and signals intent to downstream users./// Converts a base58 encoded public key into a raw byte array [u8; 32]. #[proc_macro] pub fn pubkey_array(input: TokenStream) -> TokenStream { @@ } + +/// ``` +/// use light_macros::pubkey_array; +/// +/// const KEY: [u8; 32] = pubkey_array!("11111111111111111111111111111111"); +/// assert_eq!(KEY.len(), 32); +/// ```examples/anchor/counter/src/lib.rs (1)
51-52: Minor duplication – consider a small helper
&crate::IDis repeated in everyLightAccount::new_*call. A tiny wrapper (e.g.const PROGRAM_ID: &Pubkey = &crate::ID;) or a locallet pid = &crate::ID;would remove the repetition and future-proof refactors.- let mut counter = LightAccount::<'_, CounterAccount>::new_mut( - &crate::ID, + let pid = &crate::ID; + let mut counter = LightAccount::<'_, CounterAccount>::new_mut( + pid,Purely cosmetic, feel free to skip.
Also applies to: 83-84, 118-119, 159-160, 198-199
program-tests/system-cpi-test/src/invalidate_not_owned_account.rs (2)
126-135: Validate.into()conversion forownerfield
light_compressed_token::ID.into()changes the compile-time type ofIDto whateverCompressedAccount.ownerexpects.
Please double-check that:
IDis now[u8; 32](per the new local-pubkey approach) and not a wrapper struct.Intois implemented without allocating or hashing, otherwise this runs in hot paths for every CPI test and will add avoidable overhead.If the conversion is just
From<[u8;32]>→[u8;32], you can drop the call entirely and assign the constant directly.
612-617: Same concern forID.into()insidewrite_into_cpi_accountFor consistency and potential micro-optimisation, consider avoiding the redundant
.into()here as well once theownertype and constant match.program-tests/e2e-test/tests/test.rs (1)
100-107: Minor tidy-up possibilityBecause the omitted fields already default to
None, setting them explicitly is redundant.
No action required—just a note in case you prefer the terser form:let general_config = GeneralActionConfig { ..GeneralActionConfig::default() };program-libs/batched-merkle-tree/src/batch.rs (1)
170-176: Logging macro now Solana-only – unify with a thin wrapperYou replaced the previous
crate::msg!macro withsolana_msg::msg!behindcfg(feature = "solana").
As a result, nothing is logged when building withpinocchio, which might hinder debugging.Consider re-introducing a simple shim:
#[cfg(feature = "solana")] macro_rules! log { ($($t:tt)*) => { solana_msg::msg!($($t)*) } } #[cfg(not(feature = "solana"))] macro_rules! log { ($($t:tt)*) => { /* no-op or eprintln! */ } }and use
log!everywhere. It removes the repeated cfg attributes and keeps parity across back-ends.examples/anchor/token-escrow/src/escrow_with_compressed_pda/withdrawal.rs (1)
90-91: Minor: unnecessary.into()– keep the code symmetric
crate::IDis already aPubkey; adding.into()is redundant and slightly obscures intent. Harmless, just consider reverting for consistency with the rest of the codebase.Also applies to: 115-116
program-tests/utils/src/batched_address_tree.rs (1)
183-185: Byte-wisePubkeycomparison is fine but document the reasonSwitching to
.to_bytes()avoids potential trait‐implementation mismatches between differentPubkeytypes, but the magic constants ([0u8; 32]) are now less self-explanatory. Consider a short comment:// Compare raw bytes because different `Pubkey` newtypes are used under // Solana vs Pinocchio builds and they don't share `PartialEq`.Also applies to: 196-198
program-libs/account-checks/src/account_info/mod.rs (1)
1-6: Module scaffold looks goodStraight-forward
moddeclarations with proper feature gates. Add a brief crate-level doc comment explaining the purpose of this module to aid newcomers.program-tests/utils/src/spl.rs (2)
595-598: Avoid repeated.into()calls inside hot loopsEvery changed slice builds a new
Vec<Pubkey>by mapping[u8; 32]→Pubkeywith.into().
In performance-sensitive test runs this conversion is executed for every token in the vector and allocates newPubkeys repeatedly.If
get_state_merkle_tree_accountsalready accepts[u8; 32], let the collection stay asVec<[u8; 32]>and move the conversion (if really necessary) once at the API boundary.
This drops one allocation + oneFromper element.- let input_merkle_tree_pubkeys: Vec<Pubkey> = input_merkle_tree_context - .iter() - .map(|x| x.merkle_tree_pubkey.into()) - .collect(); + let input_merkle_tree_pubkeys: Vec<[u8; 32]> = + input_merkle_tree_context.iter().map(|x| x.merkle_tree_pubkey).collect();Same remark applies to all other changed blocks listed above.
(Not performance-critical in CI, but worth keeping the data model lean.)Also applies to: 770-778, 1149-1155, 1323-1329, 1489-1495, 1781-1785
686-687: Minor API-use inconsistencyHere you switched to
output_merkle_tree_pubkeys.as_slice()but a few lines below still pass&input_merkle_tree_pubkeysdirectly.
Pick one style (&vecor.as_slice()) for consistency.program-libs/batched-merkle-tree/src/constants.rs (1)
3-4: Constant now raw bytes – audit downstream usagesSwitching
ACCOUNT_COMPRESSION_PROGRAM_IDfromPubkeyto[u8; 32]reduces deps and compile time – nice.
However, every caller that expects aPubkeymust now wrap the array (Pubkey::new_from_array(id)orid.into()). Failure to do so yields silent type mismatches.Search for the constant name and ensure all comparisons / assignments still compile and perform the same equality semantics.
Also applies to: 29-30
program-tests/sdk-test/tests/test.rs (1)
47-48: Typo in variable name
ouput_queueis missing the second “t”.- let ouput_queue = rpc.get_random_state_tree_info().queue; + let output_queue = rpc.get_random_state_tree_info().queue;.github/workflows/rust.yml (1)
48-58:light-batched-merkle-treeis built three timesThe matrix now compiles this crate in:
concurrent-merkle-treegroup (--features test-only)program-libsgroup (--all-features --test test_e2e)sdk-libsgroup (--features test-only --skip …)Consider dropping one invocation to shave several minutes off CI runtime and avoid cache evictions.
program-tests/system-cpi-test/src/sdk.rs (1)
25-26: Two helper functions that do the same thing – consolidate
to_account_metas_lightis introduced here whileto_account_metasis still used lower in the file.
They both turn aHashMap<Pubkey, usize>into sortedAccountMetas, so having two flavours increases cognitive load and the chance of subtle divergence.Consider exposing a single helper (e.g. the new
*_lightversion) and using it consistently:- let remaining_accounts = to_account_metas_light(remaining_accounts); + let remaining_accounts = to_account_metas(remaining_accounts);(or remove the old one altogether).
This keeps the SDK surface minimal and avoids future maintenance friction.
Also applies to: 116-116
program-libs/macros/src/pubkey.rs (1)
42-60: Duplicate decoding logic – small DRY opportunity
pubkey_array()repeats the exact decoding & length-check logic frompubkey().
Extract the common part into a helper (e.g.decode_pubkey_bytes(&LitStr) -> Result<[u8;32]>) to avoid divergence in error handling.program-tests/utils/src/assert_compressed_tx.rs (2)
235-238: Minor: Avoid double.into()conversions inside hot assert loopThe predicate converts two
[u8;32]values toPubkeyon every iteration:*x == output_account.merkle_context.merkle_tree_pubkey.into() || *x == output_account.merkle_context.queue_pubkey.into()Compute once and reuse to save two temporary
Pubkeyconstructions per output account:- assert!(output_merkle_tree_pubkeys.iter().any(|x| *x - == output_account.merkle_context.merkle_tree_pubkey.into() - || *x == output_account.merkle_context.queue_pubkey.into()),); + let tree_pk: Pubkey = output_account.merkle_context.merkle_tree_pubkey.into(); + let queue_pk: Pubkey = output_account.merkle_context.queue_pubkey.into(); + assert!(output_merkle_tree_pubkeys.iter().any(|x| *x == tree_pk || *x == queue_pk));Very small, but this loop can run thousands of times in big tests.
260-263: Readability: pull the converted key out of the iteratorSame comment as above – convert once, compare many.
program-libs/compressed-account/src/constants.rs (1)
1-8: Public-key constants now raw[u8;32]– consider companionPubkeyhelpersMany call-sites will now require
ACCOUNT_COMPRESSION_PROGRAM_ID.into().
Providing a companion constant orconst fnreturningPubkeyavoids repetitive conversions and keeps type‐safety:pub const ACCOUNT_COMPRESSION_PROGRAM_ID_BYTES: [u8;32] = pubkey_array!(...); pub const ACCOUNT_COMPRESSION_PROGRAM_ID: Pubkey = Pubkey::new_from_array(ACCOUNT_COMPRESSION_PROGRAM_ID_BYTES);Not mandatory, but improves ergonomics & micro-performance.
program-libs/merkle-tree-metadata/src/rollover.rs (1)
70-82: DuplicatedClock::get()branches can be collapsedBoth the
pinocchioand thesolanabranches perform identical logic (importClock, callClock::get().unwrap().slot).
Collapsing the duplicate code behind a singleusebased on the feature flag will shorten the block and avoid accidental drift in the future.-#[cfg(feature = "pinocchio")] -{ - use pinocchio::sysvars::{clock::Clock, Sysvar}; - self.rolledover_slot = Clock::get().unwrap().slot; -} -#[cfg(not(feature = "pinocchio"))] -{ - use solana_sysvar::{clock::Clock, Sysvar}; - self.rolledover_slot = Clock::get().unwrap().slot; -} +#[cfg(feature = "pinocchio")] +use pinocchio::sysvars::{clock::Clock, Sysvar}; +#[cfg(not(feature = "pinocchio"))] +use solana_sysvar::{clock::Clock, Sysvar}; + +self.rolledover_slot = Clock::get().unwrap().slot;forester/tests/batched_state_async_indexer_test.rs (1)
870-876: Minor: redundant.into()allocations
payer.pubkey().into()is executed for every loop iteration, yetpayer.pubkey()is alreadyPubkey; converting for every transfer adds avoidable copies.
Cache the converted key once outside the loop if the destination type differs, or storePubkeydirectly inCompressedAccount.program-tests/utils/src/assert_rollover.rs (1)
145-151: Inconsistent key comparison styleAbove you switched to byte–wise comparison (
to_bytes()) forassociated_merkle_tree, but here you compare the raw structs:assert_eq!( old_queue_metadata.next_queue, light_compressed_account::Pubkey::from(*new_queue_pubkey) );For consistency and easier diffing, prefer the same
to_bytes()approach or use.eq()everywhere.program-libs/batched-merkle-tree/src/rollover_address_tree.rs (1)
31-32: Unnecessary.into()onnew_account.key()
new_account.key()already returns the Light-formatPubkey; converting it again is redundant and may allocate.- new_account.key().into(), + new_account.key(),program-libs/verifier/src/lib.rs (2)
221-226:target_os = "solana"gating makes local tests silentThe extra
target_os = "solana"filter means themsg!logs are stripped from
off-chain tests (cargo test --features solana).
If the intention is to see those diagnostics during unit / integration tests, gate only onfeature = "solana".
276-297: On-chain debug spam risks exceeding compute budgetDumping full public inputs and proof elements every time verification fails
will inflate the log buffer and consume BPF compute units.
Consider limiting the log size (e.g. first N bytes) or guard with an additional
debug_assert!()/cfg!(debug_assertions).program-tests/utils/src/system_program.rs (2)
57-61: Avoid repeated.into()inside tight loops
derive_address_legacy(&address_merkle_tree_pubkeys[i].into(), …)allocates a
new wrapper on every iteration. Convert once outside the loop to avoid needless
copies:-for (i, address_seed) in address_seeds.iter().enumerate() { - let derived_address = - derive_address_legacy(&address_merkle_tree_pubkeys[i].into(), address_seed).unwrap(); +for (i, address_seed) in address_seeds.iter().enumerate() { + let tree: light_compressed_account::Pubkey = address_merkle_tree_pubkeys[i].into(); + let derived_address = derive_address_legacy(&tree, address_seed).unwrap(); derived_addresses.push(derived_address); }
569-610: Unnecessary.into()churn when populatingremaining_accounts
merkle_tree_pubkey.into()/queue_pubkey.into()is called multiple times
for the same keys, causing duplicate conversions and extra HashMap look-ups.
Store the converted value in a local before using it inget/insert.This also prevents accidental mismatches should
Intoperform non-idempotent
transformations (unlikely but safer).program-tests/system-test/tests/test.rs (3)
252-267: Silencing Clippy instead of fixing the loop hides an easy improvement
#[allow(clippy::needless_range_loop)]suppresses a lint that is correct here – the manual index loop can be replaced by an iterator, improving readability and reducing the chance of off-by-one errors:-#[allow(clippy::needless_range_loop)] -for i in 0..num_outputs { - ... -} +for (i, output_account) in output_compressed_accounts.iter_mut().enumerate().take(num_outputs) { + if output_compressed_accounts_with_address && i < num_addresses { + output_account.address = Some(derived_addresses[i]); + } +}If the range loop is truly necessary (e.g. for
iarithmetic), document the reason instead of globally muting the lint.
260-265: RepeatedPubkey → Wrapperconversions – cache onceThe pattern
owner: <pubkey>.into()is repeated dozens of times. Each call performs a conversion that is cheap but clutters the code and can become error-prone if the wrapper type changes.Extract once per scope:
let payer_owner = payer_pubkey.into(); ... CompressedAccount { lamports: 0, owner: payer_owner, ... }This improves readability and avoids accidental mismatches (e.g. mixing
payer.pubkey()andpayer_pubkey).Also applies to: 894-899, 943-949, 978-984, 1111-1117, 1470-1476, 1572-1577, 1856-1862
563-569: Passing references to temporaries created by.into()
derive_address_legacy(&env.v1_address_trees[0].merkle_tree.into(), address_seed)takes a reference to a value returned by.into().
While this compiles (the temporary lives for the duration of the call), it is easy to misuse and obscures the lifetime. Allocate the converted value once and pass a reference to the binding:let address_tree: _ = env.v1_address_trees[0].merkle_tree.into(); let derived_address = derive_address_legacy(&address_tree, address_seed).unwrap();The intent becomes explicit and avoids accidental dangling-reference bugs if the function signature ever changes.
program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (2)
720-733: Legacy helper signatures diverged – test helper still compiles but hides a panic-path
derive_address_legacy(&address_tree.into(), &seed).unwrap()
derive_address_legacypreviously returnedPubkey, now it returnsResult<Pubkey, _>and is always unwrapped in test-code.
If the refactor ever changes the internal validation (e.g. rejects mixed back-ends) these tests will start panicking instead of failing gracefully.Recommend replacing unconditional
unwrap()withexpect("derive_address_legacy failed")or a properResultpropagation so test output is meaningful.
2921-2923: Avoid double allocation inmap(|x| x.merkle_tree.into())Inside a tight loop this conversion is called for every output account.
Ifinto()produces a copy it is fine, otherwise we are allocating a new value for every iteration when a simpleclone()(or keeping a reference) would be cheaper.- .map(|x| x.merkle_tree.into()) + .map(|x| x.merkle_tree) // avoid extra Into if both types are identicalRun
cargo bloat --tests --releaseto confirm allocations are not regressed.examples/anchor/token-escrow/tests/test_compressed_pda.rs (1)
211-213: Minor readability hit from chained.into()callsThe extra
.into()casts on every*_pubkeyfield make the statement noise-heavy and add no functional value once the localPubkeybecomes the canonical type.- address_merkle_tree_pubkey: env.v1_address_trees[0].merkle_tree.into(), - address_queue_pubkey: env.v1_address_trees[0].queue.into(), + address_merkle_tree_pubkey: env.v1_address_trees[0].merkle_tree, + address_queue_pubkey: env.v1_address_trees[0].queue,Same applies to the two
MerkleContextliterals below (lines 225-227 & 413-422).Not a blocker but worth considering.
Also applies to: 225-227, 413-422
program-libs/compressed-account/src/instruction_data/zero_copy.rs (4)
124-126: ReturningPubkeyby value copies 32 bytes each call
owner()now returns thePubkeyby value instead of by reference, introducing an unnecessary 32-byte copy every time the trait is used.Unless the caller needs ownership, prefer
&Pubkeyfor zero-cost access:-fn owner(&self) -> Pubkey { - self.compressed_account.owner -} +fn owner(&self) -> &Pubkey { + &self.compressed_account.owner +}Mirrors the
InputAccount::ownersignature further below and keeps the API consistent.
276-279: Dropped.into()is correct but document the invarianceAssigning
owner: compressed_account.ownerassumes both structs use the samePubkeyconcrete type after the refactor.
Add a short comment so future maintainers know no additional conversion is required.// Same `Pubkey` concrete type – direct copy is fine. owner: compressed_account.owner,
1403-1405: Avoid cloning when comparing pubkeys in tests
reference.owner.to_bytes()allocates a fresh array each time;as_bytes()on both sides is allocation-free:-if reference.owner.to_bytes() != z_copy.owner.as_bytes() { +if reference.owner.as_bytes() != z_copy.owner.as_bytes() {Tiny win but free.
460-470: Duplicate logic – factor out helper forowner()Both
ZInstructionDataInvokeandZInstructionDataInvokeCpiimplement identicalowner()bodies.
Extracting a small utility (or a blanket default method in the trait) removes duplication and future drift.Not blocking this PR but worth tracking.
Also applies to: 535-547
program-tests/utils/src/create_address_test_program_sdk.rs (1)
33-37: HashMap ordering may cause non-deterministic meta orderingTransaction
AccountMetaorder must be deterministic for replay protection.
BecauseHashMapiteration order is random, the order in which keys reachto_account_metas_lightdepends on the map’s internal hash order. If that helper does not sort internally, signatures could differ run-to-run.-use std::collections::HashMap; +use std::collections::BTreeMap; ... -let mut remaining_accounts = HashMap::<light_compressed_account::Pubkey, usize>::new(); +let mut remaining_accounts = BTreeMap::<light_compressed_account::Pubkey, usize>::new();A
BTreeMap(or explicit sort before serialisation) guarantees stable ordering while keeping code size minimal.programs/compressed-token/src/process_mint.rs (1)
40-45: Generic slice overimpl AsPubkeycan balloon monomorphisation
&[impl AsPubkey]creates a new monomorphised version ofprocess_mint_to_or_compressfor every concrete type array it is called with.
Unless you rely on multiple distinct key types, a single generic parameter avoids extra codegen:-pub fn process_mint_to_or_compress<'info, const IS_MINT_TO: bool>( - ctx: Context<'_, '_, '_, 'info, MintToInstruction<'info>>, - recipient_pubkeys: &[impl AsPubkey], +pub fn process_mint_to_or_compress<'info, const IS_MINT_TO: bool, P: AsPubkey>( + ctx: Context<'_, '_, '_, 'info, MintToInstruction<'info>>, + recipient_pubkeys: &[P],This keeps one instantiation irrespective of the caller’s concrete slice element type.
program-libs/compressed-account/src/lib.rs (2)
19-29:Pubkeyre-export: missing#[doc]/ visibility clarityGreat to see the unified re-export, but consider adding a short rust-doc so downstream users understand that this is always the local, backend-agnostic type and not Solana’s
Pubkey./// Backend-agnostic 32-byte public key used across Lightprotocol. /// This is *not* `solana_sdk::pubkey::Pubkey`. pub use pubkey::Pubkey;
82-88: Guard against feature combinationsTwo independent
From<CompressedAccountError> for ProgramErrorimpls are fine, but if bothsolanaandpinocchioare enabled you still avoid overlap because the target types differ.Please add an explicit mutually-exclusive feature check to make intent obvious:
#[cfg(all(feature = "solana", feature = "pinocchio"))] compile_error!("`solana` and `pinocchio` features cannot be enabled together in this crate");This prevents accidental “everything” builds from compiling invalid binaries.
program-libs/batched-merkle-tree/src/queue.rs (2)
147-154: Redundant conversion when constructing program id
Pubkey::new_from_array(ACCOUNT_COMPRESSION_PROGRAM_ID)allocates a newPubkeyevery call.
Since the constant is already a 32-byte array, cache thePubkey:lazy_static::lazy_static! { static ref ACCOUNT_COMPRESSION_PROGRAM_ID_PK: Pubkey = Pubkey::new_from_array(ACCOUNT_COMPRESSION_PROGRAM_ID); } ... Self::from_account_info::<OUTPUT_STATE_QUEUE_TYPE_V2, A>( &*ACCOUNT_COMPRESSION_PROGRAM_ID_PK, account_info, )This avoids repeated hashing of the same bytes.
499-502:solana_msgcrate dependency may be unnecessaryThe conditional log uses
solana_msg::msg!, yetsolana_program::msg!is already available with thesolanafeature and avoids another crate:- #[cfg(feature = "solana")] - for batch in batch_metadata.batches.iter() { - solana_msg::msg!("batch {:?}", batch); - } + #[cfg(feature = "solana")] + for batch in batch_metadata.batches.iter() { + solana_program::msg!("batch {:?}", batch); + }Unless
solana_msgwraps additional functionality, removing it simplifies dependency graph.program-tests/utils/src/e2e_test_env.rs (1)
2876-2893: Repeatedto_bytes()allocations inside tight filterInside
get_random_compressed_sol_accountsthe closure used in the
.find(..)chain converts both sides to byte arrays on every iteration:x.accounts.merkle_tree.to_bytes() == first_account.merkle_context.merkle_tree_pubkey.to_bytes()
Pubkey::to_bytes()allocates a temporary[u8; 32]each time. When the
state set is large this becomes a hot path. Cache the byte array of
first_accountonce outside the loop and compare against it, or compare
thePubkeyvalues directly if both types now implementPartialEq.program-libs/account-checks/tests/tests.rs (2)
1-1: Consider splitting tests by feature for better modularity.While the current approach works with
--all-featuresin CI, consider organizing tests into separate modules that can run independently with each feature. This would allow testing each backend in isolation when needed.
789-845: Track the commented out rent balance tests.These tests are commented out due to sysvar unavailability in the test environment. Consider creating an issue to track implementing these tests with proper test setup or mock sysvar functionality.
Would you like me to help create a solution that mocks the sysvar functionality for testing or open an issue to track this work?
program-tests/account-compression-test/tests/merkle_tree_tests.rs (3)
229-229: Fix nested list formatting in test comment
Add a space after6.and capitalizeTryfor consistency in comment style:- /// 6.try inserting again it should fail with full error + /// 6. Try inserting again; it should still fail with a full error
422-424: Improve sub-point comment consistency (3.x)
The nested list items under "3. invalid queue accounts:" would be clearer with consistent punctuation and capitalization. Consider this style:- /// 3.1 pass non queue account as queue account + /// 3.1. Pass non-queue account as queue account - /// 3.2 pass address queue account + /// 3.2. Pass address queue account instead of nullifier queue - /// 3.3 pass non associated queue account + /// 3.3. Pass non-associated queue account
427-428: Align punctuation for Merkle tree account comments (4.x)
Add periods after sub-point numbers and standardize wording:- /// 4.1 pass non Merkle tree account as Merkle tree account + /// 4.1. Pass non-Merkle tree account as Merkle tree account - /// 4.2 pass non associated Merkle tree account + /// 4.2. Pass non-associated Merkle tree account
📜 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 (107)
.github/workflows/rust.yml(1 hunks)examples/anchor/counter/src/lib.rs(5 hunks)examples/anchor/counter/tests/test.rs(1 hunks)examples/anchor/token-escrow/Cargo.toml(1 hunks)examples/anchor/token-escrow/src/escrow_with_compressed_pda/escrow.rs(1 hunks)examples/anchor/token-escrow/src/escrow_with_compressed_pda/sdk.rs(2 hunks)examples/anchor/token-escrow/src/escrow_with_compressed_pda/withdrawal.rs(3 hunks)examples/anchor/token-escrow/src/escrow_with_pda/sdk.rs(1 hunks)examples/anchor/token-escrow/tests/test.rs(3 hunks)examples/anchor/token-escrow/tests/test_compressed_pda.rs(7 hunks)forester-utils/src/instructions/address_batch_update.rs(2 hunks)forester/tests/address_v2_test.rs(4 hunks)forester/tests/batched_state_async_indexer_test.rs(6 hunks)program-libs/account-checks/Cargo.toml(2 hunks)program-libs/account-checks/src/account_info/account_info_trait.rs(1 hunks)program-libs/account-checks/src/account_info/mod.rs(1 hunks)program-libs/account-checks/src/account_info/pinocchio.rs(1 hunks)program-libs/account-checks/src/account_info/solana.rs(1 hunks)program-libs/account-checks/src/checks.rs(7 hunks)program-libs/account-checks/src/error.rs(1 hunks)program-libs/account-checks/src/lib.rs(1 hunks)program-libs/account-checks/tests/account_info.rs(1 hunks)program-libs/account-checks/tests/tests.rs(1 hunks)program-libs/batched-merkle-tree/Cargo.toml(1 hunks)program-libs/batched-merkle-tree/src/batch.rs(3 hunks)program-libs/batched-merkle-tree/src/constants.rs(2 hunks)program-libs/batched-merkle-tree/src/initialize_address_tree.rs(5 hunks)program-libs/batched-merkle-tree/src/initialize_state_tree.rs(4 hunks)program-libs/batched-merkle-tree/src/lib.rs(0 hunks)program-libs/batched-merkle-tree/src/merkle_tree.rs(8 hunks)program-libs/batched-merkle-tree/src/queue.rs(7 hunks)program-libs/batched-merkle-tree/src/queue_batch_metadata.rs(0 hunks)program-libs/batched-merkle-tree/src/rollover_address_tree.rs(3 hunks)program-libs/batched-merkle-tree/src/rollover_state_tree.rs(4 hunks)program-libs/batched-merkle-tree/tests/account_access.rs(9 hunks)program-libs/bloom-filter/src/lib.rs(1 hunks)program-libs/compressed-account/Cargo.toml(2 hunks)program-libs/compressed-account/src/constants.rs(1 hunks)program-libs/compressed-account/src/indexer_event/event.rs(1 hunks)program-libs/compressed-account/src/indexer_event/parse.rs(4 hunks)program-libs/compressed-account/src/instruction_data/with_readonly.rs(3 hunks)program-libs/compressed-account/src/instruction_data/zero_copy.rs(3 hunks)program-libs/compressed-account/src/lib.rs(3 hunks)program-libs/compressed-account/src/pubkey.rs(7 hunks)program-libs/concurrent-merkle-tree/Cargo.toml(1 hunks)program-libs/concurrent-merkle-tree/src/errors.rs(1 hunks)program-libs/hasher/Cargo.toml(1 hunks)program-libs/hasher/src/errors.rs(1 hunks)program-libs/hasher/src/hash_to_field_size.rs(2 hunks)program-libs/hasher/src/lib.rs(0 hunks)program-libs/indexed-merkle-tree/src/errors.rs(1 hunks)program-libs/macros/src/lib.rs(1 hunks)program-libs/macros/src/pubkey.rs(1 hunks)program-libs/merkle-tree-metadata/Cargo.toml(1 hunks)program-libs/merkle-tree-metadata/src/errors.rs(1 hunks)program-libs/merkle-tree-metadata/src/lib.rs(0 hunks)program-libs/merkle-tree-metadata/src/rollover.rs(2 hunks)program-libs/verifier/Cargo.toml(1 hunks)program-libs/verifier/src/lib.rs(4 hunks)program-tests/account-compression-test/tests/address_merkle_tree_tests.rs(1 hunks)program-tests/account-compression-test/tests/merkle_tree_tests.rs(4 hunks)program-tests/client-test/tests/light_client.rs(2 hunks)program-tests/client-test/tests/light_program_test.rs(2 hunks)program-tests/compressed-token-test/tests/test.rs(33 hunks)program-tests/create-address-test-program/src/create_pda.rs(2 hunks)program-tests/e2e-test/tests/test.rs(2 hunks)program-tests/registry-test/tests/tests.rs(4 hunks)program-tests/sdk-anchor-test/programs/sdk-anchor-test/src/lib.rs(2 hunks)program-tests/sdk-test/src/create_pda.rs(3 hunks)program-tests/sdk-test/src/update_pda.rs(1 hunks)program-tests/sdk-test/tests/test.rs(2 hunks)program-tests/system-cpi-test/Cargo.toml(1 hunks)program-tests/system-cpi-test/src/create_pda.rs(4 hunks)program-tests/system-cpi-test/src/invalidate_not_owned_account.rs(2 hunks)program-tests/system-cpi-test/src/sdk.rs(3 hunks)program-tests/system-cpi-test/tests/test.rs(10 hunks)program-tests/system-cpi-test/tests/test_program_owned_trees.rs(2 hunks)program-tests/system-cpi-v2-test/tests/event.rs(27 hunks)program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs(40 hunks)program-tests/system-test/tests/test.rs(48 hunks)program-tests/utils/src/assert_compressed_tx.rs(3 hunks)program-tests/utils/src/assert_merkle_tree.rs(1 hunks)program-tests/utils/src/assert_rollover.rs(2 hunks)program-tests/utils/src/assert_token_tx.rs(1 hunks)program-tests/utils/src/batched_address_tree.rs(2 hunks)program-tests/utils/src/create_address_test_program_sdk.rs(4 hunks)program-tests/utils/src/e2e_test_env.rs(10 hunks)program-tests/utils/src/lib.rs(1 hunks)program-tests/utils/src/pack.rs(1 hunks)program-tests/utils/src/spl.rs(7 hunks)program-tests/utils/src/system_program.rs(16 hunks)programs/account-compression/src/instructions/initialize_batched_state_merkle_tree.rs(1 hunks)programs/account-compression/src/instructions/nullify_leaves.rs(1 hunks)programs/account-compression/src/instructions/resize_registered_program_account.rs(1 hunks)programs/account-compression/src/instructions/update_address_merkle_tree.rs(1 hunks)programs/account-compression/src/processor/insert_addresses.rs(1 hunks)programs/account-compression/src/processor/insert_nullifiers.rs(1 hunks)programs/compressed-token/src/freeze.rs(2 hunks)programs/compressed-token/src/process_mint.rs(4 hunks)programs/compressed-token/src/process_transfer.rs(8 hunks)programs/registry/src/lib.rs(1 hunks)programs/system/src/accounts/init_context_account.rs(1 hunks)programs/system/src/context.rs(2 hunks)programs/system/src/errors.rs(0 hunks)programs/system/src/invoke/verify_signer.rs(2 hunks)programs/system/src/invoke_cpi/process_cpi_context.rs(5 hunks)programs/system/src/lib.rs(2 hunks)
⛔ Files not processed due to max files limit (19)
- programs/system/src/processor/create_address_cpi_data.rs
- programs/system/src/processor/sum_check.rs
- programs/system/tests/invoke_cpi_instruction.rs
- programs/system/tests/invoke_cpi_instruction_small.rs
- scripts/format.sh
- scripts/lint.sh
- sdk-libs/client/src/indexer/types.rs
- sdk-libs/client/src/rpc/client.rs
- sdk-libs/macros/Cargo.toml
- sdk-libs/macros/src/discriminator.rs
- sdk-libs/program-test/src/indexer/test_indexer.rs
- sdk-libs/program-test/src/program_test/rpc.rs
- sdk-libs/program-test/src/utils/setup_light_programs.rs
- sdk-libs/sdk/Cargo.toml
- sdk-libs/sdk/src/account.rs
- sdk-libs/sdk/src/account_info.rs
- sdk-libs/sdk/src/address.rs
- sdk-libs/sdk/src/instruction/tree_info.rs
- sdk-libs/sdk/src/lib.rs
💤 Files with no reviewable changes (5)
- programs/system/src/errors.rs
- program-libs/hasher/src/lib.rs
- program-libs/batched-merkle-tree/src/queue_batch_metadata.rs
- program-libs/merkle-tree-metadata/src/lib.rs
- program-libs/batched-merkle-tree/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (16)
examples/anchor/token-escrow/src/escrow_with_pda/sdk.rs (1)
program-tests/utils/src/pack.rs (1)
add_and_get_remaining_account_indices(17-34)
program-tests/utils/src/assert_token_tx.rs (1)
sdk-libs/sdk/src/account.rs (1)
owner(153-155)
program-libs/macros/src/lib.rs (1)
program-libs/macros/src/pubkey.rs (2)
pubkey_array(42-61)pubkey(21-40)
programs/system/src/lib.rs (2)
program-libs/macros/src/lib.rs (1)
pubkey_array(20-25)program-libs/macros/src/pubkey.rs (1)
pubkey_array(42-61)
programs/system/src/invoke/verify_signer.rs (1)
program-libs/compressed-account/src/pubkey.rs (1)
new_unique(148-150)
examples/anchor/token-escrow/src/escrow_with_compressed_pda/escrow.rs (1)
program-libs/compressed-account/src/address.rs (1)
derive_address_legacy(18-28)
forester-utils/src/instructions/address_batch_update.rs (1)
js/stateless.js/src/state/types.ts (1)
CompressedProof(133-137)
program-libs/batched-merkle-tree/src/constants.rs (2)
program-libs/macros/src/lib.rs (1)
pubkey_array(20-25)program-libs/macros/src/pubkey.rs (1)
pubkey_array(42-61)
program-libs/batched-merkle-tree/src/rollover_address_tree.rs (1)
program-libs/account-checks/src/checks.rs (1)
check_account_balance_is_rent_exempt(93-115)
program-tests/utils/src/spl.rs (2)
program-tests/system-cpi-test/tests/test.rs (1)
test_indexer(1280-1288)program-tests/compressed-token-test/tests/test.rs (4)
test_indexer(2562-2571)test_indexer(3025-3034)test_indexer(3077-3086)test_indexer(3878-3887)
program-libs/compressed-account/src/constants.rs (2)
program-libs/macros/src/lib.rs (1)
pubkey_array(20-25)program-libs/macros/src/pubkey.rs (1)
pubkey_array(42-61)
programs/compressed-token/src/process_mint.rs (4)
program-libs/compressed-account/src/instruction_data/with_readonly.rs (1)
data(96-102)program-libs/compressed-account/src/indexer_event/parse.rs (3)
data(353-361)data(399-419)data(420-442)program-libs/compressed-account/src/instruction_data/zero_copy.rs (4)
data(140-145)data(361-363)data(751-755)data(756-760)program-libs/macros/src/pubkey.rs (1)
pubkey(21-40)
program-libs/account-checks/src/account_info/pinocchio.rs (3)
program-libs/account-checks/src/account_info/account_info_trait.rs (12)
key(15-15)is_writable(16-16)is_signer(17-17)executable(18-18)lamports(19-19)data_len(20-20)try_borrow_data(23-23)try_borrow_mut_data(24-24)is_owned_by(27-27)find_program_address(30-30)create_program_address(31-34)get_min_rent_balance(37-37)program-libs/account-checks/src/account_info/solana.rs (12)
key(15-17)is_writable(19-21)is_signer(23-25)executable(27-29)lamports(31-33)data_len(35-37)try_borrow_data(39-44)try_borrow_mut_data(46-51)is_owned_by(53-55)find_program_address(57-61)create_program_address(63-71)get_min_rent_balance(73-78)program-libs/account-checks/src/error.rs (6)
from(37-54)from(59-61)from(66-68)from(73-75)from(80-82)from(87-89)
program-libs/compressed-account/src/instruction_data/zero_copy.rs (1)
program-libs/compressed-account/src/compressed_account.rs (1)
reference(555-676)
program-libs/batched-merkle-tree/src/rollover_state_tree.rs (3)
program-libs/account-checks/src/checks.rs (1)
check_account_balance_is_rent_exempt(93-115)program-libs/account-checks/src/account_info/account_info_trait.rs (1)
get_min_rent_balance(37-37)program-libs/account-checks/src/account_info/solana.rs (1)
get_min_rent_balance(73-78)
program-libs/account-checks/tests/tests.rs (2)
program-libs/account-checks/src/account_info/test_account_info.rs (3)
new(121-129)get_account_info(13-64)get_account_info(131-142)program-libs/account-checks/src/checks.rs (9)
check_account_info_mut(23-31)check_account_info_non_mut(37-44)check_non_mut(46-51)check_signer(117-122)check_owner(124-132)check_program(134-145)check_pda_seeds(147-157)check_pda_seeds_with_bump(159-169)check_account_balance_is_rent_exempt(93-115)
🪛 GitHub Actions: forester-tests
program-tests/utils/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
program-libs/macros/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
programs/system/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
program-tests/sdk-anchor-test/programs/sdk-anchor-test/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
program-libs/bloom-filter/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
examples/anchor/counter/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
programs/registry/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
program-libs/account-checks/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
program-libs/verifier/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
program-libs/compressed-account/src/lib.rs
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: stateless-js
🔇 Additional comments (87)
program-libs/hasher/Cargo.toml (1)
9-13: Removingdefaultfeatures can break downstream builds – double-check all dependents
default = []means crates that previously compiled implicitly with thesolanapath must now opt-in viafeatures = ["solana"](or"pinocchio"). Please make sure:
- Every
program-libs/*crate that relies onlight-hasheradds the correct feature in its ownCargo.toml.- CI jobs that build or test sub-crates run with
--all-features(already updated) and at least one job exercises the “no-features” build to keep that configuration green.A quick grep for
light-hasherwithout an explicit feature flag in the workspace would catch any omissions.program-libs/indexed-merkle-tree/src/errors.rs (1)
60-65: Consistent feature gating – LGTMThe conversion to
solana_program_error::ProgramErroris now enabled byfeature = "solana"rather than “not pinocchio”, mirroring the pattern used everywhere else. Good clean-up.program-libs/bloom-filter/src/lib.rs (1)
22-34: Feature flags simplified – looks correctEach
From<...> for ProgramErrorimpl is now guarded solely by its own backend feature. This matches other crates after the refactor and avoids mutually exclusive cfgs. No action needed.program-tests/utils/src/lib.rs (1)
32-32: Export the newpackmodule
Addingpub mod pack;correctly exposes the packing utilities to consumers oflight_test_utils.examples/anchor/token-escrow/src/escrow_with_pda/sdk.rs (1)
16-16: Redirectadd_and_get_remaining_account_indicesimport
Swapping the import tolight_test_utils::packaligns with the new utility placement and is consistent with related examples.program-libs/batched-merkle-tree/Cargo.toml (1)
42-42: Makesolana-msga non-optional dependency
Includingsolana-msgunconditionally ensures logging macros are available when both Solana and Pinocchio features are enabled, resolving previous compilation conflicts.program-tests/sdk-test/src/update_pda.rs (1)
28-28: SimplifyLightAccount::new_mutcall
Removing the intermediateprogram_idvariable and using&crate::IDdirectly cleans up the code without altering behavior.program-tests/system-cpi-test/Cargo.toml (1)
37-37: Conditionally includelight-test-utilsfor non-Solana targets
Movinglight-test-utilsinto thecfg(not(target_os = "solana"))section provides the necessary utilities where they’re used, while keeping Solana builds lean.program-libs/verifier/Cargo.toml (1)
9-12: Removal of default features is potentially breaking – confirm reverse-dependencies still buildSetting
default = []means any downstream crate that previously relied on
light-verifier’s implicit"solana"feature must now opt‐in explicitly. Please run a workspace-widecargo check --all(and CI) with no extra features to ensure nothing unexpectedly fails to compile, and document the change in the release notes.program-libs/account-checks/Cargo.toml (1)
9-12: Same breaking-change risk as above
default = []removes the automatic Solana backend. Verify all crates/tests that depend onlight-account-checksspecify a backend feature or they will now fail to compile.program-tests/account-compression-test/tests/address_merkle_tree_tests.rs (1)
509-512: Docstring numbering fix looks goodThe clarification in comment numbering improves readability; no further action required.
programs/account-compression/src/instructions/initialize_batched_state_merkle_tree.rs (1)
60-64: Conversion looks good – just confirm trait boundsPassing
owner.into()is correct ifinit_batched_state_merkle_tree_from_account_infonow expectsimpl Into<[u8;32]>. No further action needed.examples/anchor/token-escrow/src/escrow_with_compressed_pda/sdk.rs (1)
16-18: Import path update LGTMSwitching to
light_test_utils::pack::*centralises packing utilities and removes duplicate helpers. No issues spotted.examples/anchor/counter/tests/test.rs (1)
1-1: Feature-gate now active – double-check CI matrixThe test module is now compiled only when the
test-sbffeature is enabled.
Be sure that all CI jobs intended to execute these tests pass--features test-sbf(or--all-features, as in the updated workflow) and that local developers are aware of the flag.programs/system/src/accounts/init_context_account.rs (1)
57-58: Generic parameter order looks correct
account_info_init::<CpiContextAccount, AccountInfo>matches the new two-parameter signature introduced byAccountInfoTrait; no issues spotted.programs/system/src/context.rs (1)
337-347: Inconsistentownervalue vs reference
owner: *input.owner(),dereferences a&Pubkeyto copy the value – fine if
InputAccount::owner() -> &Pubkey.
A few lines below, the output path usesowner: output.owner(),(line 359) with no
dereference. IfOutputAccount::owner()returns aPubkeyby value this is
consistent; otherwise the code will fail to compile or incur an extra copy.Please confirm the two trait signatures are indeed different. If both return
&Pubkeyuse the same pattern for consistency:- owner: output.owner(), + owner: *output.owner(),programs/system/src/invoke/verify_signer.rs (1)
50-52: Conversion now explicit – no objectionsUsing
.into()makes the[u8;32] → Pubkeyconversion explicit and self-documenting; looks good.Also applies to: 72-75
program-tests/sdk-anchor-test/programs/sdk-anchor-test/src/lib.rs (1)
47-52: Redundant local removed – 👍Passing
&crate::IDdirectly simplifies the call sites and removes an unnecessary binding. No functional change.Also applies to: 78-82
programs/account-compression/src/instructions/nullify_leaves.rs (1)
107-114:✅ Verification successful
Double-check
associated_queue’s concrete type
merkle_tree.metadata.associated_queueis now compared toctx.accounts.nullifier_queue.key()without the previous.into().
This indicates thatassociated_queuehas been promoted to aPubkey, while other metadata fields (e.g.owner) still store[u8; 32]and require.into().Please verify that:
- The struct definition of
StateMerkleTreeAccount::metadata.associated_queuehas indeed been updated toPubkey.- All serialization/deserialization paths were adjusted accordingly (Borsh / Anchor zero-copy).
If the field is still
[u8; 32], this comparison will not compile and the missing.into()needs to be reinstated.
🏁 Script executed:
#!/bin/bash # Locate the definition of `associated_queue` and print its type. ast-grep --pattern $'struct StateMerkleTreeAccount { $$$ associated_queue: $_, $$$ }'Length of output: 86
🏁 Script executed:
#!/bin/bash # Search for where `associated_queue` is defined and inspect its type rg 'associated_queue' -n -C 3 # Locate the definition of `StateMerkleTreeAccount` to confirm its metadata field types rg 'struct StateMerkleTreeAccount' -n -C 5Length of output: 27705
No action needed:
associated_queueis already aPubkey.
Verified that in:
program-libs/merkle-tree-metadata/src/merkle_tree.rsprograms/system/src/account_compression_state/state.rsprograms/account-compression/src/state/public_state_merkle_tree.rs
the fieldassociated_queueis declared asPubkey, so comparing directly toctx.accounts.nullifier_queue.key()is type-correct and no.into()is required.program-tests/client-test/tests/light_client.rs (1)
639-656: Conversions look correctThe switch to
.into()forowner,address_queue_pubkey, andaddress_merkle_tree_pubkeymatches the new[u8; 32]-first model.
No issues spotted.Also applies to: 670-675
program-tests/client-test/tests/light_program_test.rs (1)
639-642: Conversions align with the updated key representationSame pattern as in
light_client.rs; the explicit.into()keeps the test compiling across both back-ends. Looks good.Also applies to: 657-663
programs/account-compression/src/processor/insert_addresses.rs (1)
139-145: Redundant.into()removed – OK
queue.metadata.associated_merkle_treeand*merkle_pubkeyare now compared directly.
Assuming both arePubkey, this improves clarity and avoids the extra allocation the former conversion incurred. Looks good.programs/account-compression/src/processor/insert_nullifiers.rs (1)
176-184: DirectPubkeycomparison – looks correctThe equality check now compares two
Pubkeys without a superfluous.into().
This keeps the hot path tight and avoids an unnecessary conversion.examples/anchor/token-escrow/tests/test.rs (1)
370-374: Verify the signer-slice change
create_and_send_transactionpreviously received&[&Keypair]; it now receives&[Keypair].
If the function signature still expects a slice of references (&[&Keypair]), this will fail to compile.
Please confirm that the helper now accepts&[Keypair]; otherwise revert to&[&payer].forester-utils/src/instructions/address_batch_update.rs (1)
17-17: Import path & extra dereference look fineThe new
CompressedProofpath and the extra*in&(*merkle_tree_pubkey).into()compile cleanly and avoid any ambiguity with the trait impls you introduced elsewhere.
No follow-up required.Also applies to: 48-49
program-tests/utils/src/assert_merkle_tree.rs (1)
87-89: Byte-wise comparison is a sensible decouplingSwitching to
.to_bytes()avoids the cross-cratePubkeytype mismatch and keeps the tests stable; good move.Also applies to: 91-109
programs/registry/src/lib.rs (1)
685-687: Type unification looks correctRemoving
.into()aligns both sides to the samePubkeytype after your earlier macro changes.programs/account-compression/src/instructions/update_address_merkle_tree.rs (1)
58-66: Removed.into()– ensure both sides share the exact type
merkle_tree.metadata.associated_queueis now compared directly withctx.accounts.queue.key().
Ifassociated_queueis[u8; 32]whilekey()returns a wrapperPubkey, the comparison will fail to compile or silently diverge depending onPartialEqimpls.Please confirm the two fields are the same concrete type after the refactor; otherwise restore a deterministic conversion on one side.
program-tests/e2e-test/tests/test.rs (1)
45-50: Idiomatic struct initialisation – looks goodNice use of struct-update syntax; it eliminates the mutable default and is clearer.
program-libs/account-checks/src/lib.rs (1)
1-7: Public re-export ofAccountInfoTraitlooks correct
pub use account_info::account_info_trait::AccountInfoTrait;cleanly exposes the trait to dependants; modules compile with a single import line.🧰 Tools
🪛 GitHub Actions: forester-tests
[warning] 1-1: Operation failed, retrying in 1s (attempt 1/30): CustomError("Airdrop not confirmed")
program-libs/compressed-account/src/instruction_data/with_readonly.rs (2)
133-136: LGTM – directPubkeyassignment is clearerRemoving the
.into()keeps the field type explicit and avoids an extra conversion. No concerns here.
471-475: Tests updated correctlyThe test structs now pass a concrete
Pubkeywithout conversion – matches the production code change above.Also applies to: 686-690
program-tests/system-cpi-test/tests/test_program_owned_trees.rs (1)
53-56: Initialization simplified – no issuesReplacing the mutable config block with a struct literal improves readability. Behaviour is unchanged.
Also applies to: 203-206
program-libs/compressed-account/src/indexer_event/event.rs (1)
72-76: Confirm raw-array vsPubkeyalias after.into()removal
tree_pubkey/queue_pubkeyare now copied directly fromseq.*without.into().
Ifcrate::Pubkeyis still a distinct new-type, this will not compile; if you intentionally re-aliased it to[u8; 32], all good.Please run
cargo check --all-featuresacross dependent crates to make sure no implicitFrom<[u8;32]> for Pubkeyusages remain elsewhere.program-tests/sdk-test/tests/test.rs (1)
1-1: Feature-gated test now activeUn-commenting
#![cfg(feature = "test-sbf")]means this file is ignored unless the flag is on.
If you rely on these checks in CI, make sure the workflow sets--features test-sbf; otherwise the test will silently disappear again.program-tests/system-cpi-test/src/create_pda.rs (1)
531-533: Owner supplied from caller – sanity-check before writing to state
owner_programis taken from the instruction data and written straight into the compressed account.
Nothing guards against a malicious client passing an arbitrary key and thereby spoofing ownership.At minimum:
require!( *owner_program == crate::ID, CustomError::InvalidOwner );(or gate it behind a test-only cfg).
This closes a potential privilege-escalation vector.
program-libs/batched-merkle-tree/tests/account_access.rs (1)
4-32: Import & conversion updates look soundThe switch to the new
account_infopath and the pervasive.into()conversions compile on nightly and make the tests backend-agnostic. No concerns here.program-tests/utils/src/assert_compressed_tx.rs (1)
351-353: Consistent type usage 👍Switching to
.into()keeps the API uniform after the[u8;32]refactor.program-tests/registry-test/tests/tests.rs (1)
1880-1883: Good simplificationDirect struct-literal initialisation is clearer and removes a mutable temp.
forester/tests/address_v2_test.rs (1)
353-355: Good use of the newpackhelpersSwitching to
pack_output_compressed_accountscentralises packing logic and reduces boilerplate.
No issues spotted in this segment.program-libs/batched-merkle-tree/src/rollover_address_tree.rs (1)
15-18: GenericAccountInfoTraitadoption looks correctThe signature update makes the helper usable from both Solana & Pinocchio contexts without duplication.
No functional concerns—nice refactor.program-libs/hasher/src/errors.rs (1)
55-59: Pinocchio path looks goodConversion for the pinocchio backend is consistent and compiles as-is.
program-tests/system-cpi-v2-test/tests/event.rs (1)
88-90: 👍 Consistent newtype conversionThe switch to
.into()formerkle_treewhen hashing brings the test in line
with the production code that now operates on the localPubkeynewtype.
No issues spotted.program-libs/merkle-tree-metadata/src/errors.rs (1)
41-52: Confirm unique error-code spaces per backendBoth impls blindly wrap the same numeric space into a
Customerror for each backend.
If Solana and Pinocchio are ever enabled together the numeric codes are still unique inside each backend, so there is no collision, but please double-check that other crates do not rely on a shared global namespace.program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (2)
291-295: Type conversion may hide a logic bug
address_merkle_tree_pubkey: address_tree.into(),
address_treeis already aPubkey. Converting it with.into()only has an effect when a newtype wrapper is in scope; otherwise it is a no-op that moves the value.
Because the sameaddress_treeis reused later in the loop, repeated.into()calls will move the value the first time and trigger a use-after-move compilation error unless aCopyimplementation exists for the target type. ThatCopyautomatically falls back to a byte-wise copy, which is cheap but easy to overlook.Please confirm that:
- Both source and target types implement
Copy.- The semantic difference between the two types (likely Solana
Pubkeyvs locallight_sdk::Pubkey) is indeed only a wrapper, not endianness or domain specific.If (1) or (2) is not true, introduce an explicit
(&address_tree).into()oraddress_tree.clone().into()to avoid accidental moves.
2848-2861:light_test_utils::packis pulled in – verify dev-dependency scopeThe new import tree (
light_test_utils::pack::*) increases compile time for every test crate that depends on this file.
If the pack helpers are only needed for tests, ensurelight_test_utilsis behinddev-dependenciesin Cargo.toml of the parent crate to avoid bloating normal builds.examples/anchor/token-escrow/tests/test_compressed_pda.rs (1)
194-196: Use the newtypePubkeyconsistently – removal of the previous.into()conversions looks goodThe call now passes
&env.v1_address_trees[0].merkle_tree.into()instead of the rawPubkey.
Becausederive_address_legacywas recently made generic over both Solana and LightPubkeyvariants, this change keeps the compile-time type inference straightforward.No action needed.
program-libs/compressed-account/src/instruction_data/zero_copy.rs (1)
339-341: API inconsistency betweenInputAccount::ownerandOutputAccount::owner
InputAccount::ownerreturns&PubkeywhileOutputAccount::owner(line 124) now returnsPubkeyby value.
Aligning the two keeps downstream generic code simpler.Consider making both return references.
program-tests/compressed-token-test/tests/test.rs (10)
1627-1628: Add.into()conversion formerkle_tree_pubkey
Converts the rawPubkeyfield to the unified local type, matching the newAccountInfoTraitabstraction.
1871-1873: Apply.into()on delegated account’smerkle_tree_pubkey
Maintains consistency by casting the field into the localPubkeyrepresentation.
2012-2014: Convertmerkle_tree_pubkeywith.into()in mixed delegation test
Ensures compatibility with the new trait-based key type.
2539-2541: Castmerkle_tree_pubkeyvia.into()in revoke helper
Aligns the key conversion with the updatedAccountInfoTraitAPI.
2687-2689: Use.into()formerkle_tree_pubkeyin revoke failing tests
Standardizes the type conversion for negative-path scenarios.
2894-2896: Uniform.into()conversion formerkle_tree_pubkeyin burn test
Keeps test logic in sync with the new Pubkey abstraction.
3042-3044: Introduce.into()forqueue_pubkeyin freeze failure test
Casts the queue key to the local type to satisfy trait requirements.
3383-3384: Add.into()forqueue_pubkeyin secondary burn failure
Ensures the queue key is converted to our unified Pubkey type.
4066-4066: Convertoutputs_merkle_treewith.into()in thaw failing test
Maintains consistent key handling in negative-path thaw logic.
5291-5292: Apply.into()conversions inMerkleContextmapping
Wraps bothmerkle_tree_pubkeyandqueue_pubkeyinto the local Pubkey type for downstream usage.program-libs/concurrent-merkle-tree/src/errors.rs (1)
64-76: Feature-gatedFromimpls look correctProviding separate implementations for Solana and Pinocchio without the previous mutual-exclusion guard removes the earlier compilation conflict while keeping the APIs intact.
program-libs/batched-merkle-tree/src/initialize_address_tree.rs (1)
73-80: Confirm error propagation after switching toAccountInfoTrait
try_borrow_mut_data()?now propagates the trait’s error type, relying on aFrom<TraitError> for BatchedMerkleTreeErrorimpl.
Please verify that such an impl exists for both Solana and Pinocchio builds; otherwise the?will fail to compile.If missing, add:
impl From<light_account_checks::error::AccountError> for BatchedMerkleTreeError { fn from(e: light_account_checks::error::AccountError) -> Self { BatchedMerkleTreeError::AccountChecks(e) } }(or similar) to keep the conversion explicit.
programs/compressed-token/src/process_transfer.rs (1)
242-244: Potential missingAsPubkeyimpl for references
owner.to_pubkey_bytes()is called on&O(O: AsPubkey).
Unless a blanketimpl<T: AsPubkey + ?Sized> AsPubkey for &Texists, this line will not compile.
Either add that blanket impl inpubkey.rsor deref here:- let hashed_owner = hash_to_bn254_field_size_be(owner.to_pubkey_bytes().as_slice()); + let hashed_owner = hash_to_bn254_field_size_be((*owner).to_pubkey_bytes().as_slice());program-libs/compressed-account/Cargo.toml (1)
9-16: Removal of defaultsolanafeature is a breaking changeSetting
default = []requires every downstream crate to explicitly opt-in to eithersolanaorpinocchio. All internal workspace crates have been updated, but external users will experience unexpected compilation errors.Consider publishing this as
0.3.0(semver-major) and highlight the migration step in the changelog.program-libs/batched-merkle-tree/src/initialize_state_tree.rs (3)
2-2: LGTM!The import correctly brings in the necessary trait and function for the generic account info handling.
69-113: Generic implementation looks good!The function has been properly generalized to accept any type implementing
AccountInfoTrait. The key conversions using.key().into()correctly handle the[u8; 32]toPubkeyconversion, and the simplifiedownerparameter type improves clarity.
139-140: Logging update is consistent!The change to use
solana_msg::msg!with a positive feature gate#[cfg(feature = "solana")]is cleaner and aligns with the logging pattern across the codebase.program-libs/batched-merkle-tree/src/rollover_state_tree.rs (3)
1-1: Import changes look good!The imports correctly bring in the trait and rent exemption check function needed for the generic implementation.
233-234: Consistent logging pattern!The logging change follows the same pattern as other files in the codebase.
48-111:✅ Verification successful
Generic rollover implementation is correct!
The function properly uses the trait abstraction. The rent calculation now correctly uses
A::get_min_rent_balance()which provides the appropriate implementation for each backend.Let me verify that the rent calculation behavior is preserved:
🏁 Script executed:
#!/bin/bash # Description: Verify that get_min_rent_balance implementations match the previous behavior # Check the trait definition for get_min_rent_balance ast-grep --pattern 'fn get_min_rent_balance' # Check how the old code calculated rent rg -A 5 'crate::Rent::get' --type rustLength of output: 2931
Rent calculation equivalence confirmed
The trait’s
get_min_rent_balanceimplementations in
program-libs/account-checks/src/account_info/solana.rsprogram-libs/account-checks/src/account_info/pinocchio.rsboth invoke
Rent::get().minimum_balance(size), exactly matching the originalcrate::Rent::get().minimum_balance(...)logic. No semantic changes to rent computation—approving the generic rollover implementation.program-libs/account-checks/src/account_info/pinocchio.rs (2)
5-44: Trait implementation looks correct!The basic trait methods properly delegate to pinocchio's native methods with appropriate error conversion.
90-111: Rent calculation implementation is correct!The method properly handles different environments with appropriate fallbacks.
program-libs/account-checks/src/account_info/solana.rs (1)
5-79: Excellent Solana trait implementation!The implementation correctly handles all trait methods with proper lifetime bounds on associated types and consistent error mapping. The data borrowing methods elegantly map RefCell's smart pointers to slice references.
program-libs/account-checks/tests/account_info.rs (1)
1-493: Comprehensive test coverage!The tests thoroughly validate both Solana and Pinocchio implementations of
AccountInfoTrait, covering all methods including edge cases and error conditions. The test structure is well-organized and easy to follow.Note: The commented-out
get_min_rent_balance()tests (lines 233-257 and 448-492) should be enabled once the sysvar is available in the test environment.program-libs/account-checks/src/account_info/account_info_trait.rs (1)
1-43: LGTM! Well-designed trait abstraction.The
AccountInfoTraitprovides a comprehensive abstraction over different AccountInfo implementations with proper error handling and all necessary methods for account validation and manipulation.program-tests/system-cpi-test/tests/test.rs (1)
224-225: LGTM! Consistent public key handling improvements.All changes properly standardize public key comparisons to byte-level comparisons and use explicit
.into()conversions, aligning with the PR's goal of improving type compatibility across different public key representations.Also applies to: 236-237, 333-336, 622-623, 719-720, 839-839, 842-850, 877-887, 963-963, 1805-1806, 1812-1812, 1820-1820, 1825-1825, 2010-2011
program-libs/compressed-account/src/pubkey.rs (1)
14-15: LGTM! Excellent improvements to Pubkey interoperability.The changes enhance type compatibility:
- Added
EqandHashderives for use in collections- Added
AsRef<[u8]>andPartialEqimplementations for better comparisons- Proper conversions between different Pubkey types
- Better trait naming from
PubkeyTraittoAsPubkeyfollowing Rust conventionsAlso applies to: 67-84, 118-147, 157-200
program-libs/account-checks/tests/tests.rs (1)
45-787: LGTM! Comprehensive test coverage.Excellent test suite that thoroughly covers both success and failure cases for all account check functions across both Solana and Pinocchio backends. The helper functions and test organization are well-structured.
program-libs/batched-merkle-tree/src/merkle_tree.rs (4)
6-6: LGTM!The addition of
AccountInfoTraitimport is necessary for the generic refactoring.
93-96: Consistent generic parameter usage across public methods.The refactoring to use generic
A: AccountInfoTraitparameter is applied consistently across all public methods that handle account information, enabling seamless support for different implementations.Also applies to: 104-107, 120-122, 147-149
156-167: Clean abstraction using byte arrays for program IDs.The change from
&Pubkeyto&[u8; 32]for program IDs, combined with the generic trait approach, provides better decoupling from specific implementations. The.into()conversion on line 166 properly handles the type conversion.
253-261: Verify logging behavior for non-Solana implementations.The logging is now exclusively gated by the
solanafeature. This means logs won't appear when using other implementations like Pinocchio. Is this intentional, or should there be logging support for other backends as well?program-tests/utils/src/pack.rs (2)
17-34: LGTM! Well-structured account index tracking.The function correctly maintains account indices and safely handles the unwrap on line 31 since the pubkey is guaranteed to be in the map after the insertion logic.
157-182: 🛠️ Refactor suggestionUnexpected mutation of input data.
The function mutates
merkle_context.prove_by_index = trueon line 170, which is unexpected for a packing function. This side effect could surprise callers who don't expect their input data to be modified.Consider cloning the merkle_context before modification to avoid side effects.
.zip(root_indices.iter()) .map(|(x, root_index)| { - let mut merkle_context = x.merkle_context; + let mut merkle_context = x.merkle_context.clone(); let root_index = if let Some(root) = root_index { *root } else {Likely an incorrect or invalid review comment.
program-libs/account-checks/src/checks.rs (3)
1-5: LGTM! Clean trait import.The
AccountInfoTraitimport replaces the previous concrete type dependencies.
8-17: Excellent generic refactoring of account check functions.All account check functions have been consistently updated to:
- Accept generic
A: AccountInfoTraitparameter- Use
&[u8; 32]for program IDs instead of&Pubkey- Access account properties through trait methods
This provides a clean abstraction over different account info implementations.
Also applies to: 23-31, 37-44, 56-66
93-115: Proper trait-based rent exemption check.The function correctly uses
A::get_min_rent_balanceto access rent information through the trait interface, maintaining proper abstraction while preserving platform-specific behavior.program-tests/account-compression-test/tests/merkle_tree_tests.rs (2)
15-15: Confirm import path forInsertIntoQueuesInstructionDataMut
The module pathlight_compressed_account::instruction_data::insert_into_queues::InsertIntoQueuesInstructionDataMutaligns with the refactored structure. Ensure this matches the published crate's API and is consistent in other test files.
40-40: Updatepack_pubkeyimport to unified test utils
Good consolidation:pack_pubkeynow resides inlight_test_utils::pack, centralizing helper functions for consistency across tests.
examples/anchor/token-escrow/src/escrow_with_compressed_pda/withdrawal.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
program-libs/account-checks/src/error.rs (1)
74-101: 🛠️ Refactor suggestionStill lossy: non-
CustomPinocchio errors get flattenedThe new mapping converts every non-
CustomProgramErrorinto a numeric code and then wraps it back intoPinocchioProgramError.
Call-sites can no longer distinguish between, e.g.,InvalidArgumentandInsufficientFunds.Introduce a dedicated catch-all variant (or map 1-to-1):
- PinocchioProgramError(u32), + /// Raw Pinocchio error that does not map to a dedicated variant. + UnknownPinocchioProgramError(u32),and keep the original semantic information:
match error { pinocchio::program_error::ProgramError::Custom(code) => { AccountError::PinocchioProgramError(code) } other => AccountError::UnknownPinocchioProgramError(other.into()), }This preserves fidelity while still fitting in a single enum.
🧹 Nitpick comments (1)
program-libs/account-checks/src/error.rs (1)
33-35: Consider feature-gating or renaming the new variant
PinocchioProgramErroris always compiled in, even when thesolanabackend is used exclusively.
Either wrap the variant itself in#[cfg(feature = "pinocchio")]or rename it to a backend-agnostic catch-all such asExternalProgramError(u32)to avoid leaking Pinocchio-specific details into Solana-only builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
program-libs/account-checks/src/account_info/pinocchio.rs(1 hunks)program-libs/account-checks/src/error.rs(2 hunks)program-tests/account-compression-test/tests/merkle_tree_tests.rs(5 hunks)program-tests/utils/src/e2e_test_env.rs(10 hunks)program-tests/utils/src/system_program.rs(16 hunks)programs/compressed-token/src/process_transfer.rs(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- program-tests/utils/src/system_program.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- program-tests/account-compression-test/tests/merkle_tree_tests.rs
- program-tests/utils/src/e2e_test_env.rs
- programs/compressed-token/src/process_transfer.rs
- program-libs/account-checks/src/account_info/pinocchio.rs
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-test"])
- GitHub Check: system-programs (counter-test, ["cargo test-sbf -p counter"])
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test"])
- GitHub Check: system-programs (token-escrow-test, [ "cargo test-sbf -p token-escrow" ])
- GitHub Check: lint
- GitHub Check: test-state-batched
- GitHub Check: Test program-libs
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: test-2-foresters
- GitHub Check: test-address-batched
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: test-double-registration
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: test-state-photon-batched
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: test-e2e
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: stateless-js
🔇 Additional comments (1)
program-libs/account-checks/src/error.rs (1)
103-115: LGTM – keeps Solana borrow errors isolatedThe targeted
BorrowError/BorrowMutErrorconversions are concise and backend-specific. No issues spotted.
Issue:
Pubkey,AccountInfoand other types. This approach can cause compilation issues for example when cratelight-compressed-accountis used with pinocchio features in a program its tests uselight-program-testwhich depends onlight-compressed-accountwith solana features.light-account-checkslight-batched-merkle-treelight-compressed-accountSolution
light-crate can be compiled withall-targetsall(not(feature = "pinocchio"), feature = "solana")Pubkeyinlight-compressed-accountwith localPubkeytypeAccountInfoTraitand implement it forpinocchioandsolana-*AccountInfoAccountInfoTraitinlight-accounts-checksandlight-batched-merkle-treeRentas associated constant toAccountInfoTraitSummary by CodeRabbit
New Features
Refactor
.into()and byte-level comparisons.Bug Fixes
Chores