Conversation
ananas-block
requested review from
SwenSchaeferjohann and
sergeytimoshin
as code owners
| let merkle_tree_indices = add_and_get_remaining_account_indices( | ||
| input_params.output_compressed_account_merkle_tree_pubkeys, | ||
| input_params.output_compressed_account_merkle_tree_pubkeys, // .iter() | ||
| // .map(|pubkey| anchor_lang::prelude::Pubkey::from(pubkey)) |
| } | ||
|
|
||
| #[cfg(not(feature = "anchor"))] | ||
| #[cfg(all(feature = "solana", not(feature = "anchor")))] |
There was a problem hiding this comment.
Wdyt about renaming it to more idiomatic AsPubkey (like std::convert::AsRef and std::convert::AsMut)?
There was a problem hiding this comment.
Do you mean rename PubkeyTrait -> AsPubkey ?
trait_to_bytes -> to_pubkey_bytes
or trait_to_bytes -> into_bytes
There was a problem hiding this comment.
PubkeyTrait -> AsPubkey
There was a problem hiding this comment.
changed to AsPubkey and to_pubkey_bytes
| solana_program_error::ProgramError, | ||
| solana_sysvar::{clock::Clock, Sysvar}, | ||
| }; | ||
| // // Pinocchio imports |
|
|
||
| [features] | ||
| default = ["solana"] | ||
| default = [] |
There was a problem hiding this comment.
does not compiling light-verifier with feature=solana in CI matter, given some of the solana flags used?
There was a problem hiding this comment.
I want to remove all these flags.
The reason we have them is local VerifierError to ProgramError conversion which was cause for the compile issues I encountered two days ago in light-compressed-account.
| 2, | ||
| MerkleTreeMetadataError::MerkleTreeAndQueueNotAssociated.into(), | ||
| AccountCompressionErrorCode::MerkleTreeMetadataError.into(), | ||
| // MerkleTreeMetadataError::MerkleTreeAndQueueNotAssociated.into(), |
There was a problem hiding this comment.
There are a couple of these. I am not happy with the current error conversion which converts many errors into MerkleTreeMetadataError. Left it as a reminder to fix the conversion into more meaningful errors.
sdk-libs/macros/Cargo.toml
Outdated
|
|
||
| light-hasher = { workspace = true } | ||
| ark-bn254 = { workspace = true } | ||
| # ark-bn254 = { workspace = true } |
| } | ||
| } | ||
|
|
||
| // impl From<AccountCompressionErrorCode> for ProgramError { |
| accounts: &[AccountInfo], | ||
| instruction_data: &[u8], | ||
| ) -> Result<(), ProgramError> { | ||
| msg!(format!("instruction_data: {:?}", instruction_data[..8].to_vec()).as_str()); |
There was a problem hiding this comment.
is there a reason why we log 0..8 bytes?
There was a problem hiding this comment.
the first 8 are the discriminator, this comment is probably a debug leftover.
There was a problem hiding this comment.
debugging pinocchio programs is really awful you get very little feedback from inner cpis
SwenSchaeferjohann
left a comment
SwenSchaeferjohann
left a comment
There was a problem hiding this comment.
lgtm, just adding some clarifying questions / leftovers
| msg!(format!("instruction_data: {:?}", instruction_data[..8].to_vec()).as_str()); | ||
| let discriminator = InstructionType::try_from(instruction_data[0]).unwrap(); | ||
| msg!(format!("instruction_data: {:?}", instruction_data[..8].to_vec()).as_str()); |
There was a problem hiding this comment.
(duplicate msg, and u8 discriminator)
| msg!(format!("instruction_data: {:?}", instruction_data[..8].to_vec()).as_str()); | |
| let discriminator = InstructionType::try_from(instruction_data[0]).unwrap(); | |
| msg!(format!("instruction_data: {:?}", instruction_data[..8].to_vec()).as_str()); | |
| msg!(format!("discriminator: {:?}", instruction_data[..1].to_vec()).as_str()); | |
| let discriminator = InstructionType::try_from(instruction_data[0]).unwrap(); |
There was a problem hiding this comment.
ty, I will remove these prints.
ananas-block
force-pushed
the
jorrit/refactor-pinocchio-sdk
branch
2 times, most recently
from
37859af to
c5f63c0
Compare
|
based on #1782 we should merge it first |
|
something like let (address, address_seed) = derive_address(
&[b"counter", ctx.accounts.fee_payer.key().as_ref()],
&light_cpi_accounts.tree_accounts()
[address_tree_info.address_merkle_tree_pubkey_index as usize]
.key(),
&crate::ID,
);into (A) let (address, address_seed) = derive_address(
&[b"counter", ctx.accounts.fee_payer.key().as_ref()],
address_tree_info.get_tree_pubkey(&light_cpi_accounts),
&crate::ID,
);or (B) let (address, address_seed) = derive_address(
&[b"counter", ctx.accounts.fee_payer.key().as_ref()],
address_tree_info.get_tree_pubkey(&light_cpi_accounts.tree_accounts()),
&crate::ID,
);(A) would be more restrictive but easier to use since we can expect consumers to always have a wdyt @ananas-block |
agreed on A to avoid the footgun |
ananas-block
force-pushed
the
jorrit/refactor-pinocchio-sdk
branch
2 times, most recently
from
fa89709 to
8a7211f
Compare
|
rebased on #1795 |
sdk-libs/sdk/src/cpi/accounts.rs
Outdated
| account_metas.push(AccountMeta { | ||
| pubkey: *acc.key, | ||
| is_signer: false, | ||
| is_writable: true, |
There was a problem hiding this comment.
make mutability variable
| config: CpiAccountsConfig, | ||
| ) -> Result<Self> { | ||
| msg!("config {:?}", config); | ||
| solana_msg::msg!("config {:?}", config); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
sdk-libs/sdk/src/instruction/tree_info.rs (1)
52-60: 🛠️ Refactor suggestionGuard against slice length mismatches
address_tree_infos.iter().zip(root_index)will silently truncate to the shorter of the two slices.
A forgotten or off-by-one entry inroot_indexwill therefore go unnoticed and corrupt the packing order instead of exploding fast.Consider an explicit length check (even a
debug_assert!) before zipping:pub fn pack_address_tree_infos<'a>( address_tree_infos: &'a [AddressTreeInfo], root_index: &'a [u16], remaining_accounts: &'a mut PackedAccounts, ) -> impl Iterator<Item = PackedAddressTreeInfo> + 'a { - address_tree_infos - .iter() - .zip(root_index) + debug_assert_eq!( + address_tree_infos.len(), + root_index.len(), + "address_tree_infos and root_index must have equal length" + ); + address_tree_infos + .iter() + .zip(root_index.iter()) .map(move |(x, root_index)| pack_address_tree_info(x, remaining_accounts, *root_index)) }
♻️ Duplicate comments (4)
sdk-libs/sdk/src/cpi/invoke.rs (1)
138-142: 🛠️ Refactor suggestionApply the same optimization to avoid cloning.
let account_info_refs = light_system_accounts.to_account_infos(); -let account_infos: Vec<AccountInfo> = account_info_refs.into_iter().cloned().collect(); let bump = light_system_accounts.bump(); let account_metas: Vec<AccountMeta> = to_account_metas(light_system_accounts);And update the invoke call:
-invoke_light_system_program(account_infos.as_slice(), instruction, bump) +invoke_light_system_program(&account_info_refs, instruction, bump)sdk-libs/sdk-types/src/cpi_accounts.rs (2)
55-55:⚠️ Potential issueFix typo in enum variant name.
- DecompressionRecipient, + DecompressionRecipient,
68-83:⚠️ Potential issueAdd bounds validation to prevent potential panics.
The constructors should validate that the accounts slice has sufficient length to prevent panics in accessor methods.
impl<'a, T: AccountInfoTrait> CpiAccounts<'a, T> { pub fn new(fee_payer: &'a T, accounts: &'a [T], cpi_signer: CpiSigner) -> Self { + assert!( + accounts.len() >= SYSTEM_ACCOUNTS_LEN, + "Insufficient accounts provided: expected at least {}, got {}", + SYSTEM_ACCOUNTS_LEN, + accounts.len() + ); Self { fee_payer, accounts, config: CpiAccountsConfig::new(cpi_signer), } } pub fn new_with_config(fee_payer: &'a T, accounts: &'a [T], config: CpiAccountsConfig) -> Self { + let required_len = SYSTEM_ACCOUNTS_LEN + - (!config.sol_pool_pda as usize) + - (!config.sol_compression_recipient as usize) + - (!config.cpi_context as usize); + assert!( + accounts.len() >= required_len, + "Insufficient accounts provided: expected at least {}, got {}", + required_len, + accounts.len() + ); Self { fee_payer, accounts, config, } }sdk-libs/sdk-pinocchio/src/cpi/invoke.rs (1)
120-122:⚠️ Potential issueFix typo in function name.
pub fn invoke_light_system_program(self, cpi_accounts: CpiAccounts) -> Result<()> { - light_system_progam_instruction_invoke_cpi(self, &cpi_accounts) + light_system_program_instruction_invoke_cpi(self, &cpi_accounts) } -pub fn light_system_progam_instruction_invoke_cpi( +pub fn light_system_program_instruction_invoke_cpi(Also applies to: 125-125
🧹 Nitpick comments (1)
sdk-libs/sdk/src/instruction/tree_info.rs (1)
49-57: Doc comment driftThe docstring still mentions
merkle_contexts, but the parameter is nowaddress_tree_infos.
Updating the wording will avoid confusion for consumers of the API.
📜 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 (25)
examples/anchor/counter/Cargo.toml(1 hunks)examples/anchor/token-escrow/Cargo.toml(1 hunks)forester-utils/src/registry.rs(1 hunks)program-tests/client-test/Cargo.toml(1 hunks)program-tests/create-address-test-program/Cargo.toml(1 hunks)program-tests/create-address-test-program/src/lib.rs(2 hunks)program-tests/sdk-anchor-test/programs/sdk-anchor-test/Cargo.toml(1 hunks)program-tests/sdk-pinocchio-test/Cargo.toml(1 hunks)program-tests/system-cpi-test/Cargo.toml(1 hunks)program-tests/system-cpi-v2-test/Cargo.toml(1 hunks)programs/account-compression/src/errors.rs(1 hunks)sdk-libs/macros/src/cpi_signer.rs(1 hunks)sdk-libs/program-test/src/accounts/initialize.rs(1 hunks)sdk-libs/program-test/src/accounts/state_tree.rs(1 hunks)sdk-libs/program-test/src/accounts/state_tree_v2.rs(2 hunks)sdk-libs/program-test/src/accounts/test_accounts.rs(2 hunks)sdk-libs/program-test/src/utils/setup_light_programs.rs(3 hunks)sdk-libs/sdk-pinocchio/src/cpi/invoke.rs(1 hunks)sdk-libs/sdk-types/src/constants.rs(2 hunks)sdk-libs/sdk-types/src/cpi_accounts.rs(1 hunks)sdk-libs/sdk-types/src/cpi_accounts_small.rs(1 hunks)sdk-libs/sdk-types/src/instruction/account_info.rs(7 hunks)sdk-libs/sdk/src/cpi/invoke.rs(6 hunks)sdk-libs/sdk/src/instruction/system_accounts.rs(2 hunks)sdk-libs/sdk/src/instruction/tree_info.rs(5 hunks)
✅ Files skipped from review due to trivial changes (7)
- examples/anchor/token-escrow/Cargo.toml
- program-tests/system-cpi-test/Cargo.toml
- program-tests/sdk-anchor-test/programs/sdk-anchor-test/Cargo.toml
- examples/anchor/counter/Cargo.toml
- programs/account-compression/src/errors.rs
- sdk-libs/sdk/src/instruction/system_accounts.rs
- sdk-libs/sdk-types/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- sdk-libs/program-test/src/accounts/initialize.rs
- program-tests/create-address-test-program/Cargo.toml
- program-tests/client-test/Cargo.toml
- sdk-libs/program-test/src/accounts/state_tree.rs
- forester-utils/src/registry.rs
- sdk-libs/program-test/src/accounts/state_tree_v2.rs
- sdk-libs/program-test/src/accounts/test_accounts.rs
- sdk-libs/program-test/src/utils/setup_light_programs.rs
- program-tests/create-address-test-program/src/lib.rs
- program-tests/sdk-pinocchio-test/Cargo.toml
- sdk-libs/macros/src/cpi_signer.rs
- sdk-libs/sdk-types/src/instruction/account_info.rs
- sdk-libs/sdk-types/src/cpi_accounts_small.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
sdk-libs/sdk-types/src/cpi_accounts.rs (4)
sdk-libs/sdk-types/src/cpi_accounts_small.rs (9)
new(29-35)fee_payer(45-47)new_with_config(37-43)config(60-62)authority(49-54)self_program_id(56-58)account_infos(78-80)tree_accounts(82-84)to_account_infos(87-94)program-libs/account-checks/src/account_info/account_info_trait.rs (1)
pubkey_from_bytes(33-33)program-libs/account-checks/src/account_info/solana.rs (1)
pubkey_from_bytes(24-26)program-libs/account-checks/src/account_info/pinocchio.rs (1)
pubkey_from_bytes(18-20)
sdk-libs/sdk-pinocchio/src/cpi/invoke.rs (6)
js/stateless.js/src/state/types.ts (5)
CompressedAccount(55-67)CompressedAccountData(78-82)PackedCompressedAccountWithMerkleContext(32-37)OutputCompressedAccountWithPackedContext(73-76)InstructionDataInvokeCpi(111-120)js/stateless.js/src/utils/address.ts (1)
NewAddressParamsPacked(64-83)sdk-libs/sdk-types/src/instruction/account_info.rs (4)
input_compressed_account(72-75)input_compressed_account(173-198)output_compressed_account(76-79)output_compressed_account(200-223)sdk-libs/sdk-types/src/cpi_accounts.rs (5)
new(18-25)new(69-75)account_infos(136-138)bump(40-42)bump(114-116)sdk-libs/sdk-pinocchio/src/cpi/accounts.rs (2)
to_account_metas(9-85)to_account_infos_for_invoke(87-111)sdk-libs/sdk-pinocchio/src/error.rs (4)
from(78-80)from(84-86)from(90-110)from(114-149)
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: Test program-libs
- GitHub Check: Test sdk-libs
- GitHub Check: lint
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: stateless-js
- GitHub Check: system-programs (counter-test, ["cargo test-sbf -p counter"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-test"])
- GitHub Check: system-programs (token-escrow-test, [ "cargo test-sbf -p token-escrow" ])
- GitHub Check: test-state-batched
- GitHub Check: test-state-photon-batched
- GitHub Check: test-2-foresters
- GitHub Check: test-e2e
- GitHub Check: test-address-batched
- GitHub Check: test-double-registration
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
program-tests/system-cpi-v2-test/Cargo.toml (1)
34-34: Addlight-sdk-typesto non-Solana dependencies
Includinglight-sdk-typeshere ensures that this test harness can leverage the shared types crate on non-Solana targets, matching the overall SDK refactor.sdk-libs/sdk/src/instruction/tree_info.rs (1)
71-74: Inconsistentinsert_or_getkey typeElsewhere in this file (
pack_merkle_context) you call
insert_or_get(merkle_tree_pubkey.to_bytes().into()), while here you pass thePubkeyvalue directly.
IfPackedAccounts::insert_or_getrelies on concrete key-type equality, this inconsistency may either fail to deduplicate identical keys or break compilation once the signature changes.Double-check that
insert_or_getis generic overInto<[u8; 32]>(or similar) and consider using the same call pattern everywhere for clarity:- let address_merkle_tree_pubkey_index = remaining_accounts.insert_or_get(*tree); - let address_queue_pubkey_index = remaining_accounts.insert_or_get(*queue); + let address_merkle_tree_pubkey_index = + remaining_accounts.insert_or_get(tree.to_bytes().into()); + let address_queue_pubkey_index = + remaining_accounts.insert_or_get(queue.to_bytes().into());sdk-libs/sdk/src/cpi/invoke.rs (1)
10-10: LGTM! Consistent standardization across the SDK.The changes properly standardize the use of constants from
light_sdk_typesand align with the updated trait definitions requiring byte array conversions.Also applies to: 73-82, 117-120
sdk-libs/sdk-types/src/cpi_accounts.rs (1)
1-154: Well-structured CPI accounts abstraction.The overall design is clean and provides a good abstraction for managing CPI accounts. Once the typo and bounds validation issues are resolved, this will be a solid foundation for the SDK.
sdk-libs/sdk-pinocchio/src/cpi/invoke.rs (2)
22-86: Well-implemented extension trait pattern.The
CompressedAccountInfoExttrait cleanly extends the functionality ofCompressedAccountInfowithout modifying the original type. The implementation properly handles optional fields and maintains type safety.
177-206: Good performance monitoring and error handling.The strategic placement of
sol_log_compute_units()calls will help identify performance bottlenecks. The error handling with detailed logging provides good debugging information.
| pub fn invoke_light_system_program(self, cpi_accounts: CpiAccounts) -> Result<()> { | ||
| let instruction = create_light_system_progam_instruction_invoke_cpi(self, &cpi_accounts)?; | ||
|
|
||
| invoke_light_system_program( | ||
| cpi_accounts.self_program_id(), | ||
| cpi_accounts.to_account_infos().as_slice(), | ||
| instruction, | ||
| ) | ||
| let bump = cpi_accounts.bump(); | ||
| let account_info_refs = cpi_accounts.to_account_infos(); | ||
| let instruction = create_light_system_progam_instruction_invoke_cpi(self, cpi_accounts)?; | ||
| let account_infos: Vec<AccountInfo> = account_info_refs.into_iter().cloned().collect(); | ||
| invoke_light_system_program(account_infos.as_slice(), instruction, bump) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider avoiding unnecessary cloning of account infos.
The explicit cloning of account infos could impact performance. Since to_account_infos() already returns owned references, you could avoid the clone operation.
pub fn invoke_light_system_program(self, cpi_accounts: CpiAccounts) -> Result<()> {
let bump = cpi_accounts.bump();
let account_info_refs = cpi_accounts.to_account_infos();
- let instruction = create_light_system_progam_instruction_invoke_cpi(self, cpi_accounts)?;
- let account_infos: Vec<AccountInfo> = account_info_refs.into_iter().cloned().collect();
- invoke_light_system_program(account_infos.as_slice(), instruction, bump)
+ let instruction = create_light_system_progam_instruction_invoke_cpi(self, cpi_accounts)?;
+ invoke_light_system_program(&account_info_refs, instruction, bump)
}Note: This requires updating invoke_light_system_program to accept &[&AccountInfo] instead of &[AccountInfo].
📝 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 fn invoke_light_system_program(self, cpi_accounts: CpiAccounts) -> Result<()> { | |
| let instruction = create_light_system_progam_instruction_invoke_cpi(self, &cpi_accounts)?; | |
| invoke_light_system_program( | |
| cpi_accounts.self_program_id(), | |
| cpi_accounts.to_account_infos().as_slice(), | |
| instruction, | |
| ) | |
| let bump = cpi_accounts.bump(); | |
| let account_info_refs = cpi_accounts.to_account_infos(); | |
| let instruction = create_light_system_progam_instruction_invoke_cpi(self, cpi_accounts)?; | |
| let account_infos: Vec<AccountInfo> = account_info_refs.into_iter().cloned().collect(); | |
| invoke_light_system_program(account_infos.as_slice(), instruction, bump) | |
| } | |
| pub fn invoke_light_system_program(self, cpi_accounts: CpiAccounts) -> Result<()> { | |
| let bump = cpi_accounts.bump(); | |
| let account_info_refs = cpi_accounts.to_account_infos(); | |
| let instruction = create_light_system_progam_instruction_invoke_cpi(self, cpi_accounts)?; | |
| invoke_light_system_program(&account_info_refs, instruction, bump) | |
| } |
🤖 Prompt for AI Agents
In sdk-libs/sdk/src/cpi/invoke.rs around lines 53 to 59, avoid cloning
AccountInfo objects by changing the type passed to invoke_light_system_program
from a slice of owned AccountInfo to a slice of references to AccountInfo.
Update invoke_light_system_program to accept a slice of references
(&[&AccountInfo]) instead of owned AccountInfo (&[AccountInfo]), and pass the
references directly from to_account_infos() without cloning.
Reverts only the changes made to the programs/ directory in commit b3f14e7 while preserving all other changes from that commit. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
program-tests/account-compression-test/tests/address_merkle_tree_tests.rs(1 hunks)program-tests/account-compression-test/tests/merkle_tree_tests.rs(6 hunks)sdk-libs/sdk/src/address.rs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- program-tests/account-compression-test/tests/address_merkle_tree_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- program-tests/account-compression-test/tests/merkle_tree_tests.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
sdk-libs/sdk/src/address.rs (4)
js/stateless.js/src/utils/address.ts (2)
NewAddressParams(43-62)NewAddressParamsPacked(64-83)sdk-libs/sdk/src/account.rs (1)
address(217-219)program-tests/utils/src/e2e_test_env.rs (2)
seeds(2557-2568)seeds(2581-2597)program-libs/compressed-account/src/address.rs (1)
derive_address(30-41)
⏰ Context from checks skipped due to timeout of 90000ms (23)
- 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-test"])
- GitHub Check: system-programs (token-escrow-test, [ "cargo test-sbf -p token-escrow" ])
- GitHub Check: system-programs (counter-test, ["cargo test-sbf -p counter"])
- GitHub Check: Test program-libs
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: test-2-foresters
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-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-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: test-e2e
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: test-state-batched
- GitHub Check: test-address-batched
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: stateless-js
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
sdk-libs/sdk/src/address.rs (4)
38-45: Clean type re-exports with helpful documentation.The re-exports are well-organized and the comment explaining
PackedNewAddressParamsusage provides good context for developers.
64-96: Clean delegation to centralized address derivation logic.The functions correctly delegate to
light_sdk_types::address::v1while maintaining the same public API. Theto_bytes()conversions are properly handled.
103-104: Test imports and field names correctly updated.The tests properly reflect the API changes with updated imports and the new field names (
treeandqueue) from the refactoredAddressTreeInfostruct.Also applies to: 132-134
146-146: Test function calls correctly updated to match new API.The
derive_addresscalls properly useaddress_tree_info.treeto match the updated function signature.Also applies to: 159-159
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
sdk-libs/sdk/src/cpi/accounts.rs (1)
58-65: Consider making account mutability configurable.The
is_writablefield is hardcoded totruefor conditional accounts (sol_pool_pda, sol_compression_recipient, cpi_context). This should be determined by the account's actual mutability status to avoid potential issues with read-only accounts being marked as writable.Also applies to: 70-77, 89-96
sdk-libs/sdk/src/error.rs (1)
86-110:From<LightSdkTypesError>again duplicatedThis conversion arm is an exact clone of the one in
sdk-pinocchio. Please centralise viaimpl_from_light_sdk_types_error!macro to avoid rot.sdk-libs/sdk-pinocchio/src/cpi/accounts.rs (1)
48-50:⚠️ Potential issueReplace hardcoded zero pubkey with actual System Program ID.
The
[0u8; 32]is not a valid program ID. This should use the actual system program pubkey.Apply this diff to fix the issue:
- // System program - use default (all zeros) - account_metas.push(AccountMeta::readonly(&[0u8; 32])); - current_index += 1; + // System program + account_metas.push(AccountMeta::readonly(cpi_accounts.system_program()?.key())); + current_index += 1;Also update
to_account_infos_for_invoketo properly handle the system program account:current_index += 1; - // system program - current_index += 1; + // system program + account_infos.insert(current_index, cpi_accounts.system_program()?); + current_index += 1;sdk-libs/sdk-types/src/cpi_accounts_small.rs (2)
31-46:⚠️ Potential issueAdd validation for minimum accounts slice size.
The constructors should validate that the accounts slice has sufficient elements to prevent panics in methods that access specific indices.
Apply this diff to add validation:
pub fn new(fee_payer: &'a T, accounts: &'a [T], cpi_signer: CpiSigner) -> Self { + // Ensure accounts slice has at least the required program accounts + assert!( + accounts.len() >= PROGRAM_ACCOUNTS_LEN, + "Accounts slice must have at least {} elements", + PROGRAM_ACCOUNTS_LEN + ); Self { fee_payer, accounts, config: CpiAccountsConfig::new(cpi_signer), } } pub fn new_with_config(fee_payer: &'a T, accounts: &'a [T], config: CpiAccountsConfig) -> Self { + // Ensure accounts slice has at least the required program accounts + assert!( + accounts.len() >= PROGRAM_ACCOUNTS_LEN, + "Accounts slice must have at least {} elements", + PROGRAM_ACCOUNTS_LEN + ); Self { fee_payer, accounts, config, } }
145-156:⚠️ Potential issuePotential panic from unchecked slice operations.
The slice operations
self.accounts[PROGRAM_ACCOUNTS_LEN..]could panic if the accounts slice is smaller thanPROGRAM_ACCOUNTS_LEN.These methods rely on the constructors to validate the minimum slice size. Once constructor validation is added (as suggested above), these operations will be safe. Alternatively, use checked slicing:
pub fn to_account_infos(&self) -> Vec<&'a T> { let mut account_infos = Vec::with_capacity(1 + self.accounts.len() - PROGRAM_ACCOUNTS_LEN); account_infos.push(self.fee_payer()); - self.accounts[PROGRAM_ACCOUNTS_LEN..] - .iter() - .for_each(|acc| account_infos.push(acc)); + if let Some(slice) = self.accounts.get(PROGRAM_ACCOUNTS_LEN..) { + slice.iter().for_each(|acc| account_infos.push(acc)); + } account_infos } pub fn account_infos_slice(&self) -> &[T] { - &self.accounts[PROGRAM_ACCOUNTS_LEN..] + self.accounts.get(PROGRAM_ACCOUNTS_LEN..).unwrap_or(&[]) }
🧹 Nitpick comments (7)
sdk-libs/client/src/rpc/client.rs (1)
696-705: Good error handling, but consider refactoring to reduce duplication.The method now properly returns an error when no state trees are available. However, the selection logic is duplicated in the new
select_state_tree_infofunction below. Consider refactoring this method to use the new helper function.fn get_random_state_tree_info(&self) -> Result<TreeInfo, RpcError> { - if self.state_merkle_trees.is_empty() { - return Err(RpcError::NoStateTreesAvailable); - } - - use rand::Rng; - let mut rng = rand::thread_rng(); - - Ok(self.state_merkle_trees[rng.gen_range(0..self.state_merkle_trees.len())]) + let mut rng = rand::thread_rng(); + select_state_tree_info(&mut rng, &self.state_merkle_trees) }sdk-libs/sdk/src/cpi/mod.rs (1)
51-57:accounts_small_ixre-export guarded by new feature flag – update docs/examples accordinglyThe module gating switched from
v2tosmall_ix, but the top-level doctest block and several example programs in this PR still reference the legacyv2feature. Readers who follow the docs will end up with missing-module errors.Consider adding a short note to the module-level docs (or updating the examples) to reflect the new
--features small_ixflag.sdk-libs/sdk-pinocchio/src/error.rs (1)
118-156: Another copy–paste numeric map – prone to driftSame comment as for
LightSdkTypesError: the 50-linematchhere is copy-pasted fromsdk/error.rs. Divergence is inevitable. Suggest generating theu32mapping via anum_enum::IntoPrimitivederive or a small macro that lives inlight-sdk-typesso every crate consumes the same definition.sdk-libs/sdk/src/error.rs (1)
113-150: Hard-coded numeric table duplicatessdk-pinocchio– extract or generateSame maintenance concern: three hand-written tables must stay in sync. Moving the canonical list to
light-sdk-typesbehindnum_enum::IntoPrimitive(or a build-time code-gen script) would remove the risk entirely.sdk-libs/sdk-types/src/instruction/tree_info.rs (1)
7-14:PackedStateTreeInfoappears unusedThe struct is defined but never referenced in this crate after the recent refactor.
If it is truly deprecated, remove it or gate it behind#[deprecated]/#[allow(dead_code)]to avoid dead-code warnings.sdk-libs/sdk/src/cpi/accounts_small_ix.rs (1)
11-11: Consider tracking the const array optimization as a GitHub issue.The TODO comment indicates a potential performance optimization. Consider creating a GitHub issue to track this enhancement if the heap allocation becomes a performance bottleneck.
Would you like me to open an issue to track this optimization opportunity?
program-tests/sdk-pinocchio-test/tests/test.rs (1)
45-55: Remove debug print statements before merging.Debug prints should be removed or converted to proper logging if needed for production code.
Apply this diff to remove the debug statements:
- println!("seed {:?}", address_seed); let address = derive_address( &address_seed, &address_tree_pubkey.to_bytes(), &sdk_pinocchio_test::ID, ); - println!("address {:?}", address); - println!("address tree pubkey: {:?}", address_tree_pubkey.to_bytes()); let output_queue = rpc.get_random_state_tree_info().unwrap().queue; - println!("output_queue tree pubkey: {:?}", output_queue.to_bytes());
📜 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 (28)
examples/anchor/counter/src/lib.rs(6 hunks)examples/anchor/counter/tests/test.rs(2 hunks)program-tests/create-address-test-program/src/lib.rs(2 hunks)program-tests/sdk-anchor-test/programs/sdk-anchor-test/src/lib.rs(5 hunks)program-tests/sdk-anchor-test/programs/sdk-anchor-test/tests/test.rs(6 hunks)program-tests/sdk-pinocchio-test/src/create_pda.rs(1 hunks)program-tests/sdk-pinocchio-test/tests/test.rs(1 hunks)program-tests/sdk-test/src/create_pda.rs(3 hunks)program-tests/sdk-test/tests/test.rs(2 hunks)sdk-libs/client/src/rpc/client.rs(4 hunks)sdk-libs/client/src/rpc/errors.rs(2 hunks)sdk-libs/client/src/rpc/rpc_trait.rs(1 hunks)sdk-libs/program-test/src/program_test/rpc.rs(4 hunks)sdk-libs/sdk-pinocchio/src/cpi/accounts.rs(1 hunks)sdk-libs/sdk-pinocchio/src/cpi/accounts_small.rs(1 hunks)sdk-libs/sdk-pinocchio/src/cpi/invoke.rs(1 hunks)sdk-libs/sdk-pinocchio/src/error.rs(1 hunks)sdk-libs/sdk-types/src/cpi_accounts.rs(1 hunks)sdk-libs/sdk-types/src/cpi_accounts_small.rs(1 hunks)sdk-libs/sdk-types/src/error.rs(1 hunks)sdk-libs/sdk-types/src/instruction/tree_info.rs(1 hunks)sdk-libs/sdk/src/address.rs(6 hunks)sdk-libs/sdk/src/cpi/accounts.rs(1 hunks)sdk-libs/sdk/src/cpi/accounts_small_ix.rs(1 hunks)sdk-libs/sdk/src/cpi/invoke.rs(5 hunks)sdk-libs/sdk/src/cpi/mod.rs(1 hunks)sdk-libs/sdk/src/error.rs(3 hunks)sdk-libs/sdk/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- program-tests/sdk-test/tests/test.rs
- program-tests/sdk-test/src/create_pda.rs
- program-tests/create-address-test-program/src/lib.rs
- sdk-libs/sdk-pinocchio/src/cpi/accounts_small.rs
- examples/anchor/counter/src/lib.rs
- examples/anchor/counter/tests/test.rs
- sdk-libs/sdk/src/cpi/invoke.rs
- program-tests/sdk-pinocchio-test/src/create_pda.rs
- sdk-libs/sdk/src/address.rs
- sdk-libs/sdk-pinocchio/src/cpi/invoke.rs
- program-tests/sdk-anchor-test/programs/sdk-anchor-test/tests/test.rs
- sdk-libs/sdk-types/src/cpi_accounts.rs
- program-tests/sdk-anchor-test/programs/sdk-anchor-test/src/lib.rs
- sdk-libs/program-test/src/program_test/rpc.rs
🧰 Additional context used
🧬 Code Graph Analysis (7)
sdk-libs/client/src/rpc/rpc_trait.rs (2)
sdk-libs/client/src/rpc/client.rs (1)
get_random_state_tree_info(696-705)sdk-libs/program-test/src/program_test/rpc.rs (1)
get_random_state_tree_info(269-290)
sdk-libs/sdk-pinocchio/src/error.rs (2)
sdk-libs/sdk-types/src/error.rs (1)
from(35-50)sdk-libs/sdk/src/error.rs (3)
from(81-83)from(87-110)from(114-150)
sdk-libs/sdk/src/cpi/accounts.rs (3)
sdk-libs/sdk-pinocchio/src/cpi/accounts.rs (1)
to_account_metas(9-75)sdk-libs/sdk-types/src/cpi_accounts_small.rs (1)
tree_accounts(126-133)sdk-libs/sdk-types/src/cpi_accounts.rs (1)
tree_accounts(205-210)
sdk-libs/sdk/src/lib.rs (3)
sdk-libs/sdk/src/account.rs (3)
account(240-240)address(217-219)discriminator(193-195)sdk-libs/macros/src/discriminator.rs (1)
discriminator(6-25)program-libs/account-checks/src/discriminator.rs (1)
discriminator(6-8)
sdk-libs/sdk-types/src/error.rs (2)
sdk-libs/sdk-pinocchio/src/error.rs (4)
from(80-82)from(86-88)from(92-115)from(119-155)sdk-libs/sdk/src/error.rs (3)
from(81-83)from(87-110)from(114-150)
sdk-libs/sdk-pinocchio/src/cpi/accounts.rs (2)
sdk-libs/sdk/src/cpi/accounts.rs (1)
to_account_metas(11-109)sdk-libs/sdk-types/src/cpi_accounts.rs (2)
tree_accounts(205-210)account_infos(195-197)
sdk-libs/sdk-types/src/cpi_accounts_small.rs (1)
sdk-libs/sdk-types/src/cpi_accounts.rs (18)
new(21-28)new(72-78)fee_payer(88-90)cpi_signer(39-41)new_with_config(80-86)config(177-179)authority(99-104)registered_program_pda(113-118)account_compression_authority(127-132)sol_pool_pda(141-146)decompression_recipient(148-153)cpi_context(162-167)self_program_id(169-171)account_infos(195-197)get_account_info(199-203)tree_accounts(205-210)get_tree_account_info(212-219)to_account_infos(222-229)
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: test-double-registration
- GitHub Check: test-address-batched
- GitHub Check: test-state-photon-batched
- GitHub Check: test-2-foresters
- GitHub Check: test-state-batched
- GitHub Check: test-e2e
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-test"])
- GitHub Check: system-programs (token-escrow-test, [ "cargo test-sbf -p token-escrow" ])
- 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", "cargo-test-sbf -...
- GitHub Check: stateless-js
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: Test sdk-libs
- GitHub Check: lint
- 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 (light-registry, ["cargo-test-sbf -p registry-test"])
🔇 Additional comments (12)
sdk-libs/sdk/src/cpi/accounts.rs (1)
1-10: LGTM! Clean refactoring to use generic types.The migration to use type aliases from
light_sdk_typesimproves code reusability and maintains a cleaner separation of concerns.sdk-libs/sdk/src/lib.rs (2)
1-103: Excellent documentation and code example!The comprehensive crate-level documentation clearly explains the SDK's core functionality and features. The example code effectively demonstrates the new API patterns, including the improved
get_tree_pubkeymethod usage that was discussed in the PR comments.
123-134: Well-organized module exports and re-exports.The restructured exports provide a cleaner public API with:
- Clear re-exports from
light_sdk_macrosandlight_sdk_types- Proper trait visibility (
LightDiscriminator)- Organized constants module
The removal of
ValidityProofaligns with its relocation to the compressed-account crate.sdk-libs/client/src/rpc/errors.rs (1)
55-59: Good error handling enhancement.The new
NoStateTreesAvailableerror variant provides clear feedback when no state trees are available, with a helpful message suggesting the appropriate method to fetch them. The Clone implementation is correctly updated.Also applies to: 85-85
sdk-libs/client/src/rpc/rpc_trait.rs (1)
196-198: Good API improvement with explicit error handling.Changing the return type to
Result<TreeInfo, RpcError>makes the API more robust by explicitly handling the case when no state trees are available. This is a breaking change but improves safety.sdk-libs/client/src/rpc/client.rs (1)
720-751: Well-documented utility function with good API design.The
select_state_tree_infofunction is well-designed with:
- Clear documentation including usage example
- Generic RNG parameter for flexibility
- Proper error handling
This provides a reusable utility that can be used both internally and by external consumers.
sdk-libs/sdk-types/src/error.rs (1)
34-50: Potential trait-bound breakage on future hasher changesDeriving
PartialEqfor the enum requires every inner type to implementPartialEq; this currently transits throughHasherError.
ShouldHasherErrorever dropPartialEq, compilation of every dependent crate will fail.If equality on hashing errors is not business-critical, consider
#[cfg_attr(test, derive(PartialEq))]or manualimpl PartialEqthat treats theHashervariant as always unequal to avoid propagating upstream trait requirements.sdk-libs/sdk/src/error.rs (1)
52-75: Zero-copy variant sneaked in without code-mapping tests
ZeroCopy(#[from] ZeroCopyError)is great, but the numeric translation relies onZeroCopyError: Into<u32>.
Add a compile-time assertion or unit test thatu32::from(ZeroCopyError::SomeVariant)aligns with Solana’szero_copyerror table to avoid silently mismatched codes.sdk-libs/sdk-types/src/instruction/tree_info.rs (1)
32-40: Nice: bounds-checked lookup replaces previous panic path
get_tree_account_info()returns aResult, so the earlier OOB panic risk is gone.
LGTM.sdk-libs/sdk/src/cpi/accounts_small_ix.rs (1)
10-85: Well-structured CPI account meta conversion implementation.The function correctly handles optional accounts based on configuration flags and includes proper error handling for out-of-bounds access. The assertion at line 69 provides good validation of the index calculation.
sdk-libs/sdk-pinocchio/src/cpi/accounts.rs (1)
9-75: Consistent CPI account meta handling with proper ordering.The function correctly manages account ordering and mutability based on configuration flags, ensuring proper CPI account metadata for program invocation. The pattern matches the implementation in the main SDK.
program-tests/sdk-pinocchio-test/tests/test.rs (1)
23-206: Comprehensive integration test coverage for PDA operations.The test thoroughly exercises the create and update flows for compressed PDAs, with proper verification of the created account. The helper functions are well-structured and handle CPI account construction correctly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
program-tests/sdk-pinocchio-test/tests/test.rs (2)
147-150: Multipleunwrap()s can panic the test unnecessarily
compressed_account.hash().unwrap(),.state_trees.unwrap(),.data.unwrap(), andtry_into().unwrap()will panic on unexpected runtime conditions, turning a logic error into a hard crash that masks useful context.Consider switching to
expect()with a descriptive message, or bubbling the error with?so that theRpcErroris propagated:let leaf_hash = compressed_account .hash() .expect("compressed_account missing hash");This keeps the test expressive while avoiding opaque panics.
Also applies to: 178-187
1-1: Confirm the test-suite gatingThe crate-level attribute
#![cfg(feature = "test-sbf")]is currently commented out.
If the intention is to run this test only under thetest-sbffeature (e.g. SBF cross-compilation), uncomment it; otherwise remove the dead comment to avoid confusion.sdk-libs/sdk/src/error.rs (1)
113-150: Numeric-code table is brittle and already has a “hole” (16030)Manually matching every variant to a hard-coded
u32is error-prone:
- New variants require touching two places (enum + match), easy to forget.
- Code
16030is currently unused – likely an oversight.- A single off-by-one breaks backward-compatibility guarantees.
Consider deriving codes automatically instead (e.g.
#[repr(u32)], a macro, ornum_derive::FromPrimitive) and reserving a range for wrapped foreign errors to keep the mapping maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
program-tests/sdk-pinocchio-test/tests/test.rs(1 hunks)sdk-libs/client/src/rpc/client.rs(4 hunks)sdk-libs/sdk-pinocchio/src/cpi/invoke.rs(1 hunks)sdk-libs/sdk-pinocchio/src/error.rs(1 hunks)sdk-libs/sdk/src/error.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk-libs/client/src/rpc/client.rs
- sdk-libs/sdk-pinocchio/src/error.rs
- sdk-libs/sdk-pinocchio/src/cpi/invoke.rs
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: test-state-photon-batched
- GitHub Check: test-2-foresters
- GitHub Check: test-double-registration
- GitHub Check: test-address-batched
- GitHub Check: test-state-batched
- GitHub Check: test-e2e
- GitHub Check: lint
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (token-escrow-test, [ "cargo test-sbf -p token-escrow" ])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-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", "cargo-test-sbf -...
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: stateless-js
🔇 Additional comments (1)
sdk-libs/sdk/src/error.rs (1)
50-57: Minor grammar nit – indefinite article
"Use an CompressedAccountMeta type"→"Use a CompressedAccountMeta type".
[ suggest_nitpick ]
| use light_sdk_pinocchio::instruction::{account_meta::CompressedAccountMeta, PackedStateTreeInfo}; | ||
| use sdk_pinocchio_test::{ | ||
| create_pda::CreatePdaInstructionData, | ||
| update_pda::{UpdateMyCompressedAccount, UpdatePdaInstructionData}, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Two different CompressedAccountMeta structs seem to be conflated – redundant copy & possible type‐mix-up
light_sdk_meta and meta are both initialised with the identically named CompressedAccountMeta that was imported on line 12 from light_sdk_pinocchio.
Because no alias is used for the light_sdk::instruction::account_meta::CompressedAccountMeta, the first construction already yields the Pinocchio variant, so the “conversion” block (163-174) is just cloning fields into the same type – it does not convert anything and adds needless allocations / boilerplate.
If a real conversion is required, import both structs explicitly and alias them, e.g.:
-use light_sdk_pinocchio::instruction::{account_meta::CompressedAccountMeta, PackedStateTreeInfo};
+use light_sdk::instruction::account_meta::CompressedAccountMeta as LightCompressedAccountMeta;
+use light_sdk_pinocchio::instruction::{
+ account_meta::CompressedAccountMeta as PinocchioCompressedAccountMeta,
+ PackedStateTreeInfo,
+};Then:
-let light_sdk_meta = CompressedAccountMeta { // actually Pinocchio here
+let light_sdk_meta = LightCompressedAccountMeta {
/* … */
};
-// Convert to pinocchio CompressedAccountMeta
-let meta = CompressedAccountMeta {
+// Convert to pinocchio CompressedAccountMeta
+let meta = PinocchioCompressedAccountMeta {
/* … */
};This removes the redundant struct copy and prevents accidental misuse of the wrong struct in future edits.
Also applies to: 157-174
🤖 Prompt for AI Agents
In program-tests/sdk-pinocchio-test/tests/test.rs around lines 12 to 16, the
CompressedAccountMeta struct from light_sdk_pinocchio is used for both
light_sdk_meta and meta variables, causing redundant copying and potential type
confusion. To fix this, explicitly import both CompressedAccountMeta structs
from their respective modules with distinct aliases, then update the code to use
these aliases accordingly. This will ensure proper conversion between different
CompressedAccountMeta types, eliminate unnecessary cloning, and prevent future
mix-ups.
| let output_merkle_tree_index = accounts.insert_or_get(*merkle_tree_pubkey); | ||
| let packed_address_tree_info = rpc_result.pack_tree_infos(&mut accounts).address_trees[0]; | ||
| let (accounts, system_accounts_offset, tree_accounts_offset) = accounts.to_account_metas(); | ||
| let instruction_data = CreatePdaInstructionData { | ||
| proof: rpc_result.proof, | ||
| address_tree_info: packed_address_tree_info, | ||
| data: account_data, | ||
| output_merkle_tree_index, | ||
| system_accounts_offset: system_accounts_offset as u8, | ||
| tree_accounts_offset: tree_accounts_offset as u8, | ||
| }; |
There was a problem hiding this comment.
Casting offsets to u8 risks silent overflow
system_accounts_offset / tree_accounts_offset are obtained as usize and cast directly to u8.
If the accounts vector grows beyond 255 entries the cast will wrap, giving wrong offsets and corrupting the instruction.
- system_accounts_offset: system_accounts_offset as u8,
- tree_accounts_offset: tree_accounts_offset as u8,
+ system_accounts_offset: u8::try_from(system_accounts_offset)
+ .expect("too many system accounts (>255)"),
+ tree_accounts_offset: u8::try_from(tree_accounts_offset)
+ .expect("too many tree accounts (>255)"),Same issue appears in update_pda. Using try_from (or widening the field to u16) prevents silent bugs in larger transactions.
Also applies to: 176-191
🤖 Prompt for AI Agents
In program-tests/sdk-pinocchio-test/tests/test.rs around lines 111 to 121, the
system_accounts_offset and tree_accounts_offset variables are cast directly from
usize to u8, which can cause silent overflow if the values exceed 255. To fix
this, replace the direct casts with a safe conversion using try_from to convert
usize to u8, handling potential errors or panics if the value is too large.
Alternatively, consider changing the field types in CreatePdaInstructionData to
u16 to accommodate larger values safely. Apply the same fix to the similar code
block between lines 176 and 191.
| impl From<LightSdkError> for ProgramError { | ||
| fn from(e: LightSdkError) -> Self { | ||
| ProgramError::Custom(e.into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve inner ProgramError instead of wrapping it again
Wrapping an already-existing ProgramError inside ProgramError::Custom(..) discards its original semantic information (e.g. InvalidAccountData) and inflates the custom-code space.
Prefer forwarding the inner error untouched and reserve the custom path for SDK-specific variants only.
-impl From<LightSdkError> for ProgramError {
- fn from(e: LightSdkError) -> Self {
- ProgramError::Custom(e.into())
- }
-}
+impl From<LightSdkError> for ProgramError {
+ fn from(e: LightSdkError) -> Self {
+ match e {
+ // keep Solana errors intact
+ LightSdkError::ProgramError(inner) => inner,
+ // convert the rest to custom numeric codes
+ other => ProgramError::Custom(u32::from(other)),
+ }
+ }
+}📝 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.
| impl From<LightSdkError> for ProgramError { | |
| fn from(e: LightSdkError) -> Self { | |
| ProgramError::Custom(e.into()) | |
| } | |
| } | |
| impl From<LightSdkError> for ProgramError { | |
| fn from(e: LightSdkError) -> Self { | |
| match e { | |
| // keep Solana errors intact | |
| LightSdkError::ProgramError(inner) => inner, | |
| // convert the rest to custom numeric codes | |
| other => ProgramError::Custom(u32::from(other)), | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In sdk-libs/sdk/src/error.rs around lines 80 to 84, the current implementation
wraps a LightSdkError inside ProgramError::Custom, which loses the original
ProgramError details if LightSdkError already contains one. Modify the From
implementation to check if LightSdkError contains an inner ProgramError and
return it directly without wrapping; only wrap errors that are truly
SDK-specific in ProgramError::Custom. This preserves the original error
semantics and avoids unnecessary custom error inflation.
3 participants
Summary
sdk-typesandsdk-pinocchiocrates for improved SDK architectureCpiSignerstruct instead of rawPubkeyKey Changes
1. Compile-time CPI Signer Macro
derive_light_cpi_signer!macro that computes PDAs at compile-timefind_program_addresscalls, reducing compute costsBefore:
After:
2. New SDK Architecture
sdk-typescrate: Shared types and constants for cross-SDK compatibilitysdk-pinocchiocrate: Specialized SDK for Pinocchio-based programsCpiSignerstruct: Type-safe configuration containing program_id, cpi_signer, and bump3. Example Migration
Summary by CodeRabbit
New Features
RootIndextype for improved compressed account operations.Bug Fixes
Refactor
Tests
Chores