Conversation
ananas-block
requested review from
SwenSchaeferjohann and
sergeytimoshin
as code owners
WalkthroughBroadened CI triggers and test matrices, workspace and dependency rework, added heap accessor and account resize change, many crate-level deprecation allowances, introduced an enhanced program-test logging subsystem (config/decoder/formatter/types) and wiring, updated RPC signing/address-tree APIs, scripts/tests renamed and migration to sdk-native-test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant PT as LightProgramTest
participant LS as LiteSVM
participant Logger as Enhanced Logger
participant File as Log File
Dev->>PT: process_transaction(tx)
alt indexer present
PT->>PT: _send_transaction_with_batched_event(tx)
else no indexer
PT->>PT: transaction_counter += 1
PT->>LS: simulate/process tx
end
LS-->>PT: result + slot + logs
alt enhanced logging enabled
PT->>Logger: log_transaction_enhanced(config, tx, result, sig, slot, counter)
Logger->>File: append formatted log
alt failure & console path
PT->>Logger: log_transaction_enhanced_with_console(..., print_to_console=true)
end
else basic logging
PT->>PT: maybe_print_logs(result, logs)
end
sequenceDiagram
autonumber
participant Client as LightClient (Rpc impl)
participant Tx as Transaction
participant RPC as Cluster RPC
Client->>Client: Transaction::new_with_payer(instrs, Some(payer))
Client->>Tx: try_sign(signers, blockhash)
alt signing error
Client-->>Client: return RpcError::SigningError
else signed
Client->>RPC: send/process_transaction(Tx)
RPC-->>Client: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
ananas-block
force-pushed
the
jorrit/feat-program-test-loggig
branch
from
a905fa0 to
b771b9e
Compare
ananas-block
force-pushed
the
jorrit/feat-program-test-loggig
branch
from
a541565 to
e01c23a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/src/lib.rs (2)
37-43: Compilation/runtime error: missing ErrorCode definition and incorrect error mapping.
ErrorCode::AccountNotEnoughKeysis referenced but not defined/imported; also map it to an AnchorErrorusingerror!()to keep error types consistent.Apply this minimal change:
- .get_tree_pubkey(&light_cpi_accounts) - .map_err(|_| ErrorCode::AccountNotEnoughKeys)?, + .get_tree_pubkey(&light_cpi_accounts) + .map_err(|_| error!(ErrorCode::AccountNotEnoughKeys))?,Add the missing pieces (outside this hunk):
// near the top-level constants pub const NAME_MAX_LEN: usize = 64; #[error_code] pub enum ErrorCode { #[msg("Not enough remaining accounts were provided.")] AccountNotEnoughKeys, #[msg("Name too long.")] NameTooLong, }
106-112: Enforce max length and correct account size forname: String
- Define a constant in
src/lib.rs, e.g.pub const NAME_MAX_LEN: usize = /* your max length */;- In
without_compressed_account, guard the input:pub fn without_compressed_account<'info>( ctx: Context<'_, '_, '_, 'info, WithoutCompressedAccount<'info>>, name: String, ) -> Result<()> { + require!(name.len() <= NAME_MAX_LEN, ErrorCode::NameTooLong); ctx.accounts.my_regular_account.name = name; Ok(()) }- Update the account
spaceto include string length prefix and bytes:#[account( init, seeds = [b"compressed".as_slice(), name.as_bytes()], bump, payer = signer, - space = 8 + 8, + // 8-byte discriminator + 4-byte string length + NAME_MAX_LEN bytes + space = 8 + 4 + NAME_MAX_LEN, )] pub my_regular_account: Account<'info, MyRegularAccount>,These changes prevent runtime failures when writing
name: String.programs/registry/src/lib.rs (1)
336-343: Avoidunwrap()in on-chain paths; use explicit error propagation
- Lines 337–341, 357–361 & 363–367: replace
ctx.accounts…as_ref().unwrap().to_account_info()
with
ctx.accounts…as_ref().ok_or(ProgramError::NotEnoughAccountKeys)?.to_account_info()- Line 500: replace
params.try_to_vec().unwrap()
with
params.try_to_vec().map_err(ProgramError::from)?- Audit any direct
.to_account_info()calls onOption(e.g. around lines 462–475) and propagate errors instead of panickingCargo.toml (1)
188-202: Duplicate dependency pointing to the same path will not resolveBoth create-address-test-program (Line 188) and create-address-program-test (Line 201) point to program-tests/create-address-test-program, but the second uses a different crate key without aliasing via package=. Cargo will fail to resolve unless the inner package name matches the dependency key.
Apply one of:
- If you need an alias, add package= to the second entry:
-create-address-program-test = { path = "program-tests/create-address-test-program", version = "1.0.0" } +create-address-program-test = { package = "create-address-test-program", path = "program-tests/create-address-test-program", version = "1.0.0" }
- If an alias is not required, remove the second entry entirely.
sdk-tests/sdk-native-test/tests/test.rs (1)
48-56: Likely wrong account passed: output queue vs output state tree
create_pdanames the parammerkle_tree_pubkeyand uses it to computeoutput_merkle_tree_index, but the test passes a queue pubkey. This can break instruction account ordering.Use the state tree pubkey:
- let ouput_queue = rpc.get_random_state_tree_info().unwrap().queue; + let output_state_tree = rpc.get_random_state_tree_info().unwrap().tree; @@ - &ouput_queue, + &output_state_tree,Also fix the variable typo (“ouput” -> “output”).
🧹 Nitpick comments (34)
scripts/devenv.sh (1)
90-90: Clean up SBF_OUT_DIR on deactivatePrevent env leakage by unsetting on deactivate (mirrors other exports).
# In deactivate(): unset SBF_OUT_DIRprogram-tests/create-address-test-program/src/lib.rs (1)
3-3: Limit deprecation suppression to a compat featureKeep default builds strict; enable only when Anchor requires realloc compatibility.
-#![allow(deprecated)] +#![cfg_attr(feature = "anchor-realloc-compat", allow(deprecated))]programs/compressed-token/src/lib.rs (1)
1-1: Avoid blanket deprecation suppressionSame rationale as other crates; gate behind a feature to prevent hiding future deprecations.
-#![allow(deprecated)] +#![cfg_attr(feature = "anchor-realloc-compat", allow(deprecated))].github/workflows/light-system-programs-tests.yml (2)
18-18: Broader PR trigger is fine; consider guarding noise.Adding "program-libs/**" will increase CI frequency. If churn becomes high, add paths-ignore (e.g., docs/ README changes) or split into a lighter presubmit to keep runtime under control.
67-67: Extra sub-test added — crate verified; refactor parsing
- Confirmed
light-system-program-pinocchioexists underprograms/systemand is listed in the root workspace.- Replace the comma-based splitting (which can break on embedded commas) with newline-delimited JSON parsing, e.g.:
readarray -t sub_tests < <(jq -r '.[]' <<< '${{ matrix["sub-tests"] }}') for subtest in "${sub_tests[@]}"; do echo "$subtest" RUSTFLAGS="-D warnings" eval "$subtest" … done.github/workflows/rust.yml (2)
7-7: Trigger narrowed to program-libs/ — OK.**Matches the intent to scope this workflow to program-libs. If you still want pushes outside PRs to run, consider guarding the job-level
ifto also allow push events.Apply:
- if: github.event.pull_request.draft == false + if: github.event_name != 'pull_request' || github.event.pull_request.draft == false
53-53: Remove redundant new-unique test
Thenew-uniquefeature is already enabled by the--all-featuresrun, so the standalonecargo test -p light-compressed-account --features new-uniqueduplicates coverage. Remove this line from
.github/workflows/rust.yml.program-libs/heap/src/lib.rs (1)
68-75: New accessor get_used_heap — LGTM; reduce duplication with log_total_heap.Implementation mirrors log_total_heap. Minor refactor to centralize the calculation.
Apply:
@@ #[cfg(target_os = "solana")] - pub fn log_total_heap(&self, msg: &str) -> u64 { - const HEAP_END_ADDRESS: u64 = HEAP_START_ADDRESS as u64 + HEAP_LENGTH as u64; - - let heap_start = unsafe { self.pos() } as u64; - let heap_used = HEAP_END_ADDRESS - heap_start; - msg!("{}: total heap used: {}", msg, heap_used); - heap_used - } + pub fn log_total_heap(&self, msg: &str) -> u64 { + let heap_used = self.get_used_heap(); + msg!("{}: total heap used: {}", msg, heap_used); + heap_used + } @@ #[cfg(target_os = "solana")] - pub fn get_used_heap(&self) -> u64 { + pub fn get_used_heap(&self) -> u64 { const HEAP_END_ADDRESS: u64 = HEAP_START_ADDRESS as u64 + HEAP_LENGTH as u64; let heap_start = unsafe { self.pos() } as u64; let heap_used = HEAP_END_ADDRESS - heap_start; heap_used }Optional: add
#[inline]and a brief doc comment noting that a zero cursor (pre-allocation) reports full HEAP_LENGTH as “used.”sdk-libs/program-test/src/indexer/test_indexer.rs (2)
1632-1653: Avoid magic number for V2 token discriminator; prefer a shared constant.Hard-coding
[0,0,0,0,0,0,0,3]is brittle. Use a constant fromlight_compressed_token::constantsif available, or define a localconstwith a clear name.Example:
- let is_v2_token = data.discriminator == [0, 0, 0, 0, 0, 0, 0, 3]; // V2 discriminator + // Prefer importing from light_compressed_token::constants if exposed: + // use light_compressed_token::constants::TOKEN_COMPRESSED_ACCOUNT_V2_DISCRIMINATOR; + const TOKEN_COMPRESSED_ACCOUNT_V2_DISCRIMINATOR: [u8; 8] = [0, 0, 0, 0, 0, 0, 0, 3]; + let is_v2_token = data.discriminator == TOKEN_COMPRESSED_ACCOUNT_V2_DISCRIMINATOR;
1733-1799: Harden nullification path: avoid unwraps and use logging consistently.
- Replace
.hash().unwrap()with a safe check to prevent panics on malformed accounts.- Prefer
log::info!/warn!overprintln!for consistency with the rest of the file.Apply:
- let index = self - .compressed_accounts - .iter() - .position(|x| x.hash().unwrap() == hash); + let index = self + .compressed_accounts + .iter() + .position(|x| x.hash().map_or(false, |h| h == hash)); @@ - } else if let Some(index) = self - .token_compressed_accounts - .iter() - .position(|x| x.compressed_account.hash().unwrap() == hash) + } else if let Some(index) = self + .token_compressed_accounts + .iter() + .position(|x| x.compressed_account.hash().map_or(false, |h| h == hash)) { @@ - } else { - println!("Test indexer didn't find input compressed accounts to nullify"); + } else { + log::info!("Test indexer didn't find input compressed accounts to nullify"); }program-tests/registry-test/tests/tests.rs (1)
159-170: Boundary coverage: includeepoch + 1in continuity checkfor offset in -1..1 excludes +1. To catch off-by-one at the upper boundary, use an inclusive range.
- for offset in -1..1 { + for offset in -1..=1 {sdk-libs/program-test/Cargo.toml (1)
54-61: Trim chrono features to reduce build sizeIf you only need timestamps/clock, consider disabling default features.
-chrono = "0.4" +chrono = { version = "0.4", default-features = false, features = ["clock"] }If serde support is required for logs, add "serde" to features.
sdk-libs/program-test/src/lib.rs (1)
117-118: Optionally gate the new logging module behind a featureEnhanced logging pulls extra deps; consider a feature flag to keep default builds lean.
-pub mod logging; +#[cfg(feature = "enhanced-logging")] +pub mod logging;Follow-up: wire the Cargo feature and cfgs where logging is referenced.
sdk-libs/program-test/src/program_test/light_program_test.rs (2)
29-30: Make the tx counter private; add accessor/reset helpers and include in Debug.Avoid external mutation; document intended lifecycle.
- pub transaction_counter: usize, + transaction_counter: usize,Add helpers (outside this hunk):
impl LightProgramTest { pub fn transaction_counter(&self) -> usize { self.transaction_counter } pub fn reset_transaction_counter(&mut self) { self.transaction_counter = 0; } }Optionally include in Debug:
f.debug_struct("LightProgramTest") .field("context", &"ProgramTestContext") .field("indexer", &self.indexer) .field("test_accounts", &self.test_accounts) + .field("transaction_counter", &self.transaction_counter) .finish()
117-118: Post-setup reset is good for determinism. Provide an explicit reset API.Expose a
reset_transaction_counter()and use it here and in tests instead of mutating the field directly.- context.transaction_counter = 0; + context.reset_transaction_counter();sdk-libs/client/src/rpc/rpc_trait.rs (1)
209-210: API addition LGTM; add a brief doc comment clarifying semantics.Document that it returns the active Address V2 tree info (tree vs. queue) and whether values are static or fetched.
Example doc (outside this hunk):
/// Returns info for the Address V2 tree used for address operations. /// Implementations should clarify whether this is static or cluster-fetched. fn get_address_tree_v2(&self) -> TreeInfo;Ensure implementations don’t return identical tree and queue pubkeys unless intentional.
sdk-tests/sdk-pinocchio-test/tests/test.rs (1)
60-61: Param naming mismatch may confuse (queue vs. merkle tree).You pass
&output_queueto a parameter namedmerkle_tree_pubkey. Rename for clarity.-pub async fn create_pda( +pub async fn create_pda( payer: &Keypair, rpc: &mut LightProgramTest, - merkle_tree_pubkey: &Pubkey, + output_queue_pubkey: &Pubkey, account_data: [u8; 31], address_tree_pubkey: Pubkey, address: [u8; 32], ) -> Result<(), RpcError> { … - let output_merkle_tree_index = accounts.insert_or_get(*merkle_tree_pubkey); + let output_merkle_tree_index = accounts.insert_or_get(*output_queue_pubkey);And at the call site:
- create_pda( + create_pda( &payer, &mut rpc, - &output_queue, + &output_queue, account_data, address_tree_pubkey.tree, address, )sdk-libs/program-test/src/program_test/config.rs (3)
38-41: Two sources of truth for event logging — risk of driftYou now have both
log_light_protocol_eventsandenhanced_logging: EnhancedLoggingConfig. IfEnhancedLoggingConfigalso contains the same flag, these can diverge and create confusing behavior.Consider driving the flag solely from
EnhancedLoggingConfigand deprecating the bool onProgramTestConfig, or at minimum add a builder that sets both in sync:pub struct ProgramTestConfig { @@ - /// Log Light Protocol events (BatchPublicTransactionEvent, etc.) - pub log_light_protocol_events: bool, + /// Log Light Protocol events (BatchPublicTransactionEvent, etc.) + /// Deprecated: prefer `enhanced_logging.log_light_protocol_events` + #[deprecated(note = "Use enhanced_logging.log_light_protocol_events")] + pub log_light_protocol_events: bool, @@ /// Enhanced transaction logging configuration pub enhanced_logging: EnhancedLoggingConfig, }
90-100: Collapse the two toggles into a single builderTwo separate methods add API surface without extra value.
Provide a single boolean setter:
-/// Enable Light Protocol event logging -pub fn with_light_protocol_events(mut self) -> Self { - self.log_light_protocol_events = true; - self -} - -/// Disable Light Protocol event logging -pub fn without_light_protocol_events(mut self) -> Self { - self.log_light_protocol_events = false; - self -} +/// Toggle Light Protocol event logging +pub fn light_protocol_events(mut self, enabled: bool) -> Self { + self.log_light_protocol_events = enabled; + self +}
66-76: Legacy helper still public — consider deprecating or documenting status
default_with_batched_treesremains public while newer builders exist. If callers should migrate, mark it deprecated to guide users.-#[cfg(feature = "v2")] -pub fn default_with_batched_trees(with_prover: bool) -> Self { +#[cfg(feature = "v2")] +#[deprecated(note = "Use ProgramTestConfig::new_v2 or explicit builders")] +pub fn default_with_batched_trees(with_prover: bool) -> Self {sdk-tests/sdk-native-test/tests/test.rs (1)
70-73: Strengthen the assertion after updateYou assert the address only. Verifying the data actually changed will catch regressions.
After
update_pda, fetch and assert the new data equals[2u8; ARRAY_LEN].sdk-libs/program-test/src/logging/decoder.rs (3)
56-83: Avoid string conversions for program ID dispatchConverting to strings on every call is unnecessary and allocates.
Compare
Pubkeydirectly:- match program_id.to_string().as_str() { + if program_id == &system_program::ID { + return decode_system_instruction(data); + } + match program_id { + pid if pid.to_string() == "SySTEM1eSU2p4BGQfQpimFEWWSC1XDFeun3Nqzz3rT7" => { + decode_light_system_instruction(data, accounts, program_id) + } + // ...similar for other program IDs... + _ => Some(ParsedInstructionData::Unknown { program_name: get_program_name(program_id), data_preview: bs58::encode(&data[..data.len().min(16)]).into_string(), }), }(Or define
constPubkeys for known programs and compare directly.)
943-956: Duplicate program-name mapping across modules
get_program_namehere diverges from the one inlogging/types.rs, risking inconsistent labels.Prefer a single source (e.g., re-export from
types.rs), or keep the map in one module and reference it here.
220-227: Defensive parse error messagesError strings like “Instruction data too short for Anchor prefix” are good; consider including actual lengths to ease troubleshooting.
Append
data.len()to the message.sdk-libs/program-test/src/logging/config.rs (1)
28-42: Consider the default values forenabledandlog_eventsfields.The default configuration has
enabled: truebutlog_events: false, which means the logging system is always processing transactions even when not outputting. This could have a minor performance impact in production tests.Consider whether
enabledshould default tofalsefor minimal overhead in production tests, or document why processing is always enabled:impl Default for EnhancedLoggingConfig { fn default() -> Self { Self { - enabled: true, // Always enabled for processing + enabled: false, // Disabled by default for minimal overhead log_events: false, // Don't log by defaultAlternatively, if always-enabled processing is intentional for capturing logs to file, consider adding a comment explaining this design choice more clearly.
sdk-libs/program-test/src/logging/mod.rs (1)
43-69: Consider caching the log file path.The
get_log_file_path()function executescargo metadatacommand on every call, which could be inefficient if called frequently. Since the target directory doesn't change during a test run, consider caching this value.+use std::sync::OnceLock; + +static LOG_FILE_PATH: OnceLock<PathBuf> = OnceLock::new(); + /// Get the log file path in target directory fn get_log_file_path() -> PathBuf { + LOG_FILE_PATH.get_or_init(|| { // Always use cargo workspace target directory use std::process::Command; if let Ok(output) = Command::new("cargo") // ... existing code ... // Fallback to current directory's target let mut path = PathBuf::from("target"); path.push("light_program_test.log"); path + }).clone() }sdk-libs/program-test/src/program_test/rpc.rs (3)
145-158: Inconsistent transaction counter increment placement.The transaction counter is incremented here but the comment says "logging should be done elsewhere". This could lead to confusion about where logging actually happens.
Consider clarifying the comment or moving the counter increment to be closer to the actual logging:
- // Handle transaction directly without logging (logging should be done elsewhere) + // Increment counter before processing (logging handled by maybe_print_logs) self.transaction_counter += 1;
334-351: Consider extracting duplicate enhanced logging calls.The enhanced logging logic is repeated multiple times with similar patterns. Consider extracting a helper method to reduce duplication.
+impl LightProgramTest { + fn log_transaction_if_enhanced( + &self, + transaction: &Transaction, + result: &litesvm::types::TransactionResult, + signature: &Signature, + slot: u64, + print_to_console: bool, + ) { + if crate::logging::should_use_enhanced_logging(&self.config) { + let fn_to_call = if print_to_console { + crate::logging::log_transaction_enhanced_with_console + } else { + crate::logging::log_transaction_enhanced + }; + fn_to_call( + &self.config, + transaction, + result, + signature, + slot, + self.transaction_counter, + ); + } + } +}Then use it to simplify the code:
- if crate::logging::should_use_enhanced_logging(&self.config) { - crate::logging::log_transaction_enhanced( - &self.config, - &transaction_for_logging, - &transaction_result, - &signature, - slot, - self.transaction_counter, - ); - } + self.log_transaction_if_enhanced( + &transaction_for_logging, + &transaction_result, + &signature, + slot, + false, + );
371-393: Complex conditional logging logic could be simplified.The nested conditions for console logging with enhanced vs basic logging create complexity. Consider restructuring for clarity.
The current implementation works correctly but could be more readable with early returns or extracted methods.
sdk-libs/program-test/src/logging/formatter.rs (3)
42-68: Prefer &'static str for ANSI codes to avoid allocations.
Colorsstores heap-allocatedStrings; immutable ANSI codes can be&'static str(orCow<'static, str>if needed). Reduces allocations and clones.Example:
-#[derive(Debug, Clone)] -pub struct Colors { - pub bold: String, - pub reset: String, - pub green: String, - pub red: String, - pub yellow: String, - pub blue: String, - pub cyan: String, - pub gray: String, -} +#[derive(Debug, Clone, Copy)] +pub struct Colors { + pub bold: &'static str, + pub reset: &'static str, + pub green: &'static str, + pub red: &'static str, + pub yellow: &'static str, + pub blue: &'static str, + pub cyan: &'static str, + pub gray: &'static str, +} impl Colors { pub fn new(use_colors: bool) -> Self { if use_colors { Self { - bold: "\x1b[1m".to_string(), - reset: "\x1b[0m".to_string(), - green: "\x1b[32m".to_string(), - red: "\x1b[31m".to_string(), - yellow: "\x1b[33m".to_string(), - blue: "\x1b[34m".to_string(), - cyan: "\x1b[36m".to_string(), - gray: "\x1b[90m".to_string(), + bold: "\x1b[1m", + reset: "\x1b[0m", + green: "\x1b[32m", + red: "\x1b[31m", + yellow: "\x1b[33m", + blue: "\x1b[34m", + cyan: "\x1b[36m", + gray: "\x1b[90m", } } else { Self { - bold: String::new(), - reset: String::new(), - green: String::new(), - red: String::new(), - yellow: String::new(), - blue: String::new(), - cyan: String::new(), - gray: String::new(), + bold: "", + reset: "", + green: "", + red: "", + yellow: "", + blue: "", + cyan: "", + gray: "", } } } }Also applies to: 29-40
436-451: Micro: avoid recomputing chunks length inside loop.You call
instruction.data.chunks(32)twice per iteration to compare withlen()-1; same in theUnknownbranch. Cache into a local slice.- // Chunk the data into 32-byte groups for better readability - for (i, chunk) in instruction.data.chunks(32).enumerate() { + // Chunk the data into 32-byte groups for better readability + let chunks: Vec<&[u8]> = instruction.data.chunks(32).collect(); + for (i, chunk) in chunks.iter().enumerate() { write!(output, "{} ", indent)?; - for (j, byte) in chunk.iter().enumerate() { + for (j, byte) in chunk.iter().cloned().enumerate() { if j > 0 { write!(output, ", ")?; } write!(output, "{}", byte)?; } - if i < instruction.data.chunks(32).len() - 1 { + if i < chunks.len() - 1 { writeln!(output, ",")?; } else { writeln!(output, "]{}", self.colors.reset)?; } }Apply similarly in the
instruction_databranch.Also applies to: 990-1015
1144-1151: Tree glyphs: use branch/last-branch consistently.Currently
prefixis"├─"for depth 0 and"└─"for all nested levels, which renders every nested item as a last child. Consider calculating last/non-last to improve readability.sdk-libs/program-test/src/logging/types.rs (2)
249-252: Fee calc: consider sourcing lamports-per-signature from meta or config.Hard-coding 5000 lamports may drift from cluster settings or LiteSVM configs.
466-476: Deduplicate program-name mapping.
get_program_namehere diverges fromdecoder::get_program_name(different fallbacks/labels). Centralize to one function to prevent drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.github/workflows/light-system-programs-tests.yml(2 hunks).github/workflows/rust.yml(2 hunks).github/workflows/sdk-tests.yml(2 hunks)Cargo.toml(6 hunks)anchor-programs/system/src/lib.rs(1 hunks)program-libs/heap/src/lib.rs(1 hunks)program-tests/create-address-test-program/src/lib.rs(1 hunks)program-tests/registry-test/tests/tests.rs(1 hunks)program-tests/system-cpi-test/src/lib.rs(1 hunks)programs/account-compression/src/instructions/resize_registered_program_account.rs(1 hunks)programs/account-compression/src/lib.rs(1 hunks)programs/compressed-token/src/lib.rs(1 hunks)programs/registry/src/lib.rs(1 hunks)scripts/devenv.sh(1 hunks)scripts/format.sh(1 hunks)sdk-libs/client/src/indexer/types.rs(1 hunks)sdk-libs/client/src/rpc/client.rs(1 hunks)sdk-libs/client/src/rpc/rpc_trait.rs(2 hunks)sdk-libs/program-test/Cargo.toml(1 hunks)sdk-libs/program-test/src/indexer/test_indexer.rs(5 hunks)sdk-libs/program-test/src/lib.rs(1 hunks)sdk-libs/program-test/src/logging/config.rs(1 hunks)sdk-libs/program-test/src/logging/decoder.rs(1 hunks)sdk-libs/program-test/src/logging/formatter.rs(1 hunks)sdk-libs/program-test/src/logging/mod.rs(1 hunks)sdk-libs/program-test/src/logging/types.rs(1 hunks)sdk-libs/program-test/src/program_test/config.rs(4 hunks)sdk-libs/program-test/src/program_test/light_program_test.rs(3 hunks)sdk-libs/program-test/src/program_test/rpc.rs(7 hunks)sdk-libs/program-test/src/utils/setup_light_programs.rs(1 hunks)sdk-tests/client-test/Cargo.toml(1 hunks)sdk-tests/client-test/tests/light_program_test.rs(0 hunks)sdk-tests/sdk-anchor-test/package.json(1 hunks)sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/src/lib.rs(1 hunks)sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/tests/test.rs(1 hunks)sdk-tests/sdk-native-test/Cargo.toml(2 hunks)sdk-tests/sdk-native-test/tests/test.rs(7 hunks)sdk-tests/sdk-pinocchio-test/tests/test.rs(4 hunks)
💤 Files with no reviewable changes (1)
- sdk-tests/client-test/tests/light_program_test.rs
🧰 Additional context used
🧬 Code graph analysis (12)
sdk-libs/client/src/rpc/client.rs (3)
sdk-libs/client/src/rpc/rpc_trait.rs (1)
get_address_tree_v2(209-209)sdk-libs/program-test/src/program_test/rpc.rs (2)
get_address_tree_v2(297-305)None(444-444)js/stateless.js/src/state/types.ts (1)
TreeInfo(77-99)
sdk-libs/program-test/src/logging/decoder.rs (2)
js/stateless.js/src/state/types.ts (2)
InstructionDataInvoke(277-306)InstructionDataInvokeCpi(311-345)sdk-libs/program-test/src/logging/types.rs (1)
get_program_name(467-476)
sdk-libs/program-test/src/utils/setup_light_programs.rs (2)
sdk-libs/program-test/src/program_test/config.rs (1)
new(44-53)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(50-136)
sdk-libs/program-test/src/indexer/test_indexer.rs (5)
program-libs/compressed-account/src/indexer_event/parse.rs (3)
data(354-362)data(400-420)data(421-443)program-libs/compressed-account/src/tx_hash.rs (1)
create_tx_hash(9-26)program-libs/compressed-account/src/compressed_account.rs (6)
hash(76-82)hash(325-338)hash(363-375)from(30-39)from(51-57)from(61-66)sdk-libs/program-test/src/indexer/extensions.rs (1)
get_state_merkle_trees_mut(42-42)sdk-libs/program-test/src/program_test/extensions.rs (1)
get_state_merkle_trees_mut(76-80)
sdk-libs/program-test/src/logging/config.rs (1)
sdk-libs/program-test/src/program_test/config.rs (1)
default(104-133)
sdk-libs/program-test/src/logging/formatter.rs (2)
sdk-libs/program-test/src/program_test/config.rs (1)
new(44-53)sdk-libs/program-test/src/logging/types.rs (4)
text(42-48)text(186-193)symbol(34-40)symbol(182-184)
sdk-libs/client/src/rpc/rpc_trait.rs (3)
sdk-libs/client/src/rpc/client.rs (1)
get_address_tree_v2(754-762)sdk-libs/program-test/src/program_test/rpc.rs (1)
get_address_tree_v2(297-305)js/stateless.js/src/state/types.ts (1)
TreeInfo(77-99)
sdk-tests/sdk-native-test/tests/test.rs (3)
sdk-libs/program-test/src/program_test/config.rs (2)
new_v2(56-64)new(44-53)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(50-136)program-libs/hasher/src/hash_to_field_size.rs (1)
hashv_to_bn254_field_size_be(38-49)
sdk-libs/program-test/src/logging/types.rs (1)
sdk-libs/program-test/src/logging/decoder.rs (2)
get_program_name(944-956)decode_instruction(55-83)
sdk-libs/program-test/src/program_test/rpc.rs (1)
sdk-libs/program-test/src/logging/mod.rs (3)
should_use_enhanced_logging(199-201)log_transaction_enhanced(145-162)log_transaction_enhanced_with_console(165-196)
sdk-libs/program-test/src/logging/mod.rs (3)
sdk-libs/program-test/src/logging/formatter.rs (2)
new(43-67)new(77-82)sdk-libs/program-test/src/program_test/config.rs (1)
new(44-53)sdk-libs/program-test/src/logging/types.rs (1)
from_transaction_result(234-345)
sdk-libs/program-test/src/program_test/config.rs (1)
sdk-libs/program-test/src/logging/config.rs (1)
from_env(89-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v2
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (sdk-libs, light-macros light-sdk light-program-test light-client light-batched-m...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test"])
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- 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-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-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", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: Forester e2e test
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: lint
🔇 Additional comments (37)
sdk-tests/sdk-anchor-test/package.json (1)
3-3: Confirm SBF vs native target for sdk-native-test
Thesdk-native-testcrate is declared insdk-tests/sdk-native-test/Cargo.tomland no stalesdk-testreferences remain here. If this is a native program-test crate, update the script to"test": "cargo test -p sdk-native-test"otherwise keep the current
cargo test-sbf -p sdk-native-test.sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/src/lib.rs (1)
2-2: Crate-level deprecation allow is fine.Matches the PR intent to silence Anchor’s internal realloc deprecation.
programs/registry/src/lib.rs (1)
2-2: Crate-level deprecation allow is acceptable.Keeps builds warning-free until Anchor’s internals are updated.
anchor-programs/system/src/lib.rs (1)
1-1: OK to suppress deprecation warnings at crate root.Consistent with Anchor realloc deprecation context.
program-tests/system-cpi-test/src/lib.rs (1)
2-2: Crate-level deprecation allow looks good.Keeps program-test noise down; no runtime impact.
sdk-libs/program-test/src/utils/setup_light_programs.rs (1)
29-29: Make log bytes limit configurable via env var overrideReplace hard-coded
100_000with an environment-driven value:- let program_test = LiteSVM::new().with_log_bytes_limit(Some(100_000)); + let log_limit = std::env::var("LITESVM_LOG_BYTES_LIMIT") + .ok() + .and_then(|v| v.parse::<usize>().ok()); + let program_test = LiteSVM::new().with_log_bytes_limit(log_limit);No other
with_log_bytes_limitcalls were found; please confirm the log decoder tolerates truncated logs when this limit is applied.programs/account-compression/src/lib.rs (1)
3-3: Gateallow(deprecated)behind theanchor-realloc-compatfeature
NoAccountInfo::reallocorreallocusages were found, so the current#![allow(deprecated)]masks nothing. Replace with:- #![allow(deprecated)] + #![cfg_attr(feature = "anchor-realloc-compat", allow(deprecated))]Verify that no other deprecated APIs require suppression before removing or scoping this attribute.
sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/tests/test.rs (1)
21-21: LGTM: test rename improves clarityName aligns with crate; no behavior change.
sdk-tests/sdk-native-test/Cargo.toml (1)
11-11: Crate rename verified — no remainingsdk_testreferences.
Nouse sdk_test::imports orcrate = "sdk_test"entries found..github/workflows/rust.yml (1)
15-15: Same PR-path update mirrors push — good.No action needed.
sdk-tests/client-test/Cargo.toml (1)
10-10: Approve removal of cdylib in client-test
No references to a .so/.dylib artifact for this crate were found.programs/account-compression/src/instructions/resize_registered_program_account.rs (2)
70-70: API switch to AccountInfo::resize looks good.The move off deprecated realloc aligns with the PR goal.
57-68: Guard zero-lamport transfer and verify resize semanticsSkip the transfer if
lamports_diff == 0to avoid a no-op and wasted CU, and confirm thatAccountInfo::resizestill zero-inits new memory as before.- invoke( + if lamports_diff > 0 { + invoke( &transfer( ctx.accounts.authority.key, ctx.accounts.registered_program_pda.key, lamports_diff, ), &[ ctx.accounts.authority.to_account_info(), ctx.accounts.registered_program_pda.to_account_info(), ctx.accounts.system_program.to_account_info(), ], )?; + }sdk-libs/client/src/indexer/types.rs (1)
461-467: Helper mirrors pack_output_tree_index logic; consistent and useful.Method correctly maps StateV1->tree and StateV2->queue; returning InvalidPackTreeType for others is appropriate.
.github/workflows/sdk-tests.yml (1)
6-8: Trigger path updates make sense.Broadening to program-libs, programs, and sdk-tests is appropriate for this workflow.
Also applies to: 14-17
Cargo.toml (4)
226-233: Global crate patches: confirm compatibility and publish impactPatching solana-program-runtime, solana-bpf-loader-program, and solana-program-memory at the workspace level forces all members to those forks. Ensure:
- The patched commits are ABI/API-compatible with the Solana 2.2 stack declared above.
- Crates intended for publish won’t unintentionally ship against forked dependencies.
Would you like a quick script to list workspace crates that depend on these patched crates so we can gate publishing accordingly?
200-200: Git-pinned profiler: lockstep with patched Solana forkslight-program-profiler depends on patched Solana runtime/loader behavior. Keep its rev in lockstep with the patched agave commits above to avoid subtle runtime/logging mismatches.
Confirm the profiler rev 36a75e1 was built/tested against agave rev 580e29f for this workspace.
107-111: SPL versions and features look consistentUpgrading spl-token-2022 to 7.0.0 and adding no-entrypoint is appropriate for program-side linking. No action needed.
59-62: Workspace package metadata addition is fineSetting edition=2021 and a workspace version is good for consistency across members.
program-tests/registry-test/tests/tests.rs (1)
196-196: Struct literal updated withtransaction_counter: verify field visibilityInitializing transaction_counter: 0 assumes the field is public. If LightProgramTest recently added this field as non-pub, this test will fail to compile.
If it’s non-pub, switch to a constructor/builder or add a setter. I can draft one if helpful.
sdk-libs/client/src/rpc/client.rs (1)
754-762: Hardcoded AddressV2 constants: validate per-network valuesThese static pubkeys are returned irrespective of network. For parity with state tree discovery, consider returning network-specific constants or a lookup.
I can add a small helper mirroring get_latest_active_state_trees() to source AddressV2 trees per network. Do you want that change in this PR?
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
62-63: Initialization to 0 is fine. Confirm no pre-setup reads rely on a non-zero value.If not needed, you can rely on the post-setup reset only.
sdk-tests/sdk-pinocchio-test/tests/test.rs (3)
24-24: Test rename LGTM.
35-36: Migration to get_address_tree_v2 is correct.
48-49: Using.tree.to_bytes()for derivation is the right adaptation.sdk-libs/program-test/src/program_test/config.rs (1)
130-132: Environment-driven default may surprise CI runs
EnhancedLoggingConfig::from_env()changes defaults whenRUST_BACKTRACEis set. This can make logs noisy in CI.Confirm intended behavior in CI and, if needed, force a stable default in tests:
- Ensure CI unset
RUST_BACKTRACEor explicitly overrideenhanced_loggingin test setup.sdk-tests/sdk-native-test/tests/test.rs (2)
27-29: Feature gating mismatch for new_v2
ProgramTestConfig::new_v2is cfg(feature = "v2") in the library; this test only gates ontest-sbf. Builds without "v2" will fail.Either enable "v2" for this test target or add a fallback:
- let config = - ProgramTestConfig::new_v2(true, Some(vec![("sdk_native_test", sdk_native_test::ID)])); + #[cfg(feature = "v2")] + let config = ProgramTestConfig::new_v2(true, Some(vec![("sdk_native_test", sdk_native_test::ID)])); + #[cfg(not(feature = "v2"))] + let config = ProgramTestConfig::new(true, Some(vec![("sdk_native_test", sdk_native_test::ID)]));
114-118: Unwrap on proof can panic — prefer explicit error
rpc_result.proof.0.unwrap()will panic on missing proof, making failures opaque.Use
expectwith a message or propagate a structured error:- proof: rpc_result.proof.0.unwrap().into(), + proof: rpc_result.proof.0.expect("missing validity proof from get_validity_proof").into(),If
RpcErrorsupports custom messages here, return an error instead of panicking.Likely an incorrect or invalid review comment.
sdk-libs/program-test/src/logging/decoder.rs (1)
16-49: Assumption about account layout (+2 after system program) is brittleHard-coding “tree accounts start 2 positions after system program” can break if account ordering changes.
Document and validate this assumption (e.g., assert indices within bounds and optionally log when resolution fails). Consider passing resolved pubkeys from the caller when available.
sdk-libs/program-test/src/logging/config.rs (2)
1-26: Well-structured configuration system for enhanced logging.The
EnhancedLoggingConfigstruct provides a comprehensive set of configuration options for controlling logging behavior. The boolean flags and enum-based verbosity levels offer good granularity for different debugging scenarios.
88-96: Environment-based configuration is well implemented.The
from_env()method provides a sensible way to automatically enable debug logging whenRUST_BACKTRACEis set, which aligns well with typical debugging workflows.sdk-libs/program-test/src/logging/mod.rs (3)
102-121: Efficient ANSI stripping implementation.The
strip_ansi_codes()function provides a lightweight solution for removing ANSI escape sequences without requiring regex dependencies. The state machine approach is efficient.
124-142: Robust file writing with directory creation.The
write_to_log_file()function properly ensures parent directories exist and handles errors gracefully with thelet _ =pattern, which is appropriate for logging that shouldn't fail the main execution.
198-201: Clear gating logic for enhanced logging.The
should_use_enhanced_logging()function provides a clean way to check if enhanced logging should be used, respecting both the configuration and the globalno_logsflag.sdk-libs/program-test/src/program_test/rpc.rs (2)
309-321: Good separation of enhanced and basic logging.The
maybe_print_logs()method cleanly delegates to the enhanced logging system when enabled, avoiding duplicate output. The fallback to basic logging maintains backward compatibility.
297-305: Hardcoded pubkey is correct TheEzKE84aVTkCUhDHLELqyJaq1Y7UVVmqxXqZjVHwHY3rKstring matches theBATCHED_ADDRESS_MERKLE_TREE_TEST_KEYPAIRcomment and is used consistently intest_keypairs.rs,test_accounts.rs, and client code.sdk-libs/program-test/src/logging/types.rs (1)
409-421: Confirm instruction success flag semantics.
success: trueis set for all inner instructions; if the transaction fails mid-flight, this may mislead. If LiteSVM exposes per-instruction status, propagate it; otherwise considerOption<bool>.
| - program: sdk-libs | ||
| packages: light-macros light-sdk light-program-test light-client light-batched-merkle-tree | ||
| test_cmd: | | ||
| cargo test -p light-macros | ||
| cargo test -p light-sdk | ||
| cargo test -p light-program-test | ||
| cargo test -p light-client | ||
| cargo test -p client-test | ||
| cargo test -p light-sparse-merkle-tree | ||
| cargo test -p light-batched-merkle-tree --features test-only -- --skip test_simulate_transactions --skip test_e2e |
There was a problem hiding this comment.
Matrix entry won’t run tests; sub-tests missing and test_cmd unused in step.
The step only iterates matrix['sub-tests']; for the sdk-libs entry you provided packages/test_cmd, so the job will either fail (null JSON) or skip the intended tests.
Replace the sdk-libs matrix entry to use sub-tests like the others:
- - program: sdk-libs
- packages: light-macros light-sdk light-program-test light-client light-batched-merkle-tree
- test_cmd: |
- cargo test -p light-macros
- cargo test -p light-sdk
- cargo test -p light-program-test
- cargo test -p light-client
- cargo test -p client-test
- cargo test -p light-sparse-merkle-tree
- cargo test -p light-batched-merkle-tree --features test-only -- --skip test_simulate_transactions --skip test_e2e
+ - program: sdk-libs
+ sub-tests: '[
+ "cargo test -p light-macros",
+ "cargo test -p light-sdk",
+ "cargo test -p light-program-test",
+ "cargo test -p light-client",
+ "cargo test -p client-test",
+ "cargo test -p light-sparse-merkle-tree",
+ "cargo test -p light-batched-merkle-tree --features test-only -- --skip test_simulate_transactions --skip test_e2e"
+ ]'📝 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.
| - program: sdk-libs | |
| packages: light-macros light-sdk light-program-test light-client light-batched-merkle-tree | |
| test_cmd: | | |
| cargo test -p light-macros | |
| cargo test -p light-sdk | |
| cargo test -p light-program-test | |
| cargo test -p light-client | |
| cargo test -p client-test | |
| cargo test -p light-sparse-merkle-tree | |
| cargo test -p light-batched-merkle-tree --features test-only -- --skip test_simulate_transactions --skip test_e2e | |
| - program: sdk-libs | |
| sub-tests: '[ | |
| "cargo test -p light-macros", | |
| "cargo test -p light-sdk", | |
| "cargo test -p light-program-test", | |
| "cargo test -p light-client", | |
| "cargo test -p client-test", | |
| "cargo test -p light-sparse-merkle-tree", | |
| "cargo test -p light-batched-merkle-tree --features test-only -- --skip test_simulate_transactions --skip test_e2e" | |
| ]' |
🤖 Prompt for AI Agents
.github/workflows/sdk-tests.yml around lines 58 to 67: the sdk-libs matrix entry
uses `packages` and `test_cmd` which aren’t consumed by the runner (it reads
matrix['sub-tests']), so tests will be skipped or fail; replace that entry to
match the other matrix entries by removing `packages`/`test_cmd` and providing a
`sub-tests` array containing each individual test command (e.g., cargo test -p
light-macros, cargo test -p light-sdk, cargo test -p light-program-test, cargo
test -p light-client, cargo test -p client-test, cargo test -p
light-sparse-merkle-tree, cargo test -p light-batched-merkle-tree --features
test-only -- --skip test_simulate_transactions --skip test_e2e) so the job
iterates these via matrix.sub-tests.
| export NPM_CONFIG_PREFIX | ||
| export LIGHT_PROTOCOL_TOPLEVEL | ||
| export LIGHT_PROTOCOL_DEVENV | ||
| export SBF_OUT_DIR=target/deploy |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make SBF_OUT_DIR absolute and respect caller overrides
Using a relative path can break if the CWD changes; align with other vars that use LIGHT_PROTOCOL_TOPLEVEL and don’t override if already set.
-export SBF_OUT_DIR=target/deploy
+export SBF_OUT_DIR="${SBF_OUT_DIR:-${LIGHT_PROTOCOL_TOPLEVEL}/target/deploy}"📝 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.
| export SBF_OUT_DIR=target/deploy | |
| export SBF_OUT_DIR="${SBF_OUT_DIR:-${LIGHT_PROTOCOL_TOPLEVEL}/target/deploy}" |
🤖 Prompt for AI Agents
In scripts/devenv.sh around line 90, the SBF_OUT_DIR is set to a relative path
which can break if the working directory changes; change it to only set
SBF_OUT_DIR when not already defined and make it absolute using the existing
LIGHT_PROTOCOL_TOPLEVEL variable (e.g. export
SBF_OUT_DIR="${SBF_OUT_DIR:-$LIGHT_PROTOCOL_TOPLEVEL/target/deploy}"), ensuring
you do not overwrite an existing caller-provided value and that the path is
anchored to LIGHT_PROTOCOL_TOPLEVEL.
| fn get_address_tree_v2(&self) -> TreeInfo { | ||
| TreeInfo { | ||
| tree: pubkey!("EzKE84aVTkCUhDHLELqyJaq1Y7UVVmqxXqZjVHwHY3rK"), | ||
| queue: pubkey!("EzKE84aVTkCUhDHLELqyJaq1Y7UVVmqxXqZjVHwHY3rK"), | ||
| cpi_context: None, | ||
| next_tree_info: None, | ||
| tree_type: TreeType::AddressV2, | ||
| } | ||
| } |
There was a problem hiding this comment.
Trait vs inherent method: ensure Rpc impl provides get_address_tree_v2
The Rpc trait declares fn get_address_tree_v2(&self) -> TreeInfo;, but this addition is an inherent method on LightClient. If the Rpc impl doesn’t also define it, compilation will fail.
Add the method to the Rpc impl, delegating to the inherent one:
impl Rpc for LightClient {
+ fn get_address_tree_v2(&self) -> TreeInfo {
+ // Delegate to inherent method
+ LightClient::get_address_tree_v2(self)
+ }
// ...
}Also, queue equals tree here. Verify the AddressV2 queue pubkey; if distinct, update accordingly.
| input_data.push(super::types::InputAccountData { | ||
| lamports: input.lamports, | ||
| owner: Some(*program_id), // Use invoking program as owner | ||
| merkle_tree_index: None, // Note: merkle tree context not available in CompressedAccountInfo | ||
| merkle_tree_pubkey: None, | ||
| queue_index: None, | ||
| queue_pubkey: None, | ||
| address: account_info.address, // Use address from CompressedAccountInfo | ||
| data_hash: input.data_hash.to_vec(), | ||
| discriminator: input.discriminator.to_vec(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Owner field should use invoking program, not the Light System Program ID
For the AccountInfo path you set owner: Some(*program_id); unlike other paths, this loses the actual invoking program.
Align with other decoders:
- owner: Some(*program_id), // Use invoking program as owner
+ owner: Some(instruction_data.invoking_program_id.into()), // Use invoking program as owner
@@
- owner: Some(*program_id), // Use invoking program as owner
+ owner: Some(instruction_data.invoking_program_id.into()), // Use invoking program as ownerAlso applies to: 742-744
🤖 Prompt for AI Agents
In sdk-libs/program-test/src/logging/decoder.rs around lines 706 to 717 (and
also adjust the similar block at 742-744), the owner field is incorrectly set to
the invoking program ID (Some(*program_id)); instead, set owner to the owner
provided by the CompressedAccountInfo so the actual invoking program is
preserved — replace the hardcoded Some(*program_id) with the account_info owner
value (preserving its Option/clone semantics used elsewhere in the file) to
match the other decoder paths.
| // Handle long base58 strings (44+ characters) in table cells | ||
| if line.contains('|') && !line.trim_start().starts_with('|') { | ||
| // This is a table content line, not a border | ||
| let mut new_line = String::new(); | ||
| let mut modified = false; | ||
|
|
||
| // Split by table separators while preserving them | ||
| let parts: Vec<&str> = line.split('|').collect(); | ||
| for (i, part) in parts.iter().enumerate() { | ||
| if i > 0 { | ||
| new_line.push('|'); | ||
| } | ||
|
|
||
| // Check if this cell contains a long value | ||
| for word in part.split_whitespace() { | ||
| if word.len() > 44 && word.chars().all(|c| c.is_alphanumeric()) { | ||
| let indent = " ".repeat(leading_chars.len() + 2); // Extra space for table formatting | ||
| let formatted_word = self.format_long_value_with_indent(word, 44, &indent); | ||
| new_line.push_str(&part.replace(word, &formatted_word)); | ||
| modified = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if !modified { | ||
| new_line.push_str(part); | ||
| } | ||
| } | ||
|
|
||
| if modified { | ||
| return Some(new_line); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix: table cell wrapping drops subsequent cells and misses UTF-8 table separators.
- Only the first cell is preserved after wrapping due to the shared
modifiedflag. - Detection uses
|buttabledrenders│by default, so wrapping often never triggers.
Apply:
- // Handle long base58 strings (44+ characters) in table cells
- if line.contains('|') && !line.trim_start().starts_with('|') {
+ // Handle long base58 strings (44+ characters) in table cells
+ let is_table_line =
+ (line.contains('│') || line.contains('|')) &&
+ !line.trim_start().starts_with('│') &&
+ !line.trim_start().starts_with('|');
+ if is_table_line {
// This is a table content line, not a border
- let mut new_line = String::new();
- let mut modified = false;
+ let mut new_line = String::new();
+ let mut any_modified = false;
// Split by table separators while preserving them
- let parts: Vec<&str> = line.split('|').collect();
+ let sep = if line.contains('│') { '│' } else { '|' };
+ let parts: Vec<&str> = line.split(sep).collect();
for (i, part) in parts.iter().enumerate() {
- if i > 0 {
- new_line.push('|');
- }
-
- // Check if this cell contains a long value
- for word in part.split_whitespace() {
- if word.len() > 44 && word.chars().all(|c| c.is_alphanumeric()) {
- let indent = " ".repeat(leading_chars.len() + 2); // Extra space for table formatting
- let formatted_word = self.format_long_value_with_indent(word, 44, &indent);
- new_line.push_str(&part.replace(word, &formatted_word));
- modified = true;
- break;
- }
- }
-
- if !modified {
- new_line.push_str(part);
- }
+ if i > 0 { new_line.push(sep); }
+ // Check if this cell contains a long value
+ let mut cell = (*part).to_string();
+ let mut cell_modified = false;
+ for word in part.split_whitespace() {
+ if word.len() > 44 && word.chars().all(|c| c.is_alphanumeric()) {
+ let indent = " ".repeat(leading_chars.len() + 2); // Extra space for table formatting
+ let formatted_word = self.format_long_value_with_indent(word, 44, &indent);
+ cell = cell.replace(word, &formatted_word);
+ cell_modified = true;
+ break;
+ }
+ }
+ if cell_modified { any_modified = true; }
+ new_line.push_str(&cell);
}
- if modified {
+ if any_modified {
return Some(new_line);
}
}📝 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.
| // Handle long base58 strings (44+ characters) in table cells | |
| if line.contains('|') && !line.trim_start().starts_with('|') { | |
| // This is a table content line, not a border | |
| let mut new_line = String::new(); | |
| let mut modified = false; | |
| // Split by table separators while preserving them | |
| let parts: Vec<&str> = line.split('|').collect(); | |
| for (i, part) in parts.iter().enumerate() { | |
| if i > 0 { | |
| new_line.push('|'); | |
| } | |
| // Check if this cell contains a long value | |
| for word in part.split_whitespace() { | |
| if word.len() > 44 && word.chars().all(|c| c.is_alphanumeric()) { | |
| let indent = " ".repeat(leading_chars.len() + 2); // Extra space for table formatting | |
| let formatted_word = self.format_long_value_with_indent(word, 44, &indent); | |
| new_line.push_str(&part.replace(word, &formatted_word)); | |
| modified = true; | |
| break; | |
| } | |
| } | |
| if !modified { | |
| new_line.push_str(part); | |
| } | |
| } | |
| if modified { | |
| return Some(new_line); | |
| } | |
| } | |
| // Handle long base58 strings (44+ characters) in table cells | |
| let is_table_line = | |
| (line.contains('│') || line.contains('|')) && | |
| !line.trim_start().starts_with('│') && | |
| !line.trim_start().starts_with('|'); | |
| if is_table_line { | |
| // This is a table content line, not a border | |
| let mut new_line = String::new(); | |
| let mut any_modified = false; | |
| // Split by table separators while preserving them | |
| let sep = if line.contains('│') { '│' } else { '|' }; | |
| let parts: Vec<&str> = line.split(sep).collect(); | |
| for (i, part) in parts.iter().enumerate() { | |
| if i > 0 { | |
| new_line.push(sep); | |
| } | |
| // Check if this cell contains a long value | |
| let mut cell = (*part).to_string(); | |
| let mut cell_modified = false; | |
| for word in part.split_whitespace() { | |
| if word.len() > 44 && word.chars().all(|c| c.is_alphanumeric()) { | |
| let indent = " ".repeat(leading_chars.len() + 2); // Extra space for table formatting | |
| let formatted_word = self.format_long_value_with_indent(word, 44, &indent); | |
| cell = cell.replace(word, &formatted_word); | |
| cell_modified = true; | |
| break; | |
| } | |
| } | |
| if cell_modified { | |
| any_modified = true; | |
| } | |
| new_line.push_str(&cell); | |
| } | |
| if any_modified { | |
| return Some(new_line); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
programs/package.json (1)
10-10: RUSTFLAGS not exported; warnings may not be treated as errors in child processes.Use one of these:
- "test": "RUSTFLAGS=\"-D warnings\" && pnpm test-account-compression && pnpm test-compressed-token && pnpm e2e-test && pnpm test-registry && pnpm sdk-test-program && pnpm test-system && pnpm test-system-cpi", + "test": "export RUSTFLAGS='-D warnings' && pnpm test-account-compression && pnpm test-compressed-token && pnpm e2e-test && pnpm test-registry && pnpm sdk-test-program && pnpm test-system && pnpm test-system-cpi",or
- "test": "RUSTFLAGS=\"-D warnings\" && pnpm test-account-compression && pnpm test-compressed-token && pnpm e2e-test && pnpm test-registry && pnpm sdk-test-program && pnpm test-system && pnpm test-system-cpi", + "test": "RUSTFLAGS='-D warnings' pnpm test-account-compression && RUSTFLAGS='-D warnings' pnpm test-compressed-token && RUSTFLAGS='-D warnings' pnpm e2e-test && RUSTFLAGS='-D warnings' pnpm test-registry && RUSTFLAGS='-D warnings' pnpm sdk-test-program && RUSTFLAGS='-D warnings' pnpm test-system && RUSTFLAGS='-D warnings' pnpm test-system-cpi",.github/workflows/sdk-tests.yml (2)
33-33: Job won’t run on push events due to PR-only draft check.On push, github.event.pull_request is undefined, so this evaluates false. Make it conditional per event:
- if: github.event.pull_request.draft == false + if: github.event_name != 'pull_request' || github.event.pull_request.draft == false
95-101: test_cmd step doesn’t enforce -D warnings.Export once so all commands inherit:
- name: Run tests for ${{ matrix.program }} if: matrix.test_cmd != null run: | source ./scripts/devenv.sh npx nx build @lightprotocol/zk-compression-cli + export RUSTFLAGS="-D warnings" ${{ matrix.test_cmd }}sdk-libs/client/src/rpc/rpc_trait.rs (1)
82-89: Align should_retry with its comment: also avoid retrying on-chain TransactionError (and AssertRpcError).Comment says “Do not retry transaction errors,” but
RpcError::TransactionError(_)currently falls through totrue. Make these non-retryable to prevent hot-looping on deterministic on-chain failures.fn should_retry(&self, error: &RpcError) -> bool { match error { // Do not retry transaction errors. - RpcError::ClientError(error) => error.kind.get_transaction_error().is_none(), + RpcError::ClientError(error) => error.kind.get_transaction_error().is_none(), + RpcError::TransactionError(_) => false, + RpcError::AssertRpcError(_) => false, // Do not retry signing errors. RpcError::SigningError(_) => false, _ => true, } }
♻️ Duplicate comments (2)
.github/workflows/sdk-tests.yml (1)
58-67: sdk-libs now executed via test_cmd step—works; consider aligning on sub-tests for consistency/parallelization.If you keep test_cmd, that’s fine; using sub-tests mirrors other entries and simplifies the runner.
sdk-libs/client/src/rpc/rpc_trait.rs (1)
86-88: Non-retryable signing errors — resolves prior review.Explicitly returning
falseforRpcError::SigningError(_)matches the intent and avoids futile retries on local signing failures.
🧹 Nitpick comments (5)
programs/package.json (1)
11-13: Normalize test runner command naming.For consistency, prefer a single form (either cargo test-sbf or cargo-test-sbf) across scripts.
.github/workflows/sdk-tests.yml (2)
82-89: Robustly iterate sub-tests; avoid comma-splitting.Split by newlines to tolerate commas and trim safely:
- IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix.sub-tests), ', ') }}" + IFS=$'\n' read -r -d '' -a sub_tests < <(printf "%s\0" ${{ join(fromJSON(matrix.sub-tests), '\n') }})or with jq (runner has jq):
- IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix.sub-tests), ', ') }}" + readarray -t sub_tests < <(jq -r '.[]' <<< '${{ toJson(fromJSON(matrix.sub-tests)) }}')
24-24: Workflow name no longer matches scope.Rename to sdk-tests or programs-tests for clarity.
sdk-libs/client/src/rpc/errors.rs (1)
36-38: New SigningError variant is a good addition; consider stronger typing.Using a dedicated variant removes ambiguity and enables targeted retry logic. If feasible, prefer carrying a structured cause over
String(e.g., a concrete signer/transaction error type or a small enum), to avoid brittle string matching downstream.sdk-libs/client/src/rpc/rpc_trait.rs (1)
174-178: Map try_sign failures to SigningError — good; guard against missing payer keypair.Common footgun:
payernot included insigners. Fail fast with a clear message.let mut transaction = Transaction::new_with_payer(instructions, Some(payer)); - transaction + if !signers.iter().any(|k| k.pubkey() == *payer) { + return Err(RpcError::SigningError("payer keypair missing from signers".into())); + } + transaction .try_sign(signers, blockhash) .map_err(|e| RpcError::SigningError(e.to_string()))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/sdk-tests.yml(3 hunks)programs/package.json(1 hunks)sdk-libs/client/src/rpc/errors.rs(2 hunks)sdk-libs/client/src/rpc/rpc_trait.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
sdk-libs/client/src/rpc/errors.rs (1)
js/stateless.js/src/errors.ts (1)
RpcError(79-79)
sdk-libs/client/src/rpc/rpc_trait.rs (2)
sdk-libs/client/src/rpc/client.rs (1)
get_address_tree_v2(754-762)sdk-libs/program-test/src/program_test/rpc.rs (1)
get_address_tree_v2(297-305)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: Forester e2e test
- GitHub Check: stateless-js-v1
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: cli-v1
- GitHub Check: lint
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-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", "cargo test -p light-syst...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test"])
- GitHub Check: system-programs (sdk-libs, light-macros light-sdk light-program-test light-client light-batched-m...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
🔇 Additional comments (4)
.github/workflows/sdk-tests.yml (2)
37-47: Verify Redis image tag exists; pin to a known-good version.If redis:8.0.1 is unavailable, use a stable tag:
- redis: - image: redis:8.0.1 + redis: + image: redis:7.2-alpine
55-55: sdk-native-test switch acknowledged.sdk-libs/client/src/rpc/errors.rs (1)
79-79: Clone arm for SigningError is correct.Clones payload without lossy transformation; consistent with other cloneable variants.
sdk-libs/client/src/rpc/rpc_trait.rs (1)
211-211: Verified: all Rpc implementors include get_address_tree_v2.
| "e2e-test": "cargo-test-sbf -p e2e-test", | ||
| "test-registry": "cargo-test-sbf -p registry-test", | ||
| "sdk-test-program": "cargo test-sbf -p sdk-test", | ||
| "sdk-test-program": "cargo test-sbf -p sdk-native-test", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename to sdk-native-test looks correct. Verify no stale sdk-test references remain.
Run to confirm all callsites are updated:
🏁 Script executed:
#!/bin/bash
rg -nP '\bsdk-test\b|sdk_test' -g '!**/target/**' -SLength of output: 747
Remove remaining ‘sdk-test’ references
- sdk-tests/sdk-pinocchio-test/tests/test.rs: rename
test_pinocchio_sdk_test→test_pinocchio_sdk_native_test - sdk-tests/sdk-anchor-test/programs/sdk-anchor-test/tests/test.rs: rename
test_anchor_sdk_test→test_anchor_sdk_native_test - sdk-tests/sdk-anchor-test/Anchor.toml: update
sdk_test→sdk_native_test - program-tests/sdk-anchor-test/package.json: change
cargo test-sbf -p sdk-test→cargo test-sbf -p sdk-native-test - programs/package.json: rename script
sdk-test-program→sdk-native-test-programand fixpnpm sdk-test-programcall in thetestscript
🤖 Prompt for AI Agents
In programs/package.json around line 15, the npm script key "sdk-test-program"
should be renamed to "sdk-native-test-program" and any invocation of it in the
"test" script should be updated accordingly (change "pnpm sdk-test-program" to
"pnpm sdk-native-test-program"); update the script name and the call in the test
script so they match and run "cargo test-sbf -p sdk-native-test" as required.
2 participants
Notes:
Summary by CodeRabbit
New Features
Refactor
Chores
Tests