feat: format tx logs in light program test #1903
Conversation
WalkthroughThis update introduces an enhanced logging system to the light-program-test library, adding detailed, explorer-style transaction logs with configurable verbosity, color, and protocol-specific parsing. The changes include new configuration and formatting modules, updates to test infrastructure to support transaction counting, and integration of the logging system into transaction processing. Documentation and dependencies are also updated to support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as LightProgramTest
participant RPC as RPC Methods
participant Logger as Logging Module
participant File as Log File
participant Console as Console
Tester->>RPC: process_transaction(tx)
RPC->>Logger: log_transaction_enhanced(config, tx, result, signature, slot, counter)
alt Enhanced logging enabled
Logger->>File: Write formatted transaction log
alt Console output enabled
Logger->>Console: Print formatted transaction log
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
sdk-libs/program-test/src/program_test/rpc.rs (1)
179-190: Missing enhanced logging inprocess_transaction_with_context.This method increments the transaction counter but doesn't call the enhanced logging functions, unlike
process_transaction. This creates inconsistent logging behavior.Consider adding enhanced logging similar to
process_transaction:async fn process_transaction_with_context( &mut self, transaction: Transaction, ) -> Result<(Signature, Slot), RpcError> { let sig = *transaction.signatures.first().unwrap(); self.transaction_counter += 1; + let transaction_for_logging = transaction.clone(); + let signature = sig; let _res = self.context.send_transaction(transaction).map_err(|x| { if self.config.log_failed_tx { println!("{}", x.meta.pretty_logs()); } RpcError::TransactionError(x.err) })?; - self.maybe_print_logs(_res.pretty_logs()); let slot = self.context.get_sysvar::<Clock>().slot; + + // Add enhanced logging + if crate::logging::should_use_enhanced_logging(&self.config) { + crate::logging::log_transaction_enhanced( + &self.config, + &transaction_for_logging, + &Ok(_res.clone()), + &signature, + slot, + self.transaction_counter, + ); + } + + self.maybe_print_logs(_res.pretty_logs()); Ok((sig, slot)) }
🧹 Nitpick comments (8)
CLAUDE.md (1)
6-8: Add language specifier to fenced code block.The code block should specify the language for proper syntax highlighting.
-``` +```text target/light_program_test.log</blockquote></details> <details> <summary>sdk-libs/program-test/src/program_test/rpc.rs (1)</summary><blockquote> `449-481`: **Complex nested console logging logic could be simplified.** The console logging logic has deep nesting with multiple conditions. Consider extracting this into a helper method for better readability. Consider refactoring into a helper method: ```rust fn log_to_console(&self, transaction: &Transaction, result: &TransactionResult, signature: &Signature, slot: u64, event: &Option<Vec<BatchPublicTransactionEvent>>) { if self.config.no_logs { return; } #[cfg(debug_assertions)] { if std::env::var("RUST_BACKTRACE").is_err() { return; } let use_enhanced = crate::logging::should_use_enhanced_logging(&self.config) && self.config.enhanced_logging.log_events; if use_enhanced { crate::logging::log_transaction_enhanced_with_console( &self.config, transaction, result, signature, slot, self.transaction_counter, true, ); } else { println!("{}", result.pretty_logs()); } if self.config.log_light_protocol_events { if let Some(ref event_data) = event { println!("event:\n {:?}", event_data); } } } }sdk-libs/program-test/src/logging/mod.rs (2)
118-136: Consider adding error handling for file operations.The current implementation silently ignores file I/O errors using
let _ =. Consider at least logging errors to stderr for debugging purposes.fn write_to_log_file(content: &str) { // Ensure session is initialized initialize_log_file(); let log_path = get_log_file_path(); // Ensure parent directory exists if let Some(parent) = log_path.parent() { - let _ = std::fs::create_dir_all(parent); + if let Err(e) = std::fs::create_dir_all(parent) { + eprintln!("Warning: Failed to create log directory: {}", e); + } } // Strip ANSI color codes for file output let clean_content = strip_ansi_codes(content); // Append transaction log to existing file if let Ok(mut file) = OpenOptions::new().create(true).append(true).open(&log_path) { - let _ = writeln!(file, "{}", clean_content); + if let Err(e) = writeln!(file, "{}", clean_content) { + eprintln!("Warning: Failed to write to log file: {}", e); + } + } else { + eprintln!("Warning: Failed to open log file at {:?}", log_path); } }
71-93: Consider log rotation to prevent unbounded file growth.The log file is only truncated at session start but appends indefinitely during the session. For long-running test suites, this could lead to very large log files.
Consider implementing:
- A maximum file size limit with rotation (e.g.,
light_program_test.log.1,light_program_test.log.2)- A configuration option to control log retention
- Periodic cleanup of old log files
sdk-libs/program-test/src/logging/decoder.rs (2)
25-29: Use Solana SDK's system program ID constant instead of hardcoded string.Replace the hardcoded string comparison with the SDK's built-in constant for better maintainability.
- if account.pubkey.to_string() == "11111111111111111111111111111111" { + if account.pubkey == solana_sdk::system_program::ID {
109-109: Consider using try_into().ok() for more idiomatic error handling.While the unwrap() is safe due to the length check, using ok() would be more consistent with Rust idioms.
- let discriminator: [u8; 8] = data[0..8].try_into().unwrap(); + let discriminator: [u8; 8] = data[0..8].try_into().expect("already checked length");sdk-libs/program-test/src/logging/formatter.rs (1)
1162-1233: Consider extracting hardcoded account mappings to a configuration file or constants module.The function contains many hardcoded test account addresses that could be better organized in a separate configuration for maintainability.
Consider creating a separate module or configuration file for account name mappings:
// In a separate config module pub const KNOWN_ACCOUNTS: &[(&str, &str)] = &[ ("FNt7byTHev1k5x2cXZLBr8TdWiC3zoP5vcnZR4P682Uy", "test program"), ("smt1NamzXdq4AMqS2fS2F1i5KTYPZRhoHgWx38d8WsT", "v1 state merkle tree"), // ... other mappings ];sdk-libs/program-test/src/logging/types.rs (1)
253-255: Document or make configurable the hardcoded fee and compute limit values.The hardcoded fee calculation (5000 lamports) and compute limit (1,400,000) should either be documented as estimates or made configurable.
- // Calculate estimated fee (basic calculation: signatures * lamports_per_signature) - // Default Solana fee is 5000 lamports per signature - let estimated_fee = (transaction.signatures.len() as u64) * 5000; + // Calculate estimated fee (basic calculation: signatures * lamports_per_signature) + // Default Solana fee is 5000 lamports per signature + // Note: This is a simplified estimate and may not reflect actual fees + const LAMPORTS_PER_SIGNATURE: u64 = 5000; + let estimated_fee = (transaction.signatures.len() as u64) * LAMPORTS_PER_SIGNATURE; - compute_total: 1_400_000, // Default compute limit + compute_total: 1_400_000, // Default Solana compute unit limitAlso applies to: 343-343
📜 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 (13)
CLAUDE.md(1 hunks)Cargo.toml(1 hunks)program-tests/registry-test/tests/tests.rs(1 hunks)sdk-libs/program-test/Cargo.toml(1 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(2 hunks)sdk-libs/program-test/src/program_test/rpc.rs(7 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
6-6: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: 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: lint
- GitHub Check: cli-v2
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs-fast
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- 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"])
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: stateless-js-v1
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: Test program-libs-slow
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v1
🔇 Additional comments (13)
Cargo.toml (1)
200-200: LGTM!Clean addition of the
tableddependency to support enhanced transaction log formatting. The version and placement under the "Math and crypto" section are appropriate.sdk-libs/program-test/src/logging/config.rs (4)
1-26: Well-designed configuration structure.The
EnhancedLoggingConfigstruct provides a comprehensive set of options for controlling logging behavior. The field documentation is clear and the separation between enabling logging vs. event output is well thought out.
44-55: Logical verbosity levels.The
LogVerbosityenum provides a clear progression from brief to full detail, with good documentation explaining what each level includes.
88-96: Smart environment-based configuration.The
from_env()method usingRUST_BACKTRACEas a signal for debug-level logging is a intuitive pattern that aligns with developer expectations.
98-109: Clean builder pattern implementation.The
with_logging()andwithout_logging()methods follow proper builder pattern conventions and provide a fluent API for configuration.sdk-libs/program-test/src/lib.rs (1)
117-117: LGTM!Clean addition of the logging module to expose the enhanced logging functionality at the crate root. The placement among other module declarations is appropriate.
program-tests/registry-test/tests/tests.rs (1)
196-196: LGTM!Proper initialization of the new
transaction_counterfield to support enhanced logging. The initial value of 0 is appropriate for a counter.sdk-libs/program-test/Cargo.toml (1)
54-59: Necessary dependencies for enhanced logging.All six new dependencies are appropriately needed for the enhanced logging functionality:
serde/serde_jsonfor configuration serializationsolana-transaction-statusfor transaction parsingbs58for base58 encoding/decodinglight-sdk-typesfor Light Protocol typestabledfor table formattingUsing workspace versions maintains consistency with the project's dependency management pattern.
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
29-29: LGTM! Transaction counter field properly added.The
transaction_counterfield is correctly initialized to zero and will track transactions for the enhanced logging system.Also applies to: 62-62
CLAUDE.md (1)
1-29: Well-structured documentation for the enhanced logging feature.The documentation clearly explains the logging system's features, configuration options, and file location. This will be helpful for users.
sdk-libs/program-test/src/program_test/config.rs (1)
12-12: Configuration changes properly implemented.The new logging configuration fields and builder methods are well-structured and follow Rust conventions. The default initialization using
EnhancedLoggingConfig::from_env()allows for flexible environment-based configuration.Also applies to: 31-40, 90-100, 130-131
sdk-libs/program-test/src/program_test/rpc.rs (1)
144-167: Enhanced logging integration looks good.The transaction counter increments and enhanced logging calls are properly placed in the transaction processing flow. The fallback to basic logging when enhanced logging is disabled ensures backward compatibility.
Also applies to: 343-447
sdk-libs/program-test/src/logging/mod.rs (1)
42-68: Robust log file path resolution with proper fallback.The implementation correctly uses cargo metadata to find the workspace target directory and has a sensible fallback. This ensures consistent log file location across different working directories.
| fn strip_ansi_codes(text: &str) -> String { | ||
| // Simple regex-free approach to remove ANSI escape sequences | ||
| let mut result = String::with_capacity(text.len()); | ||
| let mut chars = text.chars(); | ||
|
|
||
| while let Some(ch) = chars.next() { | ||
| if ch == '\x1b' { | ||
| // Found escape character, skip until we find 'm' (end of color code) | ||
| for next_ch in chars.by_ref() { | ||
| if next_ch == 'm' { | ||
| break; | ||
| } | ||
| } | ||
| } else { | ||
| result.push(ch); | ||
| } | ||
| } | ||
|
|
||
| result | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ANSI stripping implementation could miss edge cases.
The current implementation handles basic ANSI color codes but might miss other escape sequences. Consider using a dedicated library like strip-ansi-escapes for more robust handling.
🌐 Web query:
What are the common ANSI escape sequence patterns beyond color codes that should be handled when stripping ANSI from text?
💡 Result:
When stripping ANSI escape sequences from text, it's important to handle a variety of sequences beyond just color codes. ANSI escape sequences are used for a wide range of terminal control functions, not just colors. Here are some common patterns you should consider:
-
CSI (Control Sequence Introducer) sequences:
These start withESC [, often represented as\x1b[or\033[, and are followed by zero or more parameters (digits separated by semicolons) and terminated by a single final character (a letter). Examples include:- Cursor movement:
ESC [ <n> A(cursor up),ESC [ <n> B(cursor down), etc. - Erasing parts of the screen or line:
ESC [ <n> J,ESC [ <n> K - Set graphics rendition (SGR) codes for color and style:
ESC [ <params> m
- Cursor movement:
-
OSC (Operating System Command) sequences:
Start withESC ], followed by a command number, a semicolon, an argument string, and terminated by either BEL (\x07) orESC \. Example:ESC ] 0;some title BEL(Set terminal title)
-
Other escape sequences:
- Single-character sequences: Usually
ESCfollowed by a single character likeESC M(reverse linefeed) - DEL or other control sequences
- Single-character sequences: Usually
Common regex pattern components for stripping ANSI sequences:
\033\[([0-9;]*[A-Za-z]) # CSI sequences
\033\][^\a]*\a # OSC sequences ending with BEL
\033\][^\033]*(\033\\) # OSC sequences ending with ESC \
\033[@-Z\\-_] # Other 2-character sequencesPractical example for stripping ANSI codes:
\x1b\[[0-9;]*[A-Za-z] # CSI (color, cursor control, etc.)
\x1b\][^\x07]*(\x07|\x1b\\) # OSC (terminal titles, etc.)
\x1b[@-_] # Other escape sequencesSummary of what to handle:
- Control Sequence Introducer (CSI):
ESC [sequences - Operating System Command (OSC):
ESC ]sequences - Single-character escape sequences:
ESCplus one character - Other miscellaneous escape sequences
This broad handling ensures that all ANSI escape sequences, not just colors, are properly stripped from text.
Enhance ANSI stripping to cover all control sequences or use a dedicated crate
The current strip_ansi_codes only skips SGR color codes (ESC … m) and will miss many other ANSI sequences (cursor movements, terminal titles, single-char escapes, etc.). You should either extend your implementation or pull in a battle-tested library.
Suggested approaches:
• Use a comprehensive regex covering:
– CSI sequences: \x1B\[[0-9;]*[A-Za-z]
– OSC sequences: \x1B\][^\x07]*(\x07|\x1B\\)
– Other escapes: \x1B[@-Z\\-_]
Example in sdk-libs/program-test/src/logging/mod.rs:
use regex::Regex;
lazy_static! {
static ref ANSI_RE: Regex = Regex::new(
r"(?x)
\x1B\[[0-9;]*[A-Za-z] # CSI (colors, cursor, etc.)
| \x1B\][^\x07]*(\x07|\x1B\\) # OSC (titles, etc.)
| \x1B[@-Z\\-_] # Other simple escapes
"
).unwrap();
}
fn strip_ansi_codes(text: &str) -> String {
ANSI_RE.replace_all(text, "").into_owned()
}• Or adopt an existing crate such as strip-ansi-escapes or the console crate’s strip_ansi_codes function for guaranteed coverage of edge cases.
🤖 Prompt for AI Agents
In sdk-libs/program-test/src/logging/mod.rs around lines 96 to 115, the current
strip_ansi_codes function only removes SGR color codes and misses other ANSI
escape sequences. To fix this, replace the manual implementation with a
comprehensive regex that matches all ANSI control sequences including CSI, OSC,
and other escapes, or alternatively use a well-maintained crate like
strip-ansi-escapes or console's strip_ansi_codes function to ensure full
coverage of ANSI sequences.
ananas-block
force-pushed
the
jorrit/feat-light-program-test-logs
branch
from
f623572 to
b979127
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sdk-libs/program-test/src/logging/mod.rs (1)
96-115: Enhance ANSI stripping to handle all escape sequencesThe current implementation only removes SGR color codes (ending with 'm') but misses other ANSI sequences like cursor movements, terminal titles, and other control sequences.
Consider using a comprehensive regex pattern or a dedicated crate:
-fn strip_ansi_codes(text: &str) -> String { - // Simple regex-free approach to remove ANSI escape sequences - let mut result = String::with_capacity(text.len()); - let mut chars = text.chars(); - - while let Some(ch) = chars.next() { - if ch == '\x1b' { - // Found escape character, skip until we find 'm' (end of color code) - for next_ch in chars.by_ref() { - if next_ch == 'm' { - break; - } - } - } else { - result.push(ch); - } - } - - result -} +fn strip_ansi_codes(text: &str) -> String { + use regex::Regex; + use once_cell::sync::Lazy; + + static ANSI_RE: Lazy<Regex> = Lazy::new(|| { + Regex::new( + r"(?x) + \x1B\[[0-9;]*[A-Za-z] # CSI (colors, cursor, etc.) + | \x1B\][^\x07]*(\x07|\x1B\\) # OSC (titles, etc.) + | \x1B[@-Z\\-_] # Other simple escapes + " + ).unwrap() + }); + + ANSI_RE.replace_all(text, "").into_owned() +}Or use the
consolecrate:use console::strip_ansi_codes;
🧹 Nitpick comments (3)
CLAUDE.md (1)
6-8: Add language specifier to fenced code block for proper syntax highlightingThe code block should specify the language for better rendering in markdown viewers.
-``` +```text target/light_program_test.log</blockquote></details> <details> <summary>sdk-libs/program-test/src/logging/formatter.rs (1)</summary><blockquote> `1162-1234`: **Consider extracting hardcoded account mappings to a configuration module** The `get_account_name` function contains many hardcoded account name mappings. For better maintainability, consider moving these to a dedicated constant module or configuration file. Example approach: ```rust // In a new file: src/logging/known_accounts.rs use once_cell::sync::Lazy; use std::collections::HashMap; use solana_sdk::pubkey::Pubkey; pub static KNOWN_ACCOUNTS: Lazy<HashMap<Pubkey, &'static str>> = Lazy::new(|| { let mut map = HashMap::new(); // Test accounts map.insert( "smt1NamzXdq4AMqS2fS2F1i5KTYPZRhoHgWx38d8WsT".parse().unwrap(), "v1 state merkle tree" ); // ... add other mappings map });Then simplify the function:
fn get_account_name(&self, pubkey: &Pubkey) -> String { KNOWN_ACCOUNTS.get(pubkey) .map(|&name| name.to_string()) .unwrap_or_else(|| { if pubkey.is_on_curve() { "user account".to_string() } else { "pda account".to_string() } }) }sdk-libs/program-test/src/logging/types.rs (1)
471-480: Deduplicate the get_program_name functionThis function duplicates similar logic from
decoder.rs(lines 944-956). Consider consolidating into a single shared utility function.Move to a shared module:
// In src/logging/utils.rs or similar pub fn get_program_name(program_id: &Pubkey) -> String { match program_id.to_string().as_str() { id if id == system_program::ID.to_string() => "System Program".to_string(), "ComputeBudget111111111111111111111111111111" => "Compute Budget".to_string(), "SySTEM1eSU2p4BGQfQpimFEWWSC1XDFeun3Nqzz3rT7" => "Light System Program".to_string(), "compr6CUsB5m2jS4Y3831ztGSTnDpnKJTKS95d64XVq" => "Account Compression".to_string(), "cTokenmWW8bLPjZEBAUgYy3zKxQZW6VKi7bqNFEVv3m" => "Compressed Token Program".to_string(), "FNt7byTHev1k5x2cXZLBr8TdWiC3zoP5vcnZR4P682Uy" => "Test Program".to_string(), _ => { let pubkey_str = program_id.to_string(); format!("Program {}", &pubkey_str[..8]) } } }Then import and use in both files.
📜 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 (13)
CLAUDE.md(1 hunks)Cargo.toml(1 hunks)program-tests/registry-test/tests/tests.rs(1 hunks)sdk-libs/program-test/Cargo.toml(1 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(2 hunks)sdk-libs/program-test/src/program_test/rpc.rs(7 hunks)
✅ Files skipped from review due to trivial changes (2)
- sdk-libs/program-test/src/lib.rs
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- sdk-libs/program-test/Cargo.toml
- sdk-libs/program-test/src/program_test/light_program_test.rs
- sdk-libs/program-test/src/program_test/rpc.rs
- program-tests/registry-test/tests/tests.rs
- sdk-libs/program-test/src/logging/config.rs
- sdk-libs/program-test/src/program_test/config.rs
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
6-6: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Test batched-merkle-tree-simulate
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- 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 (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-test"])
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: stateless-js-v1
- GitHub Check: lint
- GitHub Check: cli-v2
- GitHub Check: cli-v1
- GitHub Check: Analyze (javascript-typescript)
| /// Parse Invoke instruction data - display data hashes directly | ||
| fn parse_invoke_instruction(data: &[u8], accounts: &[AccountMeta]) -> InstructionParseResult { | ||
| // Skip the 4-byte vec length prefix that Anchor adds | ||
| if data.len() < 4 { | ||
| return Err("Instruction data too short for Anchor prefix".into()); | ||
| } | ||
| let instruction_data = InstructionDataInvoke::try_from_slice(&data[4..])?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document the inconsistent 4-byte Anchor prefix handling across instruction types
The parse_invoke_instruction and parse_invoke_cpi_instruction functions skip a 4-byte Anchor vector length prefix, while parse_invoke_cpi_readonly_instruction and parse_invoke_cpi_account_info_instruction don't. This inconsistency should be documented or unified.
If this is intentional due to different serialization approaches, consider adding comments to clarify:
fn parse_invoke_cpi_readonly_instruction(
data: &[u8],
accounts: &[AccountMeta],
) -> InstructionParseResult {
+ // Note: This instruction type doesn't have the 4-byte Anchor prefix
let instruction_data = InstructionDataInvokeCpiWithReadOnly::try_from_slice(data)?;Also applies to: 363-368, 505-509, 637-642
🏁 Script executed:
#!/bin/bash
# Check if the InstructionData types have different serialization attributes
ast-grep --pattern 'struct InstructionDataInvokeCpiWithReadOnly {
$$$
}'
ast_grep --pattern 'struct InstructionDataInvokeCpi {
$$$
}'Length of output: 240
🏁 Script executed:
#!/bin/bash
# Search for struct definitions and derive attributes for InstructionData types
rg "struct InstructionDataInvoke" -A 5
rg "struct InstructionDataInvokeCpi" -A 5
rg "struct InstructionDataInvokeCpiWithReadOnly" -A 5
rg "struct InstructionDataInvokeCpiAccountInfo" -A 5Length of output: 7363
🏁 Script executed:
#!/bin/bash
# Extract parse_invoke functions to verify prefix handling
rg -n -A 5 "fn parse_invoke_instruction" sdk-libs/program-test/src/logging/decoder.rs
rg -n -A 5 "fn parse_invoke_cpi_instruction" sdk-libs/program-test/src/logging/decoder.rs
rg -n -A 5 "fn parse_invoke_cpi_readonly_instruction" sdk-libs/program-test/src/logging/decoder.rs
rg -n -A 5 "fn parse_invoke_cpi_account_info_instruction" sdk-libs/program-test/src/logging/decoder.rsLength of output: 1609
Document Anchor prefix exception in CPI readonly & account_info parsers
To avoid confusion around why only some parsers skip the 4-byte Anchor vec prefix, add a clarifying comment to the CPI-readonly and CPI-account_info parsing functions.
Locations to update in sdk-libs/program-test/src/logging/decoder.rs:
- parse_invoke_cpi_readonly_instruction (lines 505–509)
- parse_invoke_cpi_account_info_instruction (lines 637–642)
Suggested diff:
fn parse_invoke_cpi_readonly_instruction(
data: &[u8],
accounts: &[AccountMeta],
) -> InstructionParseResult {
+ // This instruction data is serialized directly (no 4-byte Anchor vec length prefix)
let instruction_data = InstructionDataInvokeCpiWithReadOnly::try_from_slice(data)?;
…
}
fn parse_invoke_cpi_account_info_instruction(
data: &[u8],
accounts: &[AccountMeta],
program_id: &Pubkey,
) -> InstructionParseResult {
+ // This instruction data is serialized directly (no 4-byte Anchor vec length prefix)
let instruction_data = InstructionDataInvokeCpiWithAccountInfo::try_from_slice(data)?;
…
}📝 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.
| /// Parse Invoke instruction data - display data hashes directly | |
| fn parse_invoke_instruction(data: &[u8], accounts: &[AccountMeta]) -> InstructionParseResult { | |
| // Skip the 4-byte vec length prefix that Anchor adds | |
| if data.len() < 4 { | |
| return Err("Instruction data too short for Anchor prefix".into()); | |
| } | |
| let instruction_data = InstructionDataInvoke::try_from_slice(&data[4..])?; | |
| /// In sdk-libs/program-test/src/logging/decoder.rs | |
| fn parse_invoke_cpi_readonly_instruction( | |
| data: &[u8], | |
| accounts: &[AccountMeta], | |
| ) -> InstructionParseResult { | |
| // This instruction data is serialized directly (no 4-byte Anchor vec length prefix) | |
| let instruction_data = InstructionDataInvokeCpiWithReadOnly::try_from_slice(data)?; | |
| // … rest of function unchanged | |
| } | |
| fn parse_invoke_cpi_account_info_instruction( | |
| data: &[u8], | |
| accounts: &[AccountMeta], | |
| program_id: &Pubkey, | |
| ) -> InstructionParseResult { | |
| // This instruction data is serialized directly (no 4-byte Anchor vec length prefix) | |
| let instruction_data = InstructionDataInvokeCpiWithAccountInfo::try_from_slice(data)?; | |
| // … rest of function unchanged | |
| } |
🤖 Prompt for AI Agents
In sdk-libs/program-test/src/logging/decoder.rs around lines 505 to 509 and 637
to 642, add clarifying comments in the functions
parse_invoke_cpi_readonly_instruction and
parse_invoke_cpi_account_info_instruction to document the reason for skipping
the 4-byte Anchor vec prefix. This comment should explain that the prefix is
specific to Anchor and why only some parsers handle it by skipping those bytes,
to avoid confusion for future readers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sdk-libs/program-test/src/logging/mod.rs (1)
103-122: The ANSI stripping implementation is incomplete and matches the previous review concern.As identified in the previous review, the current
strip_ansi_codesfunction only handles SGR color codes ending with 'm' and will miss many other ANSI escape sequences like cursor movements, terminal titles, and other control sequences.The previous review suggestion to use a comprehensive regex or a dedicated crate like
strip-ansi-escapesremains valid and should be implemented to ensure proper ANSI sequence removal from log files.
🧹 Nitpick comments (2)
sdk-libs/program-test/src/logging/mod.rs (2)
44-70: Consider error handling improvements for cargo metadata command execution.The
get_log_file_path()function executescargo metadatacommand but silently falls back to a default path when the command fails. While this provides resilience, it might mask legitimate issues.Consider adding debug logging or warnings when the cargo metadata command fails to help with troubleshooting in development environments.
fn get_log_file_path() -> PathBuf { // Always use cargo workspace target directory use std::process::Command; - if let Ok(output) = Command::new("cargo") + let output = Command::new("cargo") .arg("metadata") .arg("--format-version=1") .arg("--no-deps") - .output() - { + .output(); + + if let Ok(output) = output { if output.status.success() { if let Ok(metadata) = String::from_utf8(output.stdout) { if let Ok(json) = serde_json::from_str::<serde_json::Value>(&metadata) { if let Some(target_directory) = json["target_directory"].as_str() { let mut path = PathBuf::from(target_directory); path.push("light_program_test.log"); return path; } } } } + } else { + #[cfg(debug_assertions)] + eprintln!("Warning: Failed to get cargo metadata, using fallback log path"); } // Fallback to current directory's target let mut path = PathBuf::from("target"); path.push("light_program_test.log"); path }
28-28: Consider using a more specific chrono import.The bare
use chrono;import brings in the entire chrono crate namespace. Consider importing only the specific types needed.-use chrono; +use chrono::{DateTime, Utc};And update the usage:
- let datetime = chrono::DateTime::from_timestamp(timestamp as i64, 0) - .unwrap_or_else(|| chrono::Utc::now()); + let datetime = DateTime::from_timestamp(timestamp as i64, 0) + .unwrap_or_else(|| Utc::now());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sdk-libs/program-test/Cargo.toml(1 hunks)sdk-libs/program-test/src/logging/mod.rs(1 hunks)sdk-libs/program-test/src/program_test/light_program_test.rs(3 hunks)sdk-libs/program-test/src/program_test/rpc.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk-libs/program-test/src/program_test/light_program_test.rs
- sdk-libs/program-test/Cargo.toml
- sdk-libs/program-test/src/program_test/rpc.rs
⏰ 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: Test program-libs-fast
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs-slow
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- 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: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: cli-v2
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- 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 (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: stateless-js-v1
- GitHub Check: lint
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-test"])
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
sdk-libs/program-test/src/logging/mod.rs (3)
175-177: Early return logic is correct.The early return when enhanced logging is disabled is appropriate and prevents unnecessary computation when logging is not needed.
41-41: Good use ofstd::sync::Oncefor session initialization.The use of
Onceto ensure the log file is initialized only once per session is appropriate and thread-safe.
200-202: No changes needed:no_logscorrectly overrides enhanced loggingAll usages confirm that
no_logsis treated as a global “disable all logging” flag and always takes precedence overenhanced_logging.enabled. Theshould_use_enhanced_loggingfunction’sconfig.enhanced_logging.enabled && !config.no_logslogic is therefore correct.
| if let Some(parent) = log_path.parent() { | ||
| let _ = std::fs::create_dir_all(parent); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Silent error handling could mask permission issues.
The directory creation and file operations use let _ = to ignore errors, which could mask important issues like permission problems or disk space issues that would prevent logging from working.
Consider at minimum logging errors in debug mode:
// Ensure parent directory exists
if let Some(parent) = log_path.parent() {
- let _ = std::fs::create_dir_all(parent);
+ if let Err(e) = std::fs::create_dir_all(parent) {
+ #[cfg(debug_assertions)]
+ eprintln!("Warning: Failed to create log directory: {}", e);
+ }
}📝 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.
| if let Some(parent) = log_path.parent() { | |
| let _ = std::fs::create_dir_all(parent); | |
| } | |
| // Ensure parent directory exists | |
| if let Some(parent) = log_path.parent() { | |
| if let Err(e) = std::fs::create_dir_all(parent) { | |
| #[cfg(debug_assertions)] | |
| eprintln!("Warning: Failed to create log directory: {}", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In sdk-libs/program-test/src/logging/mod.rs around lines 132 to 134, the code
ignores errors from creating parent directories using `let _ =`, which can hide
permission or disk space issues. Modify the code to handle the result of
`create_dir_all` by logging any errors encountered, at least in debug mode, so
that failures in directory creation are visible and can be diagnosed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
sdk-libs/program-test/src/logging/mod.rs (2)
102-121: ANSI stripping implementation still incomplete despite previous feedbackThe current implementation only handles basic SGR color codes ending with 'm' and misses other ANSI escape sequences like cursor movement, OSC sequences, and other control codes. This is a duplicate of the previous review comment that hasn't been addressed.
The previous review comment about comprehensive ANSI stripping still applies. Consider implementing the suggested regex-based approach or using a dedicated crate:
+use regex::Regex; +use std::sync::OnceLock; + +static ANSI_REGEX: OnceLock<Regex> = OnceLock::new(); + /// Strip ANSI escape codes from string for plain text log files fn strip_ansi_codes(text: &str) -> String { - // Simple regex-free approach to remove ANSI escape sequences - let mut result = String::with_capacity(text.len()); - let mut chars = text.chars(); - - while let Some(ch) = chars.next() { - if ch == '\x1b' { - // Found escape character, skip until we find 'm' (end of color code) - for next_ch in chars.by_ref() { - if next_ch == 'm' { - break; - } - } - } else { - result.push(ch); - } - } - - result + let regex = ANSI_REGEX.get_or_init(|| { + Regex::new(r"(?x) + \x1B\[[0-9;]*[A-Za-z] # CSI (colors, cursor, etc.) + | \x1B\][^\x07]*(\x07|\x1B\\) # OSC (titles, etc.) + | \x1B[@-Z\\-_] # Other simple escapes + ").unwrap() + }); + regex.replace_all(text, "").into_owned() }
131-133: Error handling still ignores failures despite previous feedbackThe directory creation still uses
let _ =to ignore errors, which could mask permission issues or disk space problems. This is a duplicate of the previous review comment.// Ensure parent directory exists if let Some(parent) = log_path.parent() { - let _ = std::fs::create_dir_all(parent); + if let Err(e) = std::fs::create_dir_all(parent) { + #[cfg(debug_assertions)] + eprintln!("Warning: Failed to create log directory: {}", e); + } }
🧹 Nitpick comments (2)
sdk-libs/program-test/src/logging/mod.rs (2)
43-69: Consider caching the log path to avoid repeated cargo metadata callsThe
get_log_file_path()function callscargo metadataevery time it's invoked, which could be expensive if called frequently. Since the target directory is unlikely to change during execution, consider caching the result.+use std::sync::OnceLock; + +static LOG_PATH_CACHE: OnceLock<PathBuf> = OnceLock::new(); + /// Get the log file path in target directory fn get_log_file_path() -> PathBuf { + LOG_PATH_CACHE.get_or_init(|| { // Always use cargo workspace target directory use std::process::Command; if let Ok(output) = Command::new("cargo") .arg("metadata") .arg("--format-version=1") .arg("--no-deps") .output() { if output.status.success() { if let Ok(metadata) = String::from_utf8(output.stdout) { if let Ok(json) = serde_json::from_str::<serde_json::Value>(&metadata) { if let Some(target_directory) = json["target_directory"].as_str() { let mut path = PathBuf::from(target_directory); path.push("light_program_test.log"); return path; } } } } } // Fallback to current directory's target let mut path = PathBuf::from("target"); path.push("light_program_test.log"); path + }).clone() }
87-90: Handle potential timestamp conversion edge caseThe
chrono::DateTime::from_timestamp()could fail for extreme timestamp values. While the fallback tochrono::Utc::now()handles this, consider logging a debug warning to help with troubleshooting.// Format timestamp as readable date - let datetime = chrono::DateTime::from_timestamp(timestamp as i64, 0) - .unwrap_or_else(chrono::Utc::now); + let datetime = chrono::DateTime::from_timestamp(timestamp as i64, 0) + .unwrap_or_else(|| { + #[cfg(debug_assertions)] + eprintln!("Warning: Invalid timestamp {}, using current time", timestamp); + chrono::Utc::now() + });
📜 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 (1)
sdk-libs/program-test/src/logging/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: cli-v1
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs-fast
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs-slow
- GitHub Check: cli-v2
- GitHub Check: lint
- 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 (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- 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-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- 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 (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
sdk-libs/program-test/src/logging/mod.rs (4)
1-39: Well-structured module with clear documentation and appropriate visibilityThe module documentation effectively describes the purpose and behavior of the enhanced logging system. The public module exports and imports are well-organized, providing a clean API surface for consumers.
145-162: Good API design with delegation patternThe main
log_transaction_enhancedfunction properly delegates to the more flexible version with console control, providing a clean default behavior while allowing customization when needed.
174-176: Early return optimization is appropriateThe early return when enhanced logging is disabled prevents unnecessary processing, which is efficient and correct.
199-201: Logic forshould_use_enhanced_loggingis correct as implemented
Verified its usage insdk-libs/program-test/src/program_test/rpc.rs; it correctly gates enhanced logging whenconfig.enhanced_logging.enabledis true andconfig.no_logsis false. No changes needed.
| if let Ok(mut file) = OpenOptions::new().create(true).append(true).open(&log_path) { | ||
| let _ = writeln!(file, "{}", clean_content); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
File write errors should also be handled consistently
Similar to the directory creation issue, file write errors are silently ignored. Consider applying the same error handling pattern for consistency.
// Append transaction log to existing file
if let Ok(mut file) = OpenOptions::new().create(true).append(true).open(&log_path) {
- let _ = writeln!(file, "{}", clean_content);
+ if let Err(e) = writeln!(file, "{}", clean_content) {
+ #[cfg(debug_assertions)]
+ eprintln!("Warning: Failed to write to log file: {}", e);
+ }
}🤖 Prompt for AI Agents
In sdk-libs/program-test/src/logging/mod.rs around lines 139 to 141, the file
write operation using writeln! currently ignores any errors. Update the code to
handle errors from writeln! consistently with the directory creation error
handling pattern by checking the result and logging or returning the error
appropriately instead of silently ignoring it.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation