Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a new light-instruction-decoder library and derive proc-macro, a DecoderRegistry, many program decoders (System, SPL Token, Token2022, ComputeBudget, AccountCompression, CToken, Registry, LightSystem), enhanced logging/types/formatter moved into the decoder crate, and integrates decoders into program-test with ProgramTestConfig hooks and re-exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Transaction Executor
participant Logger as Program Test Logger
participant Registry as DecoderRegistry
participant Decoder as Program Decoder
participant Formatter as TransactionFormatter
Client->>Logger: log_transaction_enhanced_with_console(tx, result)
Logger->>Logger: from_transaction_result(...)
Logger->>Registry: registry.decode(program_id, data, accounts)
Registry->>Registry: lookup decoder by program_id
Registry->>Decoder: Decoder::decode(data, accounts)
Decoder->>Decoder: match discriminator -> DecodedInstruction
Decoder-->>Registry: (DecodedInstruction, &Decoder)
Registry-->>Logger: (DecodedInstruction, &Decoder)
Logger->>Formatter: write_decoded_instruction(...)
Formatter-->>Client: formatted output
sequenceDiagram
participant Macro as Compiler Macro Expansion
participant Parser as Darling Parser
participant CrateCtx as CrateContext
participant Builder as InstructionDecoderBuilder
participant Output as Generated TokenStream
Macro->>Parser: parse derive/attribute args & input
Parser->>CrateCtx: optionally traverse crate for Accounts structs
Parser->>Builder: provide parsed args & derive input
Builder->>Builder: compute discriminators, field parsers, account names
loop per variant
Builder->>Output: emit match arm (discriminator match + field decode)
end
Builder->>Output: emit InstructionDecoder impl module
Output-->>Macro: expanded code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
sdk-libs/instruction-decoder/src/formatter.rs (1)
408-424: Decoded-but-empty fields suppress parsed/raw output.
Ifdecoded_instructionexists but has no fields, the formatter skips parsed_data and raw bytes entirely, so users see no details. Consider falling back to parsed/raw when decoded fields are empty.💡 Suggested fallback logic
- if let Some(ref decoded) = instruction.decoded_instruction { - if !decoded.fields.is_empty() { - let indent = self.get_tree_indent(depth + 1); - for field in &decoded.fields { - self.write_decoded_field(field, output, &indent, 0)?; - } - } - } else if let Some(ref parsed) = instruction.parsed_data { + let mut wrote_decoded = false; + if let Some(ref decoded) = instruction.decoded_instruction { + if !decoded.fields.is_empty() { + let indent = self.get_tree_indent(depth + 1); + for field in &decoded.fields { + self.write_decoded_field(field, output, &indent, 0)?; + } + wrote_decoded = true; + } + } + + if !wrote_decoded { + if let Some(ref parsed) = instruction.parsed_data { self.write_parsed_instruction_data( output, parsed, &instruction.data, depth + 1, )?; - } else if !instruction.data.is_empty() { + } else if !instruction.data.is_empty() { // ... - } + } + }
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder-derive/Cargo.toml`:
- Around line 1-20: Update the bs58 dependency in the
light-instruction-decoder-derive crate to match the workspace/root constraint:
replace the current bs58 = "0.5" entry in the Cargo.toml for the
light-instruction-decoder-derive package with a caret-pinned patch-aware version
(e.g., bs58 = "^0.5.1") so it aligns with the root workspace version while
leaving sha2 and other dependencies unchanged.
In `@sdk-libs/instruction-decoder-derive/src/lib.rs`:
- Around line 431-435: The generated 1-byte discriminator truncates u32 values
when casting to u8; change the discriminator handling in the block guarded by
discriminator_size == 1 to first compute let disc_u32 =
variant_info.discriminator.unwrap_or(variant_index as u32) and validate that
disc_u32 <= 255, and if not return/emit a clear compile-time error (e.g. via
syn::Error::new_spanned(...) or an Err) instead of silently casting; only after
the check convert to u8 for use in match_arms.push so dispatch is correct and
large discriminator values produce a helpful error.
- Around line 696-705: The code subtracts `#offset` from remaining.len() when
constructing the unknown-type DecodedField (see use of `#field_name`, remaining
and `#offset` in the DecodedField::new call), which can underflow if offset
exceeds remaining.len(); change the expression to use a safe length (e.g.,
remaining.len().saturating_sub(`#offset`) or check/clip the length before
formatting) and apply the same fix to the other occurrence around the
DecodedField creation at lines ~770-775 so the formatted "(Nbytes)" never
underflows or wraps.
- Around line 224-237: The custom to_snake_case function produces incorrect
snake_case for acronyms (e.g., CreateATA -> create_a_t_a) and breaks Anchor
discriminator calculation (SHA256("global:<instruction_name>")); replace the
manual to_snake_case implementation with heck's ToSnakeCase (use
name.to_snake_case()) so consecutive capitals stay grouped, and add heck = "0.4"
to Cargo.toml; update any call sites referencing to_snake_case to use the heck
conversion and ensure the discriminator code uses the resulting string.
In `@sdk-libs/instruction-decoder/src/config.rs`:
- Around line 119-133: The with_decoders method currently uses Arc::get_mut on
self.decoder_registry which fails when the Arc is shared and causes existing
decoders to be dropped; replace the get_mut logic with Arc::make_mut so the
DecoderRegistry is cloned when necessary and modifications are applied to the
unique instance. Specifically, in with_decoders operate on Arc::make_mut(arc)
(or create a new Arc if decoder_registry is None), then call
registry.register_all(decoders) on the mutable reference returned, and set
self.decoder_registry appropriately so existing decoders are preserved across
clones.
In `@sdk-libs/instruction-decoder/src/programs/registry.rs`:
- Around line 161-189: The instruction variants PauseCompressibleConfig,
UnpauseCompressibleConfig, and DeprecateCompressibleConfig currently declare
unused accounts new_update_authority and new_withdrawal_authority; update their
#[instruction_decoder(account_names = ...)] attributes to only include the
actual accounts used—["update_authority", "compressible_config"]—so the
attribute for PauseCompressibleConfig, UnpauseCompressibleConfig, and
DeprecateCompressibleConfig matches their state-only behavior.
In `@sdk-libs/instruction-decoder/src/registry.rs`:
- Around line 1286-1316: The AccountCompressionDecoder currently returns a
hardcoded "InsertIntoQueues" when data.len() >= 8 without checking instruction
discriminators; update decode in AccountCompressionDecoder to match the actual
8-byte (or appropriate) instruction discriminators for the Account Compression
program and return the correct instruction_name and parsed fields (map each
known discriminator to names like "InsertIntoQueues", "Compress", etc., and
parse any instruction-specific fields), using the existing decode(&self, data:
&[u8], _accounts: &[AccountMeta]) -> Option<DecodedInstruction> function; if you
cannot implement full parsing now, add an explicit doc comment on
AccountCompressionDecoder stating it is intentionally minimal/placeholder and
keep the current fallback behavior (return "Unknown" when no discriminator
match) so callers know this is a stub.
In `@sdk-libs/instruction-decoder/src/types.rs`:
- Around line 28-43: EnhancedTransactionLog::new currently hardcodes
compute_total to 1_400_000; change it to be configurable instead of a magic
constant by either adding an optional parameter to EnhancedTransactionLog::new
(e.g., compute_total: u64) or by reading a DEFAULT_COMPUTE_TOTAL constant /
config value (env/config) and using that value to initialize the compute_total
field; update the constructor signature and call sites that create
EnhancedTransactionLog to pass/derive the proper compute_total so the default is
not a hardcoded legacy value.
In `@sdk-libs/macros/src/light_pdas/accounts/builder.rs`:
- Around line 432-451: The impl in generate_account_names_impl() hardcodes
`#struct_name`<'_> and thus drops type/const generics; use split_for_impl() the
same way as
generate_noop_impls()/generate_pre_init_impl()/generate_finalize_impl() to
obtain (impl_generics, type_generics, where_clause) and apply them to both the
impl block and the HasAccountNames impl so ACCOUNT_NAMES preserves the struct's
generics; update references to `#struct_name`<'_> to use `#impl_generics`
`#struct_name` `#type_generics` `#where_clause` and keep the ACCOUNT_NAMES constant
and HasAccountNames const definition otherwise unchanged.
In `@sdk-libs/program-test/src/logging/mod.rs`:
- Around line 224-226: The fee estimate computed as let estimated_fee =
(transaction.signatures.len() as u64) * 5000; is a simplistic approximation;
update the code around the estimated_fee variable (referencing estimated_fee and
transaction.signatures) to add a concise comment stating this is an
approximate/logging-only estimate that does not account for priority fees,
compute unit pricing, or network fee variability, so future readers understand
it's not the canonical fee calculation.
| /// Convert PascalCase to snake_case | ||
| fn to_snake_case(name: &str) -> String { | ||
| let mut result = String::new(); | ||
| for (i, c) in name.chars().enumerate() { | ||
| if c.is_uppercase() { | ||
| if i > 0 { | ||
| result.push('_'); | ||
| } | ||
| result.push(c.to_ascii_lowercase()); | ||
| } else { | ||
| result.push(c); | ||
| } | ||
| } | ||
| result |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file and its context
cd /repo
head -n 250 sdk-libs/instruction-decoder-derive/src/lib.rs | tail -n 40Repository: Lightprotocol/light-protocol
Length of output: 1333
🏁 Script executed:
#!/bin/bash
# Search for how discriminators are calculated or used in this crate
rg -A 5 -B 5 "discriminator" sdk-libs/instruction-decoder-derive/Repository: Lightprotocol/light-protocol
Length of output: 24718
🏁 Script executed:
#!/bin/bash
# Check the Cargo.toml for dependencies
cat sdk-libs/instruction-decoder-derive/Cargo.tomlRepository: Lightprotocol/light-protocol
Length of output: 559
🌐 Web query:
Anchor instruction discriminator calculation algorithm case conversion
💡 Result:
Summary — how Anchor computes an instruction discriminator
- Algorithm: discriminator = first 8 bytes of SHA‑256("global:<instruction_name>"). [1][2]
- Case/format required: the instruction name is converted to snake_case (lowercase with underscores) before hashing. [2][3]
- Example: for instruction "InitializeAccount" you hash "global:initialize_account" and take the first 8 bytes of the SHA‑256 digest. [1][3]
Sources: Anchor docs and implementations. [1] [2] [3]
Use standard snake_case conversion to match Anchor's discriminator algorithm.
The custom to_snake_case implementation splits each uppercase character into a separate word (e.g., CreateATA → create_a_t_a), producing different SHA256 hashes than Anchor expects. This breaks instruction decoding for any instruction name containing acronyms or consecutive capitals.
Anchor's discriminator is computed as SHA256("global:<instruction_name>") where the name is converted using standard snake_case rules (acronyms stay grouped). Replace with heck::ToSnakeCase to ensure compatibility. The heck crate is permitted as an external dependency per the coding guidelines.
🔧 Suggested fix using `heck`
-fn to_snake_case(name: &str) -> String {
- let mut result = String::new();
- for (i, c) in name.chars().enumerate() {
- if c.is_uppercase() {
- if i > 0 {
- result.push('_');
- }
- result.push(c.to_ascii_lowercase());
- } else {
- result.push(c);
- }
- }
- result
-}
+fn to_snake_case(name: &str) -> String {
+ use heck::ToSnakeCase;
+ name.to_snake_case()
+}Add to Cargo.toml:
heck = "0.4"
📝 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.
| /// Convert PascalCase to snake_case | |
| fn to_snake_case(name: &str) -> String { | |
| let mut result = String::new(); | |
| for (i, c) in name.chars().enumerate() { | |
| if c.is_uppercase() { | |
| if i > 0 { | |
| result.push('_'); | |
| } | |
| result.push(c.to_ascii_lowercase()); | |
| } else { | |
| result.push(c); | |
| } | |
| } | |
| result | |
| /// Convert PascalCase to snake_case | |
| fn to_snake_case(name: &str) -> String { | |
| use heck::ToSnakeCase; | |
| name.to_snake_case() | |
| } |
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/src/lib.rs` around lines 224 - 237, The
custom to_snake_case function produces incorrect snake_case for acronyms (e.g.,
CreateATA -> create_a_t_a) and breaks Anchor discriminator calculation
(SHA256("global:<instruction_name>")); replace the manual to_snake_case
implementation with heck's ToSnakeCase (use name.to_snake_case()) so consecutive
capitals stay grouped, and add heck = "0.4" to Cargo.toml; update any call sites
referencing to_snake_case to use the heck conversion and ensure the
discriminator code uses the resulting string.
| // Generate match arm based on discriminator size | ||
| if discriminator_size == 1 { | ||
| // For 1-byte discriminator, use explicit discriminator if provided, otherwise variant index | ||
| let disc_byte = variant_info.discriminator.unwrap_or(variant_index as u32) as u8; | ||
| match_arms.push(quote! { |
There was a problem hiding this comment.
Guard against u8 discriminator truncation.
For 1-byte discriminators, casting from u32 can silently wrap when values exceed 255 (or when enum has >256 variants), leading to wrong dispatch. Validate the range and error out early.
🐛 Suggested fix
- let disc_byte = variant_info.discriminator.unwrap_or(variant_index as u32) as u8;
+ let disc_u32 = variant_info.discriminator.unwrap_or(variant_index as u32);
+ if disc_u32 > u8::MAX as u32 {
+ return Err(Error::new_spanned(
+ variant,
+ "discriminator exceeds u8 range; use discriminator_size = 4 or a valid u8 value",
+ ));
+ }
+ let disc_byte = disc_u32 as u8;🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/src/lib.rs` around lines 431 - 435, The
generated 1-byte discriminator truncates u32 values when casting to u8; change
the discriminator handling in the block guarded by discriminator_size == 1 to
first compute let disc_u32 = variant_info.discriminator.unwrap_or(variant_index
as u32) and validate that disc_u32 <= 255, and if not return/emit a clear
compile-time error (e.g. via syn::Error::new_spanned(...) or an Err) instead of
silently casting; only after the check convert to u8 for use in match_arms.push
so dispatch is correct and large discriminator values produce a helpful error.
| /// Register custom decoders | ||
| pub fn with_decoders(mut self, decoders: Vec<Box<dyn InstructionDecoder>>) -> Self { | ||
| // Since with_decoders takes self by value, there can only be one reference | ||
| // to the Arc at this point, so get_mut will succeed | ||
| if let Some(ref mut arc) = self.decoder_registry { | ||
| if let Some(registry) = Arc::get_mut(arc) { | ||
| registry.register_all(decoders); | ||
| return self; | ||
| } | ||
| } | ||
| // Create new registry if none exists | ||
| let mut registry = DecoderRegistry::new(); | ||
| registry.register_all(decoders); | ||
| self.decoder_registry = Some(Arc::new(registry)); | ||
| self |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the config.rs file and read the full context
find . -name "config.rs" -path "*/instruction-decoder/*" | head -20Repository: Lightprotocol/light-protocol
Length of output: 116
🏁 Script executed:
# Also, let's check the structure of sdk-libs
find sdk-libs -type f -name "*.rs" | head -20Repository: Lightprotocol/light-protocol
Length of output: 938
🏁 Script executed:
# Read the full config.rs file
wc -l ./sdk-libs/instruction-decoder/src/config.rsRepository: Lightprotocol/light-protocol
Length of output: 120
🏁 Script executed:
# Read the config.rs file with line numbers
cat -n ./sdk-libs/instruction-decoder/src/config.rsRepository: Lightprotocol/light-protocol
Length of output: 7393
🏁 Script executed:
# Find DecoderRegistry implementation
find sdk-libs -name "*.rs" -type f | xargs grep -l "struct DecoderRegistry" | head -5Repository: Lightprotocol/light-protocol
Length of output: 116
🏁 Script executed:
# Also search for the registry module
find sdk-libs/instruction-decoder -name "*.rs" -type fRepository: Lightprotocol/light-protocol
Length of output: 665
🏁 Script executed:
# Read the registry.rs file
cat -n ./sdk-libs/instruction-decoder/src/registry.rsRepository: Lightprotocol/light-protocol
Length of output: 50384
🏁 Script executed:
# Read just the DecoderRegistry struct and key methods
sed -n '1086,1160p' ./sdk-libs/instruction-decoder/src/registry.rsRepository: Lightprotocol/light-protocol
Length of output: 2644
🏁 Script executed:
# Also check if there are any tests or usage examples of with_decoders
find sdk-libs/instruction-decoder -name "*.rs" -type f | xargs grep -n "with_decoders" | head -20Repository: Lightprotocol/light-protocol
Length of output: 457
🏁 Script executed:
# Check if Arc::make_mut is used elsewhere in the codebase
find sdk-libs -name "*.rs" -type f | xargs grep -n "Arc::make_mut" | head -10Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check if there are any tests for with_decoders behavior
find . -name "*.rs" -type f | xargs grep -n "with_decoders" -A 5 -B 2 | head -40Repository: Lightprotocol/light-protocol
Length of output: 3677
Replace Arc::get_mut with Arc::make_mut for proper clone-on-write semantics.
The current logic silently drops previously registered custom decoders when with_decoders is called on a cloned config. When the Arc has multiple references (from prior clones), Arc::get_mut fails and the code creates a new empty registry, discarding all existing decoders. This is especially problematic since the Clone implementation explicitly shares the Arc across clones to preserve registry state.
Use Arc::make_mut instead, which ensures decoders are never lost: it modifies the registry in place if uniquely owned, or clones the data to a new Arc if shared—guaranteeing consistent behavior regardless of clone history.
🔧 Suggested fix (clone-on-write)
pub fn with_decoders(mut self, decoders: Vec<Box<dyn InstructionDecoder>>) -> Self {
- // Since with_decoders takes self by value, there can only be one reference
- // to the Arc at this point, so get_mut will succeed
- if let Some(ref mut arc) = self.decoder_registry {
- if let Some(registry) = Arc::get_mut(arc) {
- registry.register_all(decoders);
- return self;
- }
- }
+ if let Some(ref mut arc) = self.decoder_registry {
+ // Clone-on-write: keeps existing custom decoders even when shared
+ let registry = Arc::make_mut(arc);
+ registry.register_all(decoders);
+ return self;
+ }
// Create new registry if none exists
let mut registry = DecoderRegistry::new();
registry.register_all(decoders);
self.decoder_registry = Some(Arc::new(registry));
self
}📝 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.
| /// Register custom decoders | |
| pub fn with_decoders(mut self, decoders: Vec<Box<dyn InstructionDecoder>>) -> Self { | |
| // Since with_decoders takes self by value, there can only be one reference | |
| // to the Arc at this point, so get_mut will succeed | |
| if let Some(ref mut arc) = self.decoder_registry { | |
| if let Some(registry) = Arc::get_mut(arc) { | |
| registry.register_all(decoders); | |
| return self; | |
| } | |
| } | |
| // Create new registry if none exists | |
| let mut registry = DecoderRegistry::new(); | |
| registry.register_all(decoders); | |
| self.decoder_registry = Some(Arc::new(registry)); | |
| self | |
| /// Register custom decoders | |
| pub fn with_decoders(mut self, decoders: Vec<Box<dyn InstructionDecoder>>) -> Self { | |
| if let Some(ref mut arc) = self.decoder_registry { | |
| // Clone-on-write: keeps existing custom decoders even when shared | |
| let registry = Arc::make_mut(arc); | |
| registry.register_all(decoders); | |
| return self; | |
| } | |
| // Create new registry if none exists | |
| let mut registry = DecoderRegistry::new(); | |
| registry.register_all(decoders); | |
| self.decoder_registry = Some(Arc::new(registry)); | |
| self | |
| } |
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder/src/config.rs` around lines 119 - 133, The
with_decoders method currently uses Arc::get_mut on self.decoder_registry which
fails when the Arc is shared and causes existing decoders to be dropped; replace
the get_mut logic with Arc::make_mut so the DecoderRegistry is cloned when
necessary and modifications are applied to the unique instance. Specifically, in
with_decoders operate on Arc::make_mut(arc) (or create a new Arc if
decoder_registry is None), then call registry.register_all(decoders) on the
mutable reference returned, and set self.decoder_registry appropriately so
existing decoders are preserved across clones.
| impl EnhancedTransactionLog { | ||
| /// Create a new empty transaction log with basic info | ||
| pub fn new(signature: Signature, slot: u64) -> Self { | ||
| Self { | ||
| signature, | ||
| slot, | ||
| status: TransactionStatus::Unknown, | ||
| fee: 0, | ||
| compute_used: 0, | ||
| compute_total: 1_400_000, | ||
| instructions: Vec::new(), | ||
| account_changes: Vec::new(), | ||
| program_logs_pretty: String::new(), | ||
| light_events: Vec::new(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded default compute_total may not reflect actual transaction limits.
The compute_total field defaults to 1_400_000 in EnhancedTransactionLog::new(). While this was the historical default, Solana's compute budget has evolved. Consider making this configurable or documenting this as a legacy default.
💡 Consider making compute_total configurable
impl EnhancedTransactionLog {
/// Create a new empty transaction log with basic info
- pub fn new(signature: Signature, slot: u64) -> Self {
+ pub fn new(signature: Signature, slot: u64) -> Self {
+ Self::with_compute_total(signature, slot, 1_400_000)
+ }
+
+ /// Create a new transaction log with custom compute total
+ pub fn with_compute_total(signature: Signature, slot: u64, compute_total: u64) -> Self {
Self {
signature,
slot,
status: TransactionStatus::Unknown,
fee: 0,
compute_used: 0,
- compute_total: 1_400_000,
+ compute_total,
instructions: Vec::new(),
account_changes: Vec::new(),
program_logs_pretty: String::new(),
light_events: Vec::new(),
}
}
}🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder/src/types.rs` around lines 28 - 43,
EnhancedTransactionLog::new currently hardcodes compute_total to 1_400_000;
change it to be configurable instead of a magic constant by either adding an
optional parameter to EnhancedTransactionLog::new (e.g., compute_total: u64) or
by reading a DEFAULT_COMPUTE_TOTAL constant / config value (env/config) and
using that value to initialize the compute_total field; update the constructor
signature and call sites that create EnhancedTransactionLog to pass/derive the
proper compute_total so the default is not a hardcoded legacy value.
|
|
||
| let estimated_fee = (transaction.signatures.len() as u64) * 5000; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplistic fee estimation may not reflect actual fees.
The fee calculation signatures.len() * 5000 assumes a flat 5000 lamports per signature, which doesn't account for priority fees or compute unit pricing. For logging purposes this is acceptable, but consider adding a comment noting this is an estimate.
💡 Add clarifying comment
- let estimated_fee = (transaction.signatures.len() as u64) * 5000;
+ // Estimate base fee: 5000 lamports per signature (does not include priority fees)
+ let estimated_fee = (transaction.signatures.len() as u64) * 5000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let estimated_fee = (transaction.signatures.len() as u64) * 5000; | |
| // Estimate base fee: 5000 lamports per signature (does not include priority fees) | |
| let estimated_fee = (transaction.signatures.len() as u64) * 5000; | |
🤖 Prompt for AI Agents
In `@sdk-libs/program-test/src/logging/mod.rs` around lines 224 - 226, The fee
estimate computed as let estimated_fee = (transaction.signatures.len() as u64) *
5000; is a simplistic approximation; update the code around the estimated_fee
variable (referencing estimated_fee and transaction.signatures) to add a concise
comment stating this is an approximate/logging-only estimate that does not
account for priority fees, compute unit pricing, or network fee variability, so
future readers understand it's not the canonical fee calculation.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdk-libs/program-test/src/logging/mod.rs (1)
42-67: Cache the log path to avoid spawningcargo metadataper write.
get_log_file_path()runscargo metadataevery time it’s called, andwrite_to_log_file()invokes it for every log entry. In log-heavy test suites this can be expensive. Consider caching the resolved path in aOnceLock.♻️ Suggested refactor (cache path)
+static LOG_PATH: std::sync::OnceLock<PathBuf> = std::sync::OnceLock::new(); + fn get_log_file_path() -> PathBuf { - 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; - } - } - } - } - } - - let mut path = PathBuf::from("target"); - path.push("light_program_test.log"); - path + LOG_PATH + .get_or_init(|| { + 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; + } + } + } + } + } + + let mut path = PathBuf::from("target"); + path.push("light_program_test.log"); + path + }) + .clone() }sdk-libs/instruction-decoder/src/registry.rs (1)
64-93: Two parallel decoding paths may cause maintenance burden.The standalone
decode_instructionfunction (string-based dispatch) andDecoderRegistry.decode(Pubkey-based dispatch) represent parallel decoding paths. Over time, these could diverge if new programs are added to one but not the other.Consider deprecating
decode_instructionin favor ofDecoderRegistryor havingdecode_instructiondelegate to a default registry instance.♻️ Suggested approach
/// Decode instruction data for known programs /// /// Note: Consider using `DecoderRegistry::default().decode()` for /// trait-based decoding with custom decoder support. pub fn decode_instruction( program_id: &Pubkey, data: &[u8], accounts: &[AccountMeta], ) -> Option<ParsedInstructionData> { // Keep existing implementation for backward compatibility, // or delegate to registry internally // ... }
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder-derive/src/crate_context.rs`:
- Around line 246-261: The current test_find_module_file_sibling only checks
path string construction; update it to actually exercise resolution by creating
temporary directory trees and real files and asserting the resolver returns the
expected path. Use the actual resolver function (find_module_file_sibling or
whatever module-resolution function in crate_context.rs) and add table-driven
cases for lib.rs, main.rs, mod.rs and foo.rs vs foo/mod.rs (sibling vs nested)
so each case creates the on-disk layout in a tempdir, calls the resolution
function, and asserts the returned Path matches the created file; keep the test
names and use Rust's tempdir/TempDir helpers to cleanup.
- Around line 212-243: The function find_module_file currently checks sibling
module paths before parent-namespaced paths, causing modules declared in
non-root files (e.g., src/foo.rs with mod bar;) to wrongly resolve to src/bar.rs
when src/bar.rs exists; update find_module_file to first detect if parent_name
is "mod", "lib", or "main" (treating "main" like "lib") and in the non-root case
(parent_name not one of those) check the parent-namespaced paths
(parent_dir/parent_name/mod_name.rs and parent_dir/parent_name/mod_name/mod.rs)
before the sibling/standard paths so src/foo/bar.rs and src/foo/bar/mod.rs are
preferred for crate::foo::bar; apply the same change in the other
crate_context.rs mentioned.
- Around line 174-202: process_inline_modules currently ignores external modules
(Item::Mod with no content) nested inside inline modules, causing silent
omission of their structs; update process_inline_modules signature to accept
module_dir: &Path (or &str) and when encountering an external module
(item_mod.content is None) call find_module_file(mod_name, module_dir) (same
resolution logic used in parse_recursive), load and parse that file into a
ParsedModule and insert it into modules, and ensure recursive calls pass
module_dir through to handle deeper nesting; update all call sites (including
the recursive Self::process_inline_modules call and where process_inline_modules
is invoked from parse_recursive) to provide module_dir so external nested mods
are resolved.
In `@sdk-libs/instruction-decoder/Cargo.toml`:
- Around line 12-22: The Cargo.toml currently pulls sdk-libs crates
(light-sdk-types and light-instruction-decoder-derive) which violates the
sdk-to-sdk dependency rule; move the shared types and derive macro into a
program-libs crate (create or reuse a program-libs crate following the
light-token-interface pattern), update code that uses symbols from
light-sdk-types and light-instruction-decoder-derive to import them from the new
program-libs crate, and then remove the light-sdk-types and
light-instruction-decoder-derive entries from this crate's [dependencies] so the
instruction-decoder crate only depends on program-libs (and allowed external
crates like borsh/serde/bs58) instead of other sdk-libs.
In `@sdk-libs/instruction-decoder/src/config.rs`:
- Around line 31-36: The decoder_registry field is skipped during
deserialization causing it to be None; add a serde default so deserializing
EnhancedLoggingConfig restores a usable registry: define an associated function
like EnhancedLoggingConfig::default_decoder_registry() ->
Option<Arc<DecoderRegistry>> that returns a reinitialized registry (e.g.,
Some(Arc::new(DecoderRegistry::with_builtins()))) and change the field attribute
to #[serde(default = "EnhancedLoggingConfig::default_decoder_registry",
skip_serializing)] pub decoder_registry: Option<Arc<DecoderRegistry>>; also
ensure Default/impl for EnhancedLoggingConfig uses the same initializer so new
instances and deserialized instances both get the registry.
In `@sdk-libs/instruction-decoder/src/formatter.rs`:
- Around line 1132-1138: The boolean expression that sets is_simple in
formatter.rs mixes && and || without explicit grouping, which relies on
precedence and can be hard to read; update the is_simple assignment (the
variable named is_simple that inspects trimmed) to add explicit parentheses
around the combined "no delimiters" condition—i.e., wrap the sequence
!trimmed.contains('(') && !trimmed.contains('{') && !trimmed.contains('[') &&
!trimmed.contains('\n')—so the overall intent reads clearly as (no-delimiters)
|| (trimmed.parse::<i64>().is_ok()) || (trimmed == "true" || trimmed == "false")
|| trimmed.is_empty(); keep the same logical result but make the grouping
explicit for readability and future maintenance.
- Around line 1246-1314: The recursive renderer write_decoded_field currently
descends into DecodedField::children without a depth guard; add the same check
used in write_instruction (compare depth against
self.config.max_inner_instruction_depth) and when the limit is reached avoid
recursing further (e.g., emit a short marker like "<max depth reached>" or skip
children) to prevent stack overflow from maliciously deep DecodedField trees
created via with_children(); ensure the check uses the same config key
self.config.max_inner_instruction_depth and apply it before iterating over
field.children so behavior matches existing inner-instruction limits.
In `@sdk-libs/instruction-decoder/src/lib.rs`:
- Around line 1-64: The crate currently depends on light-sdk-types; remove that
dependency from Cargo.toml and eliminate any direct imports/re-exports that
reference types from that SDK lib by either (A) moving the required shared types
(e.g., AccountAccess, AccountChange, AddressAssignment, AddressParam,
CompressedAccountInfo, CompressedAccountSummary, EnhancedInstructionLog,
EnhancedTransactionLog, FeeSummary, InputAccountData, LightProtocolEvent,
MerkleTreeChange, OutputAccountData, ParsedInstructionData, ProofSummary,
TransactionStatus) into a program-lib crate and update this crate to import them
from that program-lib, or (B) define minimal equivalents locally inside this
crate (e.g., in the types module) and update all re-exports and uses (see pub
use types::... and any code that references those symbols) to point to the new
location; also ensure any cfg(not(target_os = "solana")) re-exports (config,
formatter, programs, registry, types) are updated to import the moved/created
types so the public API and the derive macro (light-instruction-decoder-derive)
continue to work without reliance on light-sdk-types.
In `@sdk-libs/instruction-decoder/src/registry.rs`:
- Around line 20-21: Replace the string SYSTEM_PROGRAM_ID constant with a Pubkey
constant (e.g. define SYSTEM_PROGRAM_PUBKEY: Pubkey) and update all places that
currently do comparisons by calling to_string() (the comparisons near where
SYSTEM_PROGRAM_ID is used) to compare Pubkey values or raw bytes directly (e.g.
my_pubkey == SYSTEM_PROGRAM_PUBKEY or my_pubkey.as_ref() ==
SYSTEM_PROGRAM_PUBKEY.as_ref()); also add the needed import
(solana_program::pubkey::Pubkey) and, if necessary, initialize the Pubkey
constant via a const-friendly constructor or a once/lazy initializer so you
avoid runtime string allocations.
- Around line 1173-1180: The program_id() function currently returns a hardcoded
Pubkey byte array that decodes to the wrong address; replace that literal array
with the existing constant light_sdk_types::constants::LIGHT_SYSTEM_PROGRAM_ID
so program_id() returns Pubkey::from(LIGHT_SYSTEM_PROGRAM_ID) (or otherwise
constructs the Pubkey from that constant) — update the program_id()
implementation in registry.rs to reference LIGHT_SYSTEM_PROGRAM_ID the same way
formatter.rs does.
In `@sdk-libs/program-test/src/logging/mod.rs`:
- Around line 258-282: The mapping of inner instructions is wrong because the
code uses enumerate() over inner_instructions_list; instead iterate over each
InnerInstructions element and use its explicit index field
(InnerInstructions.index as usize) to locate the owning top-level instruction in
instructions, and pass the element's instructions (inner_list.instructions) into
parse_inner_instructions (keeping the same args:
&transaction.message.account_keys, &transaction.message, 1, config); replace the
enumerate loop with this index-based lookup so sparse inner-instruction entries
attach to the correct top-level instruction.
♻️ Duplicate comments (4)
sdk-libs/instruction-decoder/src/config.rs (1)
125-139: Preserve existing decoders when cloning withwith_decoders.
Arc::get_mutfails when the registry is shared (e.g., afterclone()), which causes existing custom decoders to be dropped and replaced by a fresh registry. UseArc::make_mutto keep clone‑on‑write semantics.🔧 Suggested fix (clone‑on‑write)
- if let Some(ref mut arc) = self.decoder_registry { - if let Some(registry) = Arc::get_mut(arc) { - registry.register_all(decoders); - return self; - } - } + if let Some(ref mut arc) = self.decoder_registry { + let registry = Arc::make_mut(arc); + registry.register_all(decoders); + return self; + }sdk-libs/instruction-decoder-derive/src/lib.rs (3)
191-205: Use standard snake_case conversion for Anchor discriminators.The custom
to_snake_casesplits acronyms intoa_t_a, which breaks Anchor’s discriminator hashing (e.g.,CreateATA). Useheck::ToSnakeCaseinstead.🔧 Suggested fix (use heck)
-fn to_snake_case(name: &str) -> String { - let mut result = String::new(); - for (i, c) in name.chars().enumerate() { - if c.is_uppercase() { - if i > 0 { - result.push('_'); - } - result.push(c.to_ascii_lowercase()); - } else { - result.push(c); - } - } - result -} +fn to_snake_case(name: &str) -> String { + use heck::ToSnakeCase; + name.to_snake_case() +}Add to
sdk-libs/instruction-decoder-derive/Cargo.toml:heck = "0.4"
398-402: Guard against u8 discriminator truncation.Casting to
u8can silently wrap when the discriminator exceeds 255 (or when the enum has >256 variants), resulting in wrong dispatch. Validate the range first.🐛 Suggested fix
- let disc_byte = variant_info.discriminator.unwrap_or(variant_index as u32) as u8; + let disc_u32 = variant_info.discriminator.unwrap_or(variant_index as u32); + if disc_u32 > u8::MAX as u32 { + return Err(Error::new_spanned( + variant, + "discriminator exceeds u8 range; use discriminator_size = 4 or a valid u8 value", + )); + } + let disc_byte = disc_u32 as u8;
675-683: Avoid underflow when reporting remaining byte length.
remaining.len() - offsetcan underflow ifoffsetexceeds the buffer, which can panic or wrap. Usesaturating_sub.🐛 Suggested fix
- format!("({}bytes)", remaining.len() - `#offset`), + format!("({}bytes)", remaining.len().saturating_sub(`#offset`)),- format!("({}bytes)", remaining.len() - `#offset`), + format!("({}bytes)", remaining.len().saturating_sub(`#offset`)),Also applies to: 749-753
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_find_module_file_sibling() { | ||
| // This test verifies the path construction logic | ||
| let parent = Path::new("/some/src"); | ||
| let paths_checked = vec![ | ||
| parent.join("foo.rs"), | ||
| parent.join("foo").join("mod.rs"), | ||
| ]; | ||
| // Just verify the paths are constructed correctly | ||
| assert!(paths_checked[0].to_str().unwrap().contains("foo.rs")); | ||
| assert!(paths_checked[1].to_str().unwrap().contains("mod.rs")); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add tests that exercise actual resolution behavior.
The current test only checks string construction and won’t catch resolution bugs like sibling vs nested lookup. Consider table‑driven tests for lib.rs/main.rs/mod.rs and foo.rs cases.
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/src/crate_context.rs` around lines 246 -
261, The current test_find_module_file_sibling only checks path string
construction; update it to actually exercise resolution by creating temporary
directory trees and real files and asserting the resolver returns the expected
path. Use the actual resolver function (find_module_file_sibling or whatever
module-resolution function in crate_context.rs) and add table-driven cases for
lib.rs, main.rs, mod.rs and foo.rs vs foo/mod.rs (sibling vs nested) so each
case creates the on-disk layout in a tempdir, calls the resolution function, and
asserts the returned Path matches the created file; keep the test names and use
Rust's tempdir/TempDir helpers to cleanup.
| /// Truncate long byte arrays in output (show first/last few bytes) | ||
| pub truncate_byte_arrays: bool, | ||
| /// Decoder registry containing built-in and custom decoders | ||
| /// Wrapped in Arc so it can be shared across clones instead of being lost | ||
| #[serde(skip)] | ||
| decoder_registry: Option<Arc<DecoderRegistry>>, |
There was a problem hiding this comment.
Deserialized configs lose the decoder registry.
decoder_registry is skipped by serde, so deserializing EnhancedLoggingConfig yields None and decoding silently stops unless callers reinitialize manually. Consider providing a serde default to restore the registry on load.
🔧 Suggested fix (serde default)
+fn default_decoder_registry() -> Option<Arc<DecoderRegistry>> {
+ Some(Arc::new(DecoderRegistry::new()))
+}
+
- #[serde(skip)]
+ #[serde(skip, default = "default_decoder_registry")]
decoder_registry: Option<Arc<DecoderRegistry>>,🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder/src/config.rs` around lines 31 - 36, The
decoder_registry field is skipped during deserialization causing it to be None;
add a serde default so deserializing EnhancedLoggingConfig restores a usable
registry: define an associated function like
EnhancedLoggingConfig::default_decoder_registry() ->
Option<Arc<DecoderRegistry>> that returns a reinitialized registry (e.g.,
Some(Arc::new(DecoderRegistry::with_builtins()))) and change the field attribute
to #[serde(default = "EnhancedLoggingConfig::default_decoder_registry",
skip_serializing)] pub decoder_registry: Option<Arc<DecoderRegistry>>; also
ensure Default/impl for EnhancedLoggingConfig uses the same initializer so new
instances and deserialized instances both get the registry.
| // Extract inner instructions from LiteSVM metadata | ||
| let inner_instructions_list = match result { | ||
| Ok(meta) => &meta.inner_instructions, | ||
| Err(failed) => &failed.meta.inner_instructions, | ||
| }; | ||
|
|
||
| // Apply decoder to instructions if enabled | ||
| if config.decode_light_instructions { | ||
| for instruction in instructions.iter_mut() { | ||
| instruction.decode(config); | ||
| } | ||
|
|
||
| // Populate inner instructions for each top-level instruction | ||
| for (instruction_index, inner_list) in inner_instructions_list.iter().enumerate() { | ||
| if let Some(instruction) = instructions.get_mut(instruction_index) { | ||
| instruction.inner_instructions = parse_inner_instructions( | ||
| inner_list, | ||
| &transaction.message.account_keys, | ||
| &transaction.message, | ||
| 1, | ||
| config, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate LiteSVM-related imports and understand the metadata structure
rg -A 5 "inner_instructions" sdk-libs/program-test/src/logging/mod.rs | head -40Repository: Lightprotocol/light-protocol
Length of output: 1526
🏁 Script executed:
# Search for LiteSVM metadata struct definition
rg "struct.*TransactionResult|struct.*TransactionMeta" --type rs | head -20Repository: Lightprotocol/light-protocol
Length of output: 98
🏁 Script executed:
# Check for any LiteSVM dependency or import statements
rg "lite.*svm|LiteSVM" sdk-libs/program-test/ --type rs | head -20Repository: Lightprotocol/light-protocol
Length of output: 98
🏁 Script executed:
# Look at the Cargo.toml to understand LiteSVM version and structure
cat sdk-libs/program-test/Cargo.toml | grep -A 5 -i liteRepository: Lightprotocol/light-protocol
Length of output: 338
🏁 Script executed:
# Check the imports and understand what TransactionResult is
head -50 sdk-libs/program-test/src/logging/mod.rs | cat -nRepository: Lightprotocol/light-protocol
Length of output: 2360
🏁 Script executed:
# Find the TransactionResult type and its metadata field
rg "TransactionResult|\.meta\." sdk-libs/program-test/src/logging/mod.rs | head -30Repository: Lightprotocol/light-protocol
Length of output: 426
🏁 Script executed:
# Check workspace dependencies to find litesvm location
cat Cargo.toml | grep -A 10 "workspace.dependencies"Repository: Lightprotocol/light-protocol
Length of output: 381
🏁 Script executed:
# Look for the type definition of inner_instructions in litesvm or workspace
fd "Cargo.toml" | xargs grep -l "litesvm" | head -5Repository: Lightprotocol/light-protocol
Length of output: 192
🏁 Script executed:
# Find where litesvm is defined in the workspace
find . -name "Cargo.toml" -type f | xargs grep -l "^name = \"litesvm\"" 2>/dev/nullRepository: Lightprotocol/light-protocol
Length of output: 56
🏁 Script executed:
# Check the root workspace definition more carefully
head -100 Cargo.tomlRepository: Lightprotocol/light-protocol
Length of output: 2931
🏁 Script executed:
# Search for how inner_instructions are accessed elsewhere in the codebase
rg "\.inner_instructions" --type rust -B 2 -A 2 | head -60Repository: Lightprotocol/light-protocol
Length of output: 4224
🏁 Script executed:
# Look for any documentation or tests that show inner_instructions usage
rg "inner_instructions.*enumerate|enumerate.*inner_instructions" --type rust -B 3 -A 3Repository: Lightprotocol/light-protocol
Length of output: 1908
🏁 Script executed:
# Look at how litesvm is actually imported - find workspace member Cargo.toml details
rg "litesvm" Cargo.toml -A 5 -B 5Repository: Lightprotocol/light-protocol
Length of output: 394
🏁 Script executed:
# Check the actual structure - look for InnerInstructions (plural) struct definition
rg "struct InnerInstruction" --type rust -B 2 -A 8Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check rpc.rs to understand the flatten() usage better
cat sdk-libs/program-test/src/program_test/rpc.rs | head -100Repository: Lightprotocol/light-protocol
Length of output: 2776
🏁 Script executed:
# Look at the full logging context around the problematic code
sed -n '250,290p' sdk-libs/program-test/src/logging/mod.rsRepository: Lightprotocol/light-protocol
Length of output: 1478
🏁 Script executed:
# Check LiteSVM 0.7 source to understand the metadata structure
# First, try to find any documentation or look at how it's used more broadly
rg "meta\.inner_instructions" --type rust -B 3 -A 3 | head -80Repository: Lightprotocol/light-protocol
Length of output: 1623
🏁 Script executed:
# Look at the specific flatten() usage in rpc.rs to understand the type structure
sed -n '1,150p' sdk-libs/program-test/src/program_test/rpc.rs | tail -80Repository: Lightprotocol/light-protocol
Length of output: 2521
🏁 Script executed:
# Search for any tests or examples that use inner_instructions with explicit indices
rg "inner_instructions\[" --type rust -B 2 -A 2Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check if there are any comments or documentation about inner_instructions structure
rg -i "inner.*instruction.*index|index.*inner.*instruction" --type rust -B 2 -A 2 | head -40Repository: Lightprotocol/light-protocol
Length of output: 1570
🏁 Script executed:
# Most importantly, check the actual Solana SDK types being used
# Look for the InnerInstruction type definition in the codebase or imports
rg "from solana_sdk.*inner_instruction|use.*InnerInstruction" --type rust -B 1 -A 1 | head -30Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# The key insight: check if there's an explicit index field by looking at how inner_instructions are constructed or handled
rg "InnerInstructions|inner_instructions.*index" --type rust -B 2 -A 2 | head -60Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check the LiteSVM types more carefully - look at what the metadata contains
rg "TransactionResult|TransactionMeta" --type rust -B 3 -A 5 | head -100Repository: Lightprotocol/light-protocol
Length of output: 3979
🏁 Script executed:
# Critical: check how the rpc.rs code actually iterates - the flatten() is key
sed -n '100,180p' sdk-libs/program-test/src/program_test/rpc.rsRepository: Lightprotocol/light-protocol
Length of output: 2932
🏁 Script executed:
# Look at CLAUDE.md or any project documentation
find . -name "CLAUDE.md" -o -name "claude.md" | head -5Repository: Lightprotocol/light-protocol
Length of output: 256
🏁 Script executed:
# Search for any explicit handling of inner instruction indices with assignment or mapping
rg "\.index\s*=" --type rust -B 3 -A 3 | grep -i "inner\|instruction"Repository: Lightprotocol/light-protocol
Length of output: 306
🏁 Script executed:
# Check the main CLAUDE.md for insights
cat CLAUDE.mdRepository: Lightprotocol/light-protocol
Length of output: 11430
🏁 Script executed:
# Most importantly, let's check the litesvm 0.7 documentation or types
# Try to understand the actual structure by looking at how it's used in the codebase
# Check the rpc.rs file more carefully - particularly around the inner_instructions handling
sed -n '1,250p' sdk-libs/program-test/src/program_test/rpc.rs | grep -A 20 "inner_instructions"Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Search for where inner_instructions are actually constructed or populated to understand their structure
rg "inner_instructions\s*=" --type rust -B 5 -A 5Repository: Lightprotocol/light-protocol
Length of output: 2675
🏁 Script executed:
# Critical check: Look at what type inner_instructions actually is
# Check the exact type by looking at what .iter() returns
rg "for.*in.*inner_instructions" --type rust -B 3 -A 5 | head -80Repository: Lightprotocol/light-protocol
Length of output: 5125
🏁 Script executed:
# Look for the actual LiteSVM source or check how the structure is documented
# Check if there's a dependency on solana-transaction-status which defines the structure
grep -r "solana-transaction-status" Cargo.toml | head -5Repository: Lightprotocol/light-protocol
Length of output: 152
🏁 Script executed:
# Key insight: check what .flatten() on inner_instructions means
sed -n '200,250p' sdk-libs/program-test/src/program_test/rpc.rsRepository: Lightprotocol/light-protocol
Length of output: 1828
🏁 Script executed:
# Let me search for tests that might show the actual structure
find . -name "*.rs" -type f -exec grep -l "inner_instructions.*test\|test.*inner_instructions" {} \; | head -5Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# The critical clue is in rpc.rs - let's see the exact flatten() usage
sed -n '130,180p' sdk-libs/program-test/src/program_test/rpc.rs | cat -nRepository: Lightprotocol/light-protocol
Length of output: 2312
🏁 Script executed:
# Check if LiteSVM uses solana-transaction-status types directly
# The structure should clarify whether it's sparse (with index field) or sequential
rg "InnerInstructions|inner_instructions.*index" --type rust -B 2 -A 2 | head -100Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Most importantly, let's check the actual Solana SDK's InnerInstruction struct
# by looking at how it's used with an explicit index field
web_search
Solana SDK InnerInstructions struct index fieldRepository: Lightprotocol/light-protocol
Length of output: 167
🌐 Web query:
Solana SDK InnerInstructions struct solana-transaction-status
💡 Result:
The InnerInstructions struct (crate solana-transaction-status) is:
- pub struct InnerInstructions {
- index: u8, // transaction instruction index
- instructions: Vec, // list of inner instructions
}
In the RPC/JSON response, each InnerInstructions object has the same fields; each inner instruction item contains programIdIndex, accounts (indices), and data (base58 string). [1][2]
Sources:
- Rust docs for solana-transaction-status::InnerInstructions. [1]
- Solana RPC JSON "Inner Instructions" structure. [2]
Fix inner-instruction mapping to use explicit index field instead of enumerate().
Solana's InnerInstructions struct contains an explicit index: u8 field that indicates which top-level instruction owns those inner instructions. The current code uses enumerate() to assign indices, which will mismap inner instructions when the list is sparse—for example, if only instructions 0 and 2 have inner instructions, the second element will incorrectly be attached to instruction 1 instead of instruction 2.
Use inner_list.index from each InnerInstructions element instead of relying on iteration order:
for inner_list in inner_instructions_list.iter() {
let instruction_index = inner_list.index as usize;
if let Some(instruction) = instructions.get_mut(instruction_index) {
instruction.inner_instructions = parse_inner_instructions(
&inner_list.instructions,
&transaction.message.account_keys,
&transaction.message,
1,
config,
);
}
}🤖 Prompt for AI Agents
In `@sdk-libs/program-test/src/logging/mod.rs` around lines 258 - 282, The mapping
of inner instructions is wrong because the code uses enumerate() over
inner_instructions_list; instead iterate over each InnerInstructions element and
use its explicit index field (InnerInstructions.index as usize) to locate the
owning top-level instruction in instructions, and pass the element's
instructions (inner_list.instructions) into parse_inner_instructions (keeping
the same args: &transaction.message.account_keys, &transaction.message, 1,
config); replace the enumerate loop with this index-based lookup so sparse
inner-instruction entries attach to the correct top-level instruction.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder-derive/CLAUDE.md`:
- Around line 16-25: Update the fenced code block in CLAUDE.md that shows the
src/ module tree to include a language specifier (e.g., change the opening fence
from ``` to ```text) so the block renders and lints correctly; locate the
module-structure block near the top of the file (the tree listing including
lib.rs, utils.rs, parsing.rs, builder.rs, derive_impl.rs, attribute_impl.rs,
crate_context.rs) and add the language tag to the opening fence.
In `@sdk-libs/instruction-decoder-derive/src/attribute_impl.rs`:
- Around line 170-207: The call to CrateContext::parse_from_manifest() inside
extract_instruction_info causes repeated manifest parsing for each
macro-processed module; refactor extract_instruction_info to reuse a cached
CrateContext (e.g., accept an Option<&CrateContext> or maintain a
once_cell/static cache retrieved before parsing) and only call
CrateContext::parse_from_manifest() when account_names are actually needed (use
extract_context_type(&func.sig) first), or make parsing conditional so
crate_ctx.get_struct_field_names is invoked only when necessary; reference
functions/types: extract_instruction_info, CrateContext::parse_from_manifest,
crate_ctx, get_struct_field_names, and extract_context_type to locate the
change.
In `@sdk-libs/instruction-decoder-derive/src/builder.rs`:
- Around line 458-467: The code computes remaining.len() - `#offset` which can
underflow; update the unknown-type size calculation in this match arm (the block
that pushes light_instruction_decoder::DecodedField::new) to use a saturating
subtraction instead of plain - (e.g., remaining.len().saturating_sub(`#offset`) or
equivalent) so it never underflows; ensure the interpolated `#offset` has a
compatible integer type or cast as needed before calling saturating_sub.
In `@sdk-libs/instruction-decoder-derive/src/parsing.rs`:
- Around line 362-367: The fallback to crate::ID in program_id_source (when
self.program_id is None) can be surprising if the module doesn't define
declare_id!, so update program_id_source (and its doc comment) to document this
assumption and emit a helpful compile-time diagnostics when the fallback is
used: detect the None case in program_id_source (or where
ProgramIdSource::Path(syn::parse_quote!(crate::ID)) is returned) and either add
a clear doc comment explaining "defaults to crate::ID when declare_id! is not
found" or return/produce a syn/compile_error with guidance to define declare_id!
or set program_id explicitly; reference program_id_source, program_id,
ProgramIdSource, and the declare_id! fallback in the message so users can locate
the cause.
- Around line 172-184: account_names_code currently emits
<#ty<'_>>::ACCOUNT_NAMES which assumes the referenced accounts type has exactly
one lifetime; instead inspect the TypeRef stored in AccountNamesSpec::TypeRef
(the syn::Type/syn::TypePath/generics) and emit the appropriate path: if the
type already has a lifetime/generic parameter, emit <#ty<'_>>::ACCOUNT_NAMES (or
insert the missing lifetime into its generics), otherwise emit
<#ty>::ACCOUNT_NAMES; keep the rest of the mapping (.iter().map(|s|
s.to_string()).collect()) the same so both branches produce Vec<String>. Ensure
this logic is implemented in account_names_code where
AccountNamesSpec::TypeRef(ty) is handled.
In `@sdk-libs/macros/src/light_pdas/accounts/builder.rs`:
- Around line 432-454: The generated code in generate_account_names_impl
references light_instruction_decoder::HasAccountNames (and defines the const
ACCOUNT_NAMES for that trait) but the crate dependency is missing; add
light-instruction-decoder to the [dependencies] in sdk-libs/macros/Cargo.toml
(the version can match your workspace or a compatible semver), so builds that
compile the HasAccountNames impl (off-chain, #[cfg(not(target_os = "solana"))])
succeed.
♻️ Duplicate comments (2)
sdk-libs/instruction-decoder-derive/src/crate_context.rs (1)
306-318: Test validates path construction but not resolution behavior.The test verifies path strings are constructed correctly but doesn't exercise actual file resolution. Since this is a proc-macro crate where integration testing is more complex, consider adding a test that creates a temp directory structure to validate the resolution priority (namespaced vs sibling paths).
sdk-libs/instruction-decoder-derive/src/builder.rs (1)
126-140: Guard against u8 discriminator truncation.This issue was flagged in a previous review. The cast at line 128 can silently truncate values > 255. If an enum has more than 256 variants or an explicit discriminator exceeds
u8::MAX, this produces incorrect dispatch silently.🐛 Suggested fix
1 => { - let disc = self.explicit_discriminators[index].unwrap_or(index as u32) as u8; + let disc_u32 = self.explicit_discriminators[index].unwrap_or(index as u32); + if disc_u32 > u8::MAX as u32 { + return Err(syn::Error::new( + variant.ident.span(), + format!("discriminator {} exceeds u8::MAX; use discriminator_size = 4 or 8", disc_u32), + )); + } + let disc = disc_u32 as u8;
| ``` | ||
| src/ | ||
| ├── lib.rs # Macro entry points only (~100 lines) | ||
| ├── utils.rs # Case conversion, discriminator, error handling | ||
| ├── parsing.rs # Darling-based attribute parsing structs | ||
| ├── builder.rs # InstructionDecoderBuilder (code generation) | ||
| ├── derive_impl.rs # #[derive(InstructionDecoder)] implementation | ||
| ├── attribute_impl.rs # #[instruction_decoder] attribute implementation | ||
| └── crate_context.rs # Recursive crate parsing for Accounts struct discovery | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The module structure code block should have a language specifier for proper rendering and linting compliance.
📝 Suggested fix
-```
+```text
src/
├── lib.rs # Macro entry points only (~100 lines)
├── utils.rs # Case conversion, discriminator, error handling🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/CLAUDE.md` around lines 16 - 25, Update
the fenced code block in CLAUDE.md that shows the src/ module tree to include a
language specifier (e.g., change the opening fence from ``` to ```text) so the
block renders and lints correctly; locate the module-structure block near the
top of the file (the tree listing including lib.rs, utils.rs, parsing.rs,
builder.rs, derive_impl.rs, attribute_impl.rs, crate_context.rs) and add the
language tag to the opening fence.
| /// Extract public function information from an Anchor program module. | ||
| fn extract_instruction_info(module: &ItemMod) -> syn::Result<Vec<InstructionInfo>> { | ||
| // Parse entire crate to find Accounts structs | ||
| let crate_ctx = CrateContext::parse_from_manifest()?; | ||
|
|
||
| let mut instructions = Vec::new(); | ||
|
|
||
| if let Some(ref content) = module.content { | ||
| for item in &content.1 { | ||
| if let syn::Item::Fn(func) = item { | ||
| // Only include public functions | ||
| if matches!(func.vis, syn::Visibility::Public(_)) { | ||
| let name = func.sig.ident.to_string(); | ||
|
|
||
| // Extract Context<T> type from first parameter and look up account names | ||
| let account_names = if let Some(type_name) = extract_context_type(&func.sig) { | ||
| crate_ctx | ||
| .get_struct_field_names(&type_name) | ||
| .unwrap_or_default() | ||
| } else { | ||
| Vec::new() | ||
| }; | ||
|
|
||
| // Extract params type from second parameter | ||
| let params_type = extract_params_type(&func.sig); | ||
|
|
||
| instructions.push(InstructionInfo { | ||
| name, | ||
| account_names, | ||
| params_type, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(instructions) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching CrateContext across macro invocations.
CrateContext::parse_from_manifest() parses Cargo.toml and potentially scans source files each time the macro processes a module. In a crate with multiple #[instruction_decoder]-annotated modules, this work is repeated unnecessarily.
While proc macros can't easily share state across invocations, you might consider:
- Lazy-loading and caching within a single macro expansion if multiple instructions reference the same context
- Making
CrateContextparsing optional when account names aren't needed
For now this may be acceptable if usage is limited, but keep an eye on compile times.
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/src/attribute_impl.rs` around lines 170 -
207, The call to CrateContext::parse_from_manifest() inside
extract_instruction_info causes repeated manifest parsing for each
macro-processed module; refactor extract_instruction_info to reuse a cached
CrateContext (e.g., accept an Option<&CrateContext> or maintain a
once_cell/static cache retrieved before parsing) and only call
CrateContext::parse_from_manifest() when account_names are actually needed (use
extract_context_type(&func.sig) first), or make parsing conditional so
crate_ctx.get_struct_field_names is invoked only when necessary; reference
functions/types: extract_instruction_info, CrateContext::parse_from_manifest,
crate_ctx, get_struct_field_names, and extract_context_type to locate the
change.
| /// Get program ID source, defaulting to `crate::ID` if not specified. | ||
| pub fn program_id_source(&self) -> ProgramIdSource { | ||
| self.program_id | ||
| .clone() | ||
| .unwrap_or_else(|| ProgramIdSource::Path(syn::parse_quote!(crate::ID))) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the crate::ID fallback assumption.
When program_id isn't specified and declare_id! isn't found in the module, this defaults to crate::ID. While this works for standard Anchor programs, users may encounter confusing errors if their crate structure differs.
Consider adding a doc comment or emitting a helpful compile error when the fallback is used:
📝 Documentation suggestion
/// Get program ID source, defaulting to `crate::ID` if not specified.
+ ///
+ /// Note: The default assumes the crate exports `pub const ID: Pubkey` at the root,
+ /// which is standard for Anchor programs using `declare_id!`.
pub fn program_id_source(&self) -> ProgramIdSource {🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/src/parsing.rs` around lines 362 - 367,
The fallback to crate::ID in program_id_source (when self.program_id is None)
can be surprising if the module doesn't define declare_id!, so update
program_id_source (and its doc comment) to document this assumption and emit a
helpful compile-time diagnostics when the fallback is used: detect the None case
in program_id_source (or where
ProgramIdSource::Path(syn::parse_quote!(crate::ID)) is returned) and either add
a clear doc comment explaining "defaults to crate::ID when declare_id! is not
found" or return/produce a syn/compile_error with guidance to define declare_id!
or set program_id explicitly; reference program_id_source, program_id,
ProgramIdSource, and the declare_id! fallback in the message so users can locate
the cause.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder-derive/CLAUDE.md`:
- Around line 1-27: Add the required CLAUDE.md sections: create a "Summary"
section with 2–5 concise bullet points describing the crate, a "Used in" section
listing consumers (e.g., SDKs or other crates that import this derive), a
"Navigation" section with pointers to key files/entry points (e.g., lib.rs,
derive_impl.rs, attribute_impl.rs) and usage notes, and a "Source Code
Structure" section that lists the source tree in a library-style breakdown (Core
Types, Parsing, Builders, Utilities, Macros) mirroring the existing src/
listing; ensure each section uses the exact headers Summary, Used in,
Navigation, and Source Code Structure so the file meets the required template.
In `@sdk-libs/instruction-decoder-derive/src/builder.rs`:
- Around line 135-149: The code silently truncates enum variant indices when
discriminator_size == 1 (in generate_match_arms where it computes disc =
self.explicit_discriminators[index].unwrap_or(index as u32) as u8), so add a
validation in InstructionDecoderArgs::validate() (or at start of
generate_match_arms) that counts variants via self.variants().len() and ensures
it does not exceed the capacity for the configured discriminator_size (e.g., 256
for size 1, u32::MAX for size 4); on violation return a syn::Error (using
self.ident.span()) with a clear message about variant count vs discriminator
capacity to prevent silent truncation.
In `@sdk-libs/instruction-decoder-derive/src/parsing.rs`:
- Around line 180-215: Replace the stderr prints in account_names_code (the two
eprintln! calls inside the AccountNamesSpec::TypeRef match arm) with
compile-time diagnostics: produce a proc-macro warning that includes the same
message and the span of the problematic items (use self.ident.span() or the span
from the extracted type) so users get source-location context; you can implement
this either with proc_macro::Diagnostic::spanned (nightly) or with
proc-macro-error's emit_warning!/register_attr_error macros, and keep the same
fallback behavior (returning Vec::new()) when emitting the diagnostic;
references: account_names_code, AccountNamesSpec::TypeRef, extract_struct_name,
crate_ctx, get_struct_field_names, variant_name, struct_name.
♻️ Duplicate comments (3)
sdk-libs/instruction-decoder-derive/CLAUDE.md (1)
16-25: Add language specifier to the module-structure fence.This is the same MD040 issue flagged ранее. Please add a language tag (e.g.,
text).📝 Suggested fix
-``` +```text src/ ├── lib.rs # Macro entry points only (~100 lines) ├── utils.rs # Case conversion, discriminator, error handling ├── parsing.rs # Darling-based attribute parsing structs ├── builder.rs # InstructionDecoderBuilder (code generation) ├── derive_impl.rs # #[derive(InstructionDecoder)] implementation ├── attribute_impl.rs # #[instruction_decoder] attribute implementation └── crate_context.rs # Recursive crate parsing for Accounts struct discoverysdk-libs/instruction-decoder-derive/src/builder.rs (1)
467-476: Potential underflow in unknown type size calculation.The expression
remaining.len() -#offset`` at line 471 can underflow whenoffsetexceeds `remaining.len()`. This happens if earlier field parsing consumed more bytes than expected or if the instruction data is truncated.Additionally, returning
size = 0means all subsequent unknown-type fields will compute from the same offset, producing misleading output for structs with multiple unknown fields.🐛 Suggested fix using saturating_sub
_ => ( quote! { + if remaining.len() > `#offset` { fields.push(light_instruction_decoder::DecodedField::new( `#field_name`, - format!("({}bytes)", remaining.len() - `#offset`), + format!("({}bytes)", remaining.len().saturating_sub(`#offset`)), )); + } }, 0, ),sdk-libs/instruction-decoder-derive/src/parsing.rs (1)
411-416: Document thecrate::IDassumption more explicitly.The doc comment mentions the default but doesn't explain that this assumes the crate follows Anchor's
declare_id!convention (which exportspub const ID: Pubkey). Users with non-standard crate structures may encounter confusing "ID not found" errors.📝 Documentation suggestion
/// Get program ID source, defaulting to `crate::ID` if not specified. + /// + /// # Default Behavior + /// When neither `program_id` attribute nor `declare_id!` macro is found, + /// this defaults to `crate::ID`, which is the standard export from Anchor's + /// `declare_id!` macro. Ensure your crate exports `pub const ID: Pubkey` + /// at the root, or specify `program_id` explicitly. pub fn program_id_source(&self) -> ProgramIdSource {
| # light-instruction-decoder-derive | ||
|
|
||
| Procedural macros for generating `InstructionDecoder` implementations. | ||
|
|
||
| ## Overview | ||
|
|
||
| This crate provides two macros for generating instruction decoders: | ||
|
|
||
| | Macro | Type | Purpose | | ||
| |-------|------|---------| | ||
| | `#[derive(InstructionDecoder)]` | Derive | Generate decoder for instruction enums | | ||
| | `#[instruction_decoder]` | Attribute | Auto-generate from Anchor program modules | | ||
|
|
||
| ## Module Structure | ||
|
|
||
| ``` | ||
| src/ | ||
| ├── lib.rs # Macro entry points only (~100 lines) | ||
| ├── utils.rs # Case conversion, discriminator, error handling | ||
| ├── parsing.rs # Darling-based attribute parsing structs | ||
| ├── builder.rs # InstructionDecoderBuilder (code generation) | ||
| ├── derive_impl.rs # #[derive(InstructionDecoder)] implementation | ||
| ├── attribute_impl.rs # #[instruction_decoder] attribute implementation | ||
| └── crate_context.rs # Recursive crate parsing for Accounts struct discovery | ||
| ``` | ||
|
|
||
| ## Key Features |
There was a problem hiding this comment.
Add required CLAUDE.md sections (Summary/Used in/Navigation/Source Code Structure).
This file doesn’t include the mandatory sections required for every crate’s CLAUDE.md. Please add: Summary (2–5 bullets), Used in, Navigation instructions, and Source Code Structure (library-style: Core Types, etc.). As per coding guidelines, this is required for compliance and discoverability.
As per coding guidelines, ...
📌 Suggested structure
## Summary
- ...
- ...
## Used in
- ...
## Navigation
- ...
## Source Code Structure
src/
├── ...
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/CLAUDE.md` around lines 1 - 27, Add the
required CLAUDE.md sections: create a "Summary" section with 2–5 concise bullet
points describing the crate, a "Used in" section listing consumers (e.g., SDKs
or other crates that import this derive), a "Navigation" section with pointers
to key files/entry points (e.g., lib.rs, derive_impl.rs, attribute_impl.rs) and
usage notes, and a "Source Code Structure" section that lists the source tree in
a library-style breakdown (Core Types, Parsing, Builders, Utilities, Macros)
mirroring the existing src/ listing; ensure each section uses the exact headers
Summary, Used in, Navigation, and Source Code Structure so the file meets the
required template.
| pub fn account_names_code(&self, crate_ctx: Option<&CrateContext>) -> TokenStream2 { | ||
| match self.account_names_spec() { | ||
| Some(AccountNamesSpec::Inline(names)) => { | ||
| // Inline names - use directly | ||
| quote! { vec![#(#names.to_string()),*] } | ||
| } | ||
| Some(AccountNamesSpec::TypeRef(ty)) => { | ||
| // Type reference - extract struct name and lookup in CrateContext | ||
| let struct_name = extract_struct_name(&ty); | ||
| let variant_name = &self.ident; | ||
|
|
||
| let Some(ctx) = crate_ctx else { | ||
| eprintln!( | ||
| "warning: InstructionDecoder variant '{}': could not parse crate context, \ | ||
| account names for '{}' will be empty", | ||
| variant_name, struct_name | ||
| ); | ||
| return quote! { Vec::new() }; | ||
| }; | ||
|
|
||
| if let Some(field_names) = ctx.get_struct_field_names(&struct_name) { | ||
| // Found in crate - generate inline names | ||
| return quote! { vec![#(#field_names.to_string()),*] }; | ||
| } | ||
|
|
||
| // Struct not found - emit warning and fallback to empty vec | ||
| eprintln!( | ||
| "warning: InstructionDecoder variant '{}': struct '{}' not found in crate, \ | ||
| account names will be empty. Ensure the struct is defined in this crate.", | ||
| variant_name, struct_name | ||
| ); | ||
| quote! { Vec::new() } | ||
| } | ||
| None => quote! { Vec::new() }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using compile-time diagnostics instead of eprintln!.
Using eprintln! in proc-macros works but prints to stderr during compilation, which can be confusing for users—the warnings appear without source location context. For better developer experience, consider using proc_macro::Diagnostic (nightly) or the proc-macro-error crate for structured warnings.
That said, this is a minor ergonomics concern and the current approach functions correctly.
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/src/parsing.rs` around lines 180 - 215,
Replace the stderr prints in account_names_code (the two eprintln! calls inside
the AccountNamesSpec::TypeRef match arm) with compile-time diagnostics: produce
a proc-macro warning that includes the same message and the span of the
problematic items (use self.ident.span() or the span from the extracted type) so
users get source-location context; you can implement this either with
proc_macro::Diagnostic::spanned (nightly) or with proc-macro-error's
emit_warning!/register_attr_error macros, and keep the same fallback behavior
(returning Vec::new()) when emitting the diagnostic; references:
account_names_code, AccountNamesSpec::TypeRef, extract_struct_name, crate_ctx,
get_struct_field_names, variant_name, struct_name.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder-derive/CLAUDE.md`:
- Around line 37-41: The docs currently show an unsupported 8-byte discriminator
syntax using the form `#[discriminator(26, 16, ...)]`; update the documentation
examples in the "Two syntax forms for specifying explicit discriminators"
section so the array form matches the builder's expected syntax `#[discriminator
= [a, b, ...]]` (leave the integer form `#[discriminator = 5]` as-is) and change
any other occurrences of `#[discriminator(...)]` (including the later example
around lines 81-83) to the `#[discriminator = [ ... ]]` form so docs match the
compiler/builder error message.
In `@sdk-libs/instruction-decoder/src/types.rs`:
- Around line 99-114: The current
EnhancedLoggingConfig.decode_light_instructions check in Instruction::decode()
short-circuits all decoders; instead, remove the early return and only skip
applying decoders that are Light-specific. Change the logic in decode(&mut self,
config: &EnhancedLoggingConfig) so you always call
config.decoder_registry().and_then(|r| r.decode(...)) but after you get
Some((decoded, decoder)) check whether the found decoder is a Light protocol
decoder (e.g., via decoder.program_name() or a decoder.is_light() accessor) and
only apply the decoded result when either config.decode_light_instructions is
true or the decoder is not a Light decoder; update
EnhancedLoggingConfig.decode_light_instructions usage (or rename it to
decode_instructions if you prefer) so the flag semantics match this selective
gating.
♻️ Duplicate comments (13)
sdk-libs/instruction-decoder/Cargo.toml (1)
12-32: SDK‑libs dependency rule violation (sdk→sdk).Line 15‑32 pulls in
light-sdk-typesandlight-instruction-decoder-derive, which are sdk‑libs crates. The policy says sdk‑libs must depend only on program‑libs, light‑prover‑client, or external crates, so this breaks the layering rule. Please move shared types/macros into an allowed crate and remove these sdk‑libs deps from this manifest. As per coding guidelines, this dependency direction must be enforced.sdk-libs/instruction-decoder/src/config.rs (2)
32-35: Deserialization drops the decoder registry.Because
decoder_registryis skipped by serde, any deserializedEnhancedLoggingConfigends up withNone, silently disabling decoding. Add a serde default to restore a registry on load.🔧 Suggested fix (serde default)
+fn default_decoder_registry() -> Option<Arc<DecoderRegistry>> { + Some(Arc::new(DecoderRegistry::new())) +} + - #[serde(skip)] + #[serde(skip, default = "default_decoder_registry")] decoder_registry: Option<Arc<DecoderRegistry>>,
124-138:with_decoderscan drop previously registered decoders on clones.Line 124‑137 uses
Arc::get_mut, which fails if theArcis shared and forces a new empty registry, losing prior decoders. UseArc::make_mutfor clone‑on‑write semantics.🔧 Suggested fix (clone‑on‑write)
- if let Some(ref mut arc) = self.decoder_registry { - if let Some(registry) = Arc::get_mut(arc) { - registry.register_all(decoders); - return self; - } - } + if let Some(ref mut arc) = self.decoder_registry { + let registry = Arc::make_mut(arc); + registry.register_all(decoders); + return self; + }sdk-libs/instruction-decoder/src/types.rs (1)
28-43: Hardcodedcompute_totaldefault is brittle.Line 37 bakes in
1_400_000, which can drift from real limits. Consider making this configurable (constructor arg or constant) so callers can supply the appropriate value.💡 Example refactor (configurable default)
impl EnhancedTransactionLog { /// Create a new empty transaction log with basic info pub fn new(signature: Signature, slot: u64) -> Self { - Self { + Self::with_compute_total(signature, slot, 1_400_000) + } + + pub fn with_compute_total(signature: Signature, slot: u64, compute_total: u64) -> Self { + Self { signature, slot, status: TransactionStatus::Unknown, fee: 0, compute_used: 0, - compute_total: 1_400_000, + compute_total, instructions: Vec::new(), account_changes: Vec::new(), program_logs_pretty: String::new(), light_events: Vec::new(), } } }sdk-libs/instruction-decoder/src/programs/light_system.rs (1)
26-27: UsePubkeyconstant instead of string for System Program ID comparison.The string comparison at line 479 (
account.pubkey.to_string() == SYSTEM_PROGRAM_ID) allocates on every call. For a utility invoked during formatting, this is acceptable but suboptimal.♻️ More efficient approach
-/// System program ID string for account resolution -const SYSTEM_PROGRAM_ID: &str = "11111111111111111111111111111111"; +/// System program ID for account resolution +const SYSTEM_PROGRAM_ID: Pubkey = Pubkey::new_from_array([0u8; 32]);Then update the comparison at line 479:
-if account.pubkey.to_string() == SYSTEM_PROGRAM_ID { +if account.pubkey == SYSTEM_PROGRAM_ID {sdk-libs/program-test/src/logging/mod.rs (2)
266-278: Inner-instruction mapping may incorrectly assume dense indexing.At line 267, the code uses
enumerate()to iterate overinner_instructions_list, assuming the list index matches the top-level instruction index. However, Solana'sInnerInstructionsstruct contains an explicitindexfield because the list can be sparse—if instructions 0 and 2 have inner instructions but 1 doesn't, the list has 2 elements but they map to indices 0 and 2.🔧 Recommended fix using explicit index field
- for (instruction_index, inner_list) in inner_instructions_list.iter().enumerate() { - if let Some(instruction) = instructions.get_mut(instruction_index) { + for inner_list in inner_instructions_list.iter() { + let instruction_index = inner_list.index as usize; + if let Some(instruction) = instructions.get_mut(instruction_index) {Verify LiteSVM's
inner_instructionsstructure:#!/bin/bash # Check LiteSVM's inner_instructions type and whether it has an index field rg "inner_instructions" sdk-libs/program-test --type rust -B 3 -A 5 | head -60 # Check if litesvm exposes InnerInstructions with an index field rg "struct.*Inner.*Instruction" --type rust -A 5 | head -40
222-222: Add clarifying comment for fee estimation.The flat 5000 lamports per signature is the base transaction fee but doesn't include priority fees or compute unit pricing. For logging this is fine, but a brief comment would prevent confusion.
+ // Estimate base fee: 5000 lamports per signature (excludes priority fees) let estimated_fee = (transaction.signatures.len() as u64) * 5000;sdk-libs/instruction-decoder-derive/src/parsing.rs (1)
463-468: Document thecrate::IDfallback assumption.When
program_idisn't specified anddeclare_id!isn't found, this defaults tocrate::ID. While standard for Anchor programs, users with different crate structures may encounter confusing compile errors./// Get program ID source, defaulting to `crate::ID` if not specified. + /// + /// Note: The default assumes the crate exports `pub const ID: Pubkey` at the root, + /// which is standard for Anchor programs using `declare_id!`. pub fn program_id_source(&self) -> ProgramIdSource {sdk-libs/instruction-decoder-derive/src/builder.rs (2)
140-151: Guard against discriminator overflow in 1‑byte mode.Casting
u32/usizetou8silently truncates values ≥256, which can collide discriminators. Add a bounds check and emit a compile-time error instead.🔧 Suggested fix (bounds check before casting)
- let disc = match &self.explicit_discriminators[index] { - Some(ExplicitDiscriminator::U32(d)) => *d as u8, - Some(ExplicitDiscriminator::Array(_)) => { - return Err(syn::Error::new( - variant.ident.span(), - "array discriminator not supported for 1-byte discriminator size", - )); - } - None => index as u8, - }; + let disc_u32 = match &self.explicit_discriminators[index] { + Some(ExplicitDiscriminator::U32(d)) => *d, + Some(ExplicitDiscriminator::Array(_)) => { + return Err(syn::Error::new( + variant.ident.span(), + "array discriminator not supported for 1-byte discriminator size", + )); + } + None => index as u32, + }; + if disc_u32 > u8::MAX as u32 { + return Err(syn::Error::new( + variant.ident.span(), + "1-byte discriminator supports at most 256 variants/values", + )); + } + let disc = disc_u32 as u8;
526-531: Avoid underflow when formatting unknown field sizes.
remaining.len() - offsetcan underflow ifoffsetexceedsremaining.len(). Usesaturating_sub.🔧 Suggested fix
- format!("({}bytes)", remaining.len() - `#offset`), + format!("({}bytes)", remaining.len().saturating_sub(`#offset`)),sdk-libs/instruction-decoder-derive/src/attribute_impl.rs (1)
196-233: Consider cachingCrateContextto avoid repeated manifest scans.
CrateContext::parse_from_manifest()runs on every macro expansion; in crates with multiple#[instruction_decoder]modules, this adds avoidable IO. Consider a shared cache (e.g.,OnceLock) or only parsing when aContext<T>is present.sdk-libs/instruction-decoder-derive/CLAUDE.md (2)
1-27: Add required CLAUDE.md sections (Summary/Used in/Navigation/Source Code Structure).The current doc doesn’t include the mandatory sections required for crate-level CLAUDE.md files. Please add those sections with a concise module tree in “Source Code Structure.” As per coding guidelines, ...
📌 Suggested outline
## Summary - ... - ... ## Used in - ... ## Navigation - ... ## Source Code Structure src/ ├── ...
16-25: Add a language tag to the module tree code block.This fixes MD040 and improves rendering.
📝 Suggested fix
-``` +```text src/ ├── lib.rs # Macro entry points only (~100 lines) ...
| Two syntax forms for specifying explicit discriminators: | ||
|
|
||
| 1. **Integer**: `#[discriminator = 5]` - for 1-byte and 4-byte modes | ||
| 2. **Array**: `#[discriminator(26, 16, 169, 7, 21, 202, 242, 25)]` - for 8-byte mode with custom discriminators | ||
|
|
There was a problem hiding this comment.
Documentation uses unsupported 8‑byte discriminator syntax.
The builder error message expects #[discriminator = [a, b, ...]], but the docs show #[discriminator(…)]. Please align the docs with the actual syntax to prevent user-facing compile errors.
📝 Suggested fix
-2. **Array**: `#[discriminator(26, 16, 169, 7, 21, 202, 242, 25)]` - for 8-byte mode with custom discriminators
+2. **Array**: `#[discriminator = [26, 16, 169, 7, 21, 202, 242, 25]]` - for 8-byte mode with custom discriminatorsAlso applies to: 81-83
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/CLAUDE.md` around lines 37 - 41, The docs
currently show an unsupported 8-byte discriminator syntax using the form
`#[discriminator(26, 16, ...)]`; update the documentation examples in the "Two
syntax forms for specifying explicit discriminators" section so the array form
matches the builder's expected syntax `#[discriminator = [a, b, ...]]` (leave
the integer form `#[discriminator = 5]` as-is) and change any other occurrences
of `#[discriminator(...)]` (including the later example around lines 81-83) to
the `#[discriminator = [ ... ]]` form so docs match the compiler/builder error
message.
| /// Decode this instruction using the provided config's decoder registry | ||
| pub fn decode(&mut self, config: &EnhancedLoggingConfig) { | ||
| if !config.decode_light_instructions { | ||
| return; | ||
| } | ||
|
|
||
| // Try the decoder registry (includes custom decoders) | ||
| if let Some(registry) = config.decoder_registry() { | ||
| if let Some((decoded, decoder)) = | ||
| registry.decode(&self.program_id, &self.data, &self.accounts) | ||
| { | ||
| self.instruction_name = Some(decoded.name.clone()); | ||
| self.decoded_instruction = Some(decoded); | ||
| self.program_name = decoder.program_name().to_string(); | ||
| } | ||
| } |
There was a problem hiding this comment.
decode_light_instructions disables all decoding.
Line 100‑113 short‑circuits every decoder, not just Light‑specific ones, despite the field doc saying “Decode Light Protocol specific instructions.” Either rename the flag (e.g., decode_instructions) or gate only the Light decoders to match the docstring.
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder/src/types.rs` around lines 99 - 114, The current
EnhancedLoggingConfig.decode_light_instructions check in Instruction::decode()
short-circuits all decoders; instead, remove the early return and only skip
applying decoders that are Light-specific. Change the logic in decode(&mut self,
config: &EnhancedLoggingConfig) so you always call
config.decoder_registry().and_then(|r| r.decode(...)) but after you get
Some((decoded, decoder)) check whether the found decoder is a Light protocol
decoder (e.g., via decoder.program_name() or a decoder.is_light() accessor) and
only apply the decoded result when either config.decode_light_instructions is
true or the decoder is not a Light decoder; update
EnhancedLoggingConfig.decode_light_instructions usage (or rename it to
decode_instructions if you prefer) so the flag semantics match this selective
gating.
logging crate works chore: move decoder completely add spl add more programs add registry program chore: add account compression make macro work guard behind target os fmt fix: gate HasAccountNames impl with cfg(not(target_os = solana)) impl feedback refactor revert unrelated macros changes refactor: use CrateContext for account names resolution in InstructionDecoder derive - Extract struct field names at compile time via CrateContext parsing - Remove dependency on HasAccountNames trait for type references - Works with any struct with named fields (standard Anchor structs) - Emit compile-time warnings if struct resolution fails - Update CLAUDE.md documentation to reflect new approach fix formatter header, add more tests fix: anchor ix data deserialization impl better config and array truncation add ctoken pretty parsing migrate light system program logging to trait dedup remove legacy refactor formatter simplify add to more programs
ananas-block
force-pushed
the
jorrit/feat-instruction-decoder
branch
from
435d314 to
639eb72
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder/src/formatter.rs`:
- Around line 245-275: The table-row handling loop incorrectly uses a single
`modified` flag for the whole line so once a long value is handled the later
cells skip their fallback append and are dropped; inside the block that iterates
`for (i, part) in parts.iter().enumerate()` replace the global `modified` usage
with a per-cell flag (e.g., `cell_modified`) that is reset at the start of each
`part` iteration, use that to decide whether to push the replaced text or the
original `part`, and only set the outer `modified` (if still needed) when any
cell was changed; keep calls to `self.format_long_value_with_indent`, preserve
the `new_line.push('|')` behavior and ensure every `part` appends either the
modified or original content so no columns are lost.
- Around line 300-319: The wrapping branch currently resets current_line to
part.to_string(), dropping the leading separator; change it to use the computed
addition (which includes the ", " for non-first items) so separators are
preserved when starting a new line: in the loop over parts (variables parts,
addition, current_line, first_line, result, indent, max_width) replace
current_line = part.to_string() with current_line = addition (or otherwise
assign the same formatted string used for appending) so wrapped lines keep the
comma and space.
In `@sdk-libs/instruction-decoder/src/programs/ctoken.rs`:
- Around line 176-195: The CPI-context detection in format_mint_action currently
sets uses_cpi_context based on cpi_context.set_context ||
cpi_context.first_set_context, which misses execute mode where cpi_context is
Some but both flags are false; change uses_cpi_context to simply detect presence
of cpi_context (e.g., data.cpi_context.is_some()) so the resolve closure (and
any pubkey resolution using PACKED_ACCOUNTS_START) treats packed accounts from
the CPI context the same way format_transfer2 does, ensuring mint formatting
handles write and execute modes identically.
In `@sdk-libs/instruction-decoder/src/types.rs`:
- Around line 204-214: The get_program_name function currently hardcodes program
ID mappings which is brittle; update it to consult the decoder registry's
program_name() method (or make program name resolution a trait on decoders) so
new programs are discovered without editing this function; specifically, change
get_program_name to first call your decoder registry's program_name(program_id)
(or decoder::program_name()) and return that if Some, otherwise fall back to the
existing hardcoded map/Unknown Program(...) fallback to preserve behavior.
In `@sdk-libs/program-test/Cargo.toml`:
- Around line 55-56: The Cargo.toml change introduces an sdk‑libs → sdk‑libs
dependency: remove the dependency on light-instruction-decoder from
light-program-test and instead move the shared decoder types into program-libs
(or create an allowed shared crate, e.g., light-shared) so sdk‑libs do not
depend on one another; update the crate that consumed those types
(light-program-test) to import them from program-libs (or the new shared crate)
and adjust any use paths in the code referencing light-instruction-decoder
accordingly.
♻️ Duplicate comments (16)
sdk-libs/instruction-decoder-derive/CLAUDE.md (3)
1-27: Add required CLAUDE.md sections (Summary/Used in/Navigation/Source Code Structure).
This crate doc is missing the mandatory sections required for all CLAUDE.md files. Please add them to meet the standard template.As per coding guidelines, add the required Summary/Used in/Navigation/Source Code Structure sections.
16-25: Add a language specifier to the module tree code block.
Markdownlint flags the fenced block; add a language (e.g.,text) to avoid lint failures.📝 Suggested fix
-``` +```text src/ ├── lib.rs # Macro entry points only (~100 lines) ├── utils.rs # Case conversion, discriminator, error handling ├── parsing.rs # Darling-based attribute parsing structs ├── builder.rs # InstructionDecoderBuilder (code generation) ├── derive_impl.rs # #[derive(InstructionDecoder)] implementation ├── attribute_impl.rs # #[instruction_decoder] attribute implementation └── crate_context.rs # Recursive crate parsing for Accounts struct discovery
37-41: Fix discriminator syntax in docs to match the macro parser.
The docs show#[discriminator(...)], but the macro expects#[discriminator = [..]]for 8‑byte mode.📝 Suggested fix
-2. **Array**: `#[discriminator(26, 16, 169, 7, 21, 202, 242, 25)]` - for 8-byte mode with custom discriminators +2. **Array**: `#[discriminator = [26, 16, 169, 7, 21, 202, 242, 25]]` - for 8-byte mode with custom discriminators- #[discriminator(26, 16, 169, 7, 21, 202, 242, 25)] + #[discriminator = [26, 16, 169, 7, 21, 202, 242, 25]]Also applies to: 81-83
sdk-libs/instruction-decoder/Cargo.toml (1)
12-32: Remove sdk‑libs → sdk‑libs dependencies to comply with the architecture rule.
light-sdk-typesandlight-instruction-decoder-deriveare sdk‑libs crates; sdk‑libs should depend only on program‑libs, light‑prover‑client, or external crates.Based on learnings, move shared types/derives to a program‑libs crate and update imports accordingly.
sdk-libs/instruction-decoder-derive/src/utils.rs (1)
31-52: Fix snake_case conversion to match Anchor’s acronym handling.
The current splitter turnsHTTPHandlerintoh_t_t_p_handler, which changes Anchor discriminators for any acronym-heavy instruction name and breaks decoding. Useheck::ToSnakeCase(Anchor-compatible) and update tests/examples accordingly. Also add theheckdependency insdk-libs/instruction-decoder-derive/Cargo.toml.🔧 Suggested fix (Anchor-compatible snake_case)
-use proc_macro::TokenStream; +use proc_macro::TokenStream; +use heck::ToSnakeCase; @@ -/// assert_eq!(to_snake_case("HTTPHandler"), "h_t_t_p_handler"); +/// assert_eq!(to_snake_case("HTTPHandler"), "http_handler"); @@ pub(crate) fn to_snake_case(name: &str) -> String { - let mut result = String::new(); - for (i, c) in name.chars().enumerate() { - if c.is_uppercase() { - if i > 0 { - result.push('_'); - } - result.push(c.to_ascii_lowercase()); - } else { - result.push(c); - } - } - result + name.to_snake_case() } @@ - assert_eq!(to_snake_case("HTTPHandler"), "h_t_t_p_handler"); + assert_eq!(to_snake_case("HTTPHandler"), "http_handler");sdk-libs/instruction-decoder/src/config.rs (2)
32-35: Decoder registry is lost after deserialization.
#[serde(skip)]makesdecoder_registryNoneon load, so decoding silently stops unless callers reinitialize it. Considerskip_serializing+defaultto restore the registry.🔧 Suggested fix (serde default)
+fn default_decoder_registry() -> Option<Arc<DecoderRegistry>> { + Some(Arc::new(DecoderRegistry::new())) +} + - #[serde(skip)] + #[serde(skip_serializing, default = "default_decoder_registry")] decoder_registry: Option<Arc<DecoderRegistry>>,
124-133:with_decoderscan discard previously registered decoders.
Arc::get_mutfails when the config is cloned/shared; the fallback creates a fresh registry and drops existing decoders. UseArc::make_mutfor COW semantics so decoders are preserved.🔧 Suggested fix (clone‑on‑write)
- if let Some(ref mut arc) = self.decoder_registry { - if let Some(registry) = Arc::get_mut(arc) { - registry.register_all(decoders); - return self; - } - } + if let Some(ref mut arc) = self.decoder_registry { + let registry = Arc::make_mut(arc); + registry.register_all(decoders); + return self; + }sdk-libs/program-test/src/logging/mod.rs (1)
266-277: Inner‑instruction mapping should use the explicit index (not enumerate).
If LiteSVM follows Solana’sInnerInstructions { index, instructions }layout, enumerate() will mis‑attach sparse inner instruction lists.🔧 Suggested fix (index‑based mapping)
- for (instruction_index, inner_list) in inner_instructions_list.iter().enumerate() { - if let Some(instruction) = instructions.get_mut(instruction_index) { - instruction.inner_instructions = parse_inner_instructions( - inner_list, - &transaction.message.account_keys, - &transaction.message, - 1, - config, - ); - } - } + for inner_list in inner_instructions_list.iter() { + let instruction_index = inner_list.index as usize; + if let Some(instruction) = instructions.get_mut(instruction_index) { + instruction.inner_instructions = parse_inner_instructions( + &inner_list.instructions, + &transaction.message.account_keys, + &transaction.message, + 1, + config, + ); + } + }LiteSVM TransactionMeta inner_instructions type (does it include an explicit index field like solana-transaction-status InnerInstructions)?sdk-libs/instruction-decoder/src/programs/registry.rs (1)
164-174: Trim unused authority accounts for pause/unpause/deprecate decoders.
These state-only ops don’t take new authority accounts, so the currentaccount_nameslist is misleading. Please align it to the actually used accounts.🐛 Proposed fix
- #[instruction_decoder(account_names = ["update_authority", "compressible_config", "new_update_authority", "new_withdrawal_authority"])] + #[instruction_decoder(account_names = ["update_authority", "compressible_config"])] PauseCompressibleConfig, - #[instruction_decoder(account_names = ["update_authority", "compressible_config", "new_update_authority", "new_withdrawal_authority"])] + #[instruction_decoder(account_names = ["update_authority", "compressible_config"])] UnpauseCompressibleConfig, - #[instruction_decoder(account_names = ["update_authority", "compressible_config", "new_update_authority", "new_withdrawal_authority"])] + #[instruction_decoder(account_names = ["update_authority", "compressible_config"])] DeprecateCompressibleConfig,sdk-libs/instruction-decoder-derive/src/builder.rs (2)
526-535: Potential underflow when calculating unknown field size.The subtraction
remaining.len() -#offset`` on line 530 can underflow ifoffsetexceeds `remaining.len()`. This occurs when earlier field parsing consumes more bytes than expected or when instruction data is truncated.Use saturating subtraction to prevent panic:
🐛 Proposed fix
_ => ( quote! { fields.push(light_instruction_decoder::DecodedField::new( `#field_name`, - format!("({}bytes)", remaining.len() - `#offset`), + format!("({}bytes)", remaining.len().saturating_sub(`#offset`)), )); }, 0, ),
140-163: Silent truncation for 1-byte discriminator with large enum indices.When
discriminator_size = 1and an enum variant index exceeds 255, the cast at line 150 (index as u8) silently truncates, causing multiple variants to share the same discriminator byte.Consider adding validation in
InstructionDecoderArgs::validate()or here to ensure variant count doesn't exceed discriminator capacity:🐛 Suggested validation
// In generate_match_arms or validate(): let variant_count = self.args.variants().len(); if self.args.discriminator_size == 1 && variant_count > 256 { return Err(syn::Error::new( self.args.ident.span(), format!( "enum has {} variants but 1-byte discriminator supports at most 256", variant_count ), )); }sdk-libs/instruction-decoder-derive/src/parsing.rs (2)
196-216:eprintln!warnings lack source location context.The warnings at lines 197-201 and 211-215 print to stderr during compilation without source location information, making it difficult for users to identify which variant triggered the warning. This is a known limitation of proc-macro diagnostics.
The current approach works, but consider either:
- Using
proc-macro-errorcrate for structured warnings with spans- Documenting this behavior so users know to check compilation output
This is a minor ergonomics concern; the logic is correct.
463-468: Document thecrate::IDfallback assumption.When
program_idisn't explicitly specified anddeclare_id!isn't found, this defaults tocrate::ID. While standard for Anchor programs, users with different crate structures may encounter confusing errors.Consider adding a doc comment:
/// Get program ID source, defaulting to `crate::ID` if not specified. /// /// Note: The default assumes the crate exports `pub const ID: Pubkey` at the root, /// which is standard for Anchor programs using `declare_id!`. pub fn program_id_source(&self) -> ProgramIdSource {sdk-libs/instruction-decoder/src/types.rs (2)
99-115:decode_light_instructionsflag disables all decoding, not just Light-specific.The flag name suggests it controls Light Protocol-specific decoding, but lines 101-103 short-circuit the entire decode path, including generic Solana program decoders (System, SPL Token, etc.).
Either rename the flag to
decode_instructionsor modify the logic to only skip Light-specific decoders:♻️ Option 1: Rename the flag
// In config: -pub decode_light_instructions: bool, +pub decode_instructions: bool,♻️ Option 2: Selective gating
pub fn decode(&mut self, config: &EnhancedLoggingConfig) { - if !config.decode_light_instructions { - return; - } - if let Some(registry) = config.decoder_registry() { if let Some((decoded, decoder)) = registry.decode(&self.program_id, &self.data, &self.accounts) { + // Skip Light Protocol decoders if disabled + let is_light_decoder = matches!( + decoder.program_name(), + "Light System Program" | "Account Compression" | "Compressed Token Program" + ); + if is_light_decoder && !config.decode_light_instructions { + return; + } self.instruction_name = Some(decoded.name.clone()); self.decoded_instruction = Some(decoded); self.program_name = decoder.program_name().to_string(); } } }
28-43: Hardcodedcompute_totaldefault may not reflect current limits.The default value of
1_400_000at line 37 was the historical Solana compute budget. While reasonable, consider making this configurable for future-proofing:pub fn with_compute_total(signature: Signature, slot: u64, compute_total: u64) -> Self { Self { compute_total, ..Self::new(signature, slot) } }This is a minor enhancement; the current default works for most cases.
sdk-libs/instruction-decoder/src/lib.rs (1)
59-63: Fix SDK lib dependency violation: light-sdk-types should not be a default dependency.The types are correctly defined locally in
types.rs, butlight-sdk-typesis imported as a non-dev dependency in the defaultlight-protocolfeature and actively used informatter.rs(forconstants). This violates the coding guideline that SDK libs must depend only on program-libs, light-prover-client, and external crates. Sincelight-sdk-typesis itself an SDK library (not a program-lib), this dependency must either be removed or the functionality refactored to avoid the cross-SDK-lib dependency. Extract the needed constants locally if they're the only reason for the dependency.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder-derive/src/utils.rs`:
- Around line 95-104: pascal_to_display currently inserts spaces before every
uppercase char causing acronyms like "HTTPHandler" -> "H T T P Handler"; update
pascal_to_display to treat consecutive uppercase letters as a single acronym (or
use heck::ToTitleCase) by scanning runs of uppercase characters and only insert
a space before a run that is followed by a lowercase or before a new word
boundary, e.g., detect and append the whole uppercase run ("HTTP") as one token
then continue with the next char so "HTTPHandler" becomes "HTTP Handler"; modify
the loop in pascal_to_display (or replace with heck::ToTitleCase) to implement
this logic while preserving existing behavior for single-capital words.
In `@sdk-libs/instruction-decoder/src/formatter.rs`:
- Around line 362-363: The box header/footer currently use a hardcoded wide line
(the writeln! that prints "┌──... Transaction #{} ──...┐" using self.colors.gray
and self.colors.reset), which can overflow or wrap; modify the transaction box
rendering to compute a width dynamically (e.g., query terminal width with a
crate like terminal_size or derive width from the longest content such as
signature lines and tx_number) and then generate the border strings to match
that width (with minimum/maximum clamps to avoid too-small or absurdly large
boxes). Apply the same change to the corresponding footer code (the similar
writeln! near the footer) so header and footer widths stay in sync and adapt to
terminal/content size.
In `@sdk-libs/instruction-decoder/src/programs/light_system.rs`:
- Around line 600-624: The implementations of BorshDeserialize for InvokeWrapper
and InvokeCpiWrapper unconditionally skip 4 bytes which can mask malformed or
too-short inputs; update deserialize_reader for both InvokeWrapper and
InvokeCpiWrapper (the deserialize_reader methods) to validate the 4-byte Anchor
length prefix before proceeding: either require a BufRead (R: std::io::BufRead)
or wrap the provided reader in a std::io::BufReader, read the 4-byte prefix into
prefix, convert it to a u32 (little-endian) and check that the
buffered/available bytes are at least that length (or at minimum > 0) and return
an Err(std::io::Error::new(...)) if not; only then call
InstructionDataInvoke::deserialize_reader /
InstructionDataInvokeCpi::deserialize_reader and return the wrapper—this ensures
malformed or truncated inputs are rejected instead of producing garbage.
- Around line 466-505: The resolution in resolve_tree_and_queue_pubkeys relies
on the hardcoded tree_accounts_start = system_pos + 2 which is fragile; update
the function to validate and derive the base offset instead of assuming +2:
first ensure SYSTEM_PROGRAM_ID is present (otherwise return (None,None)), then
either (A) detect the first tree account by scanning accounts after system_pos
for an account whose owner/pubkey matches the expected merkle-tree program (use
that index as tree_accounts_start), or (B) change the function signature to
accept an explicit base_offset parameter derived from instruction decoding and
use that instead of +2; also add bounds checks for merkle_tree_index and
nullifier_queue_index and a brief doc comment describing the expected layout
when a base_offset is used.
♻️ Duplicate comments (15)
sdk-libs/instruction-decoder/Cargo.toml (1)
27-32: SDK-libs dependency violation (sdk-libs → sdk-libs).This crate depends on
light-instruction-decoder-deriveandlight-sdk-types, both of which reside insdk-libs/. Per the coding guidelines, SDK libs must depend only on program-libs, light-prover-client, and external crates.The derive macro and shared types should be relocated to
program-libs(following thelight-token-interfacepattern) so this crate can import them without creating sdk-to-sdk dependencies.sdk-libs/program-test/Cargo.toml (1)
56-56: SDK-libs should not depend on other sdk-libs crates.
light-program-test(sdk-libs) now depends onlight-instruction-decoder(sdk-libs), which breaks the sdk-libs dependency rule. Consider moving shared decoder types to program-libs or introducing an allowed shared crate.As per coding guidelines, avoid sdk-libs → sdk-libs dependencies.
sdk-libs/instruction-decoder-derive/CLAUDE.md (4)
16-25: Add language specifier to fenced code block.The module structure code block should have a language specifier for proper rendering and linting compliance.
📝 Suggested fix
-``` +```text src/ ├── lib.rs # Macro entry points only (~100 lines)
37-41: Discriminator array syntax in docs doesn't match actual macro syntax.The documentation shows
#[discriminator(26, 16, 169, 7, 21, 202, 242, 25)](parentheses with comma-separated values), but the macro builder expects#[discriminator = [26, 16, 169, 7, 21, 202, 242, 25]](equals sign with array brackets). Users following these docs will hit compile errors.📝 Suggested fix
Two syntax forms for specifying explicit discriminators: 1. **Integer**: `#[discriminator = 5]` - for 1-byte and 4-byte modes -2. **Array**: `#[discriminator(26, 16, 169, 7, 21, 202, 242, 25)]` - for 8-byte mode with custom discriminators +2. **Array**: `#[discriminator = [26, 16, 169, 7, 21, 202, 242, 25]]` - for 8-byte mode with custom discriminators
81-83: Same discriminator syntax issue in the example.This example also uses the incorrect parentheses form. Should use bracket array syntax.
📝 Suggested fix
// Explicit array discriminator (for 8-byte mode with custom discriminators) - #[discriminator(26, 16, 169, 7, 21, 202, 242, 25)] + #[discriminator = [26, 16, 169, 7, 21, 202, 242, 25]] #[instruction_decoder(account_names = ["fee_payer", "authority"])] CustomInstruction,
1-14: Missing required CLAUDE.md sections.Per coding guidelines, every crate's
CLAUDE.mdmust contain: Summary (2-5 bullet points), Used in, Navigation instructions, and Source Code Structure. The current file has "Overview" and "Module Structure" but doesn't follow the required template structure.The "Module Structure" section partially covers Source Code Structure, but the file is missing:
- Summary section with concise bullet points
- Used in section listing which crates/programs consume this derive macro
- Navigation section with pointers to key entry points
📌 Suggested additions at the top
# light-instruction-decoder-derive ## Summary - Procedural macro crate for generating `InstructionDecoder` trait implementations - Provides derive macro (`#[derive(InstructionDecoder)]`) for instruction enums - Provides attribute macro (`#[instruction_decoder]`) for Anchor program modules - Supports 1-byte, 4-byte, and 8-byte discriminator modes - All generated code is gated with `#[cfg(not(target_os = "solana"))]` ## Used in - `light-instruction-decoder` (re-exports the derive macro) - Program decoders: ComputeBudget, SPL Token, System, Token2022, etc. ## Navigation - Entry point: `src/lib.rs` (macro definitions) - Derive implementation: `src/derive_impl.rs` - Attribute implementation: `src/attribute_impl.rs` - Parsing: `src/parsing.rs` (Darling-based attribute extraction) ## Source Code Structure ...existing module structure...sdk-libs/instruction-decoder-derive/src/attribute_impl.rs (1)
196-233: CrateContext parsing on every macro invocation.
CrateContext::parse_from_manifest()parsesCargo.tomland scans source files each time the macro processes a module. This could impact compile times in crates with many#[instruction_decoder]-annotated modules. Consider caching or making account-name resolution optional when not needed.sdk-libs/instruction-decoder-derive/src/crate_context.rs (1)
306-318: Test only validates path construction, not resolution behavior.The test verifies string construction but doesn't exercise actual file resolution. Table-driven tests with temporary directories would catch resolution bugs like sibling vs. nested lookup. This aligns with the prior review suggestion.
sdk-libs/instruction-decoder/src/config.rs (2)
129-141:Arc::get_mutsilently drops decoders when Arc is shared.When
with_decodersis called on a config whosedecoder_registryArc has been cloned elsewhere,Arc::get_mutreturnsNone, and the code falls back to creating a new registry—discarding all previously registered custom decoders. The comment acknowledges this, butArc::make_mutwould provide clone-on-write semantics that preserve existing decoders.♻️ Suggested fix using clone-on-write
pub fn with_decoders(mut self, decoders: Vec<Box<dyn InstructionDecoder>>) -> Self { if let Some(ref mut arc) = self.decoder_registry { - if let Some(registry) = Arc::get_mut(arc) { - registry.register_all(decoders); - return self; - } + // Clone-on-write: preserves existing decoders even when Arc is shared + let registry = Arc::make_mut(arc); + registry.register_all(decoders); + return self; } // Create new registry if none exists let mut registry = DecoderRegistry::new(); registry.register_all(decoders); self.decoder_registry = Some(Arc::new(registry)); self }Note: This requires
DecoderRegistryto implementClone.
32-36: Deserialized configs lose the decoder registry.The
decoder_registryfield is marked#[serde(skip)], so deserializing anEnhancedLoggingConfigyieldsNonefor this field. Subsequent calls todecoder_registry()returnNone, and decoding silently fails unless callers explicitly callget_decoder_registry()first. Consider adding a serdedefaultto restore the registry on deserialization.♻️ Suggested fix with serde default
+fn default_decoder_registry() -> Option<Arc<DecoderRegistry>> { + Some(Arc::new(DecoderRegistry::new())) +} + - #[serde(skip)] + #[serde(skip_deserializing, skip_serializing, default = "default_decoder_registry")] decoder_registry: Option<Arc<DecoderRegistry>>,sdk-libs/instruction-decoder/src/programs/ctoken.rs (1)
176-195: Align CPI context detection with format_transfer2.The
format_mint_actionfunction checksctx.set_context || ctx.first_set_contextto detect CPI context mode, but this misses execute mode wherecpi_contextisSomewith both flags false. In execute mode, packed accounts are still stored in the CPI context account.For consistency with
format_transfer2(which usesdata.cpi_context.is_some()), simplify the detection:🔧 Proposed fix
- let uses_cpi_context = data - .cpi_context - .as_ref() - .map(|ctx| ctx.set_context || ctx.first_set_context) - .unwrap_or(false); + let uses_cpi_context = data.cpi_context.is_some();sdk-libs/program-test/src/logging/mod.rs (1)
222-223: Simplistic fee estimation may not reflect actual fees.The fee calculation
signatures.len() * 5000assumes a flat 5000 lamports per signature. For logging purposes this is acceptable, but a clarifying comment would help future maintainers understand this is an approximation.💡 Add clarifying comment
+ // Estimate base fee: 5000 lamports per signature (does not include priority fees) let estimated_fee = (transaction.signatures.len() as u64) * 5000;sdk-libs/instruction-decoder-derive/src/parsing.rs (2)
463-468: Document thecrate::IDfallback assumption more explicitly.When
program_idisn't specified anddeclare_id!isn't found, this defaults tocrate::ID. Users with non-standard crate structures will encounter confusing "cannot find valueID" errors without understanding why.📝 Suggested documentation improvement
- /// Get program ID source, defaulting to `crate::ID` if not specified. + /// Get program ID source, defaulting to `crate::ID` if not specified. + /// + /// **Note:** The default assumes the crate exports `pub const ID: Pubkey` at the root, + /// which is standard for Anchor programs using `declare_id!`. If your crate structure + /// differs, specify `program_id = "..."` explicitly. pub fn program_id_source(&self) -> ProgramIdSource {
185-220: Crate context lookup replaces problematic lifetime-based approach.The
account_names_codemethod now extracts struct names and looks them up viaCrateContext::get_struct_field_names, avoiding the previous lifetime assumption issues with<Type<'_>>::ACCOUNT_NAMES.The
eprintln!warnings work but lack source location context—users see warnings without knowing which file/line triggered them. For a proc-macro,proc_macro::Diagnostic(nightly) or theproc-macro-errorcrate would provide better DX. This is a minor ergonomics concern.sdk-libs/instruction-decoder/src/types.rs (1)
99-114: Thedecode_light_instructionsflag misleadingly disables all decoding.Despite the field being named
decode_light_instructions(suggesting it only affects Light Protocol instructions), the check at line 101 short-circuits the entire decode path for all decoders—including System, SPL Token, Compute Budget, etc.Either rename the flag to
decode_instructionsto reflect its actual behavior, or modify the logic to only skip Light-specific decoders while still allowing others.
| pub(crate) fn pascal_to_display(name: &str) -> String { | ||
| let mut result = String::new(); | ||
| for (i, c) in name.chars().enumerate() { | ||
| if i > 0 && c.is_uppercase() { | ||
| result.push(' '); | ||
| } | ||
| result.push(c); | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Acronym handling in pascal_to_display.
This function splits on every uppercase character, so "HTTPHandler" becomes "H T T P Handler" rather than "HTTP Handler". For display purposes this may be acceptable, but if human-readable output for acronyms matters, consider using heck::ToTitleCase or adding special handling for consecutive capitals.
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder-derive/src/utils.rs` around lines 95 - 104,
pascal_to_display currently inserts spaces before every uppercase char causing
acronyms like "HTTPHandler" -> "H T T P Handler"; update pascal_to_display to
treat consecutive uppercase letters as a single acronym (or use
heck::ToTitleCase) by scanning runs of uppercase characters and only insert a
space before a run that is followed by a lowercase or before a new word
boundary, e.g., detect and append the whole uppercase run ("HTTP") as one token
then continue with the next char so "HTTPHandler" becomes "HTTP Handler"; modify
the loop in pascal_to_display (or replace with heck::ToTitleCase) to implement
this logic while preserving existing behavior for single-capital words.
| writeln!(output, "{}┌──────────────────────────────────────────────────────────── Transaction #{} ─────────────────────────────────────────────────────────────┐{}", self.colors.gray, tx_number, self.colors.reset).expect("Failed to write box header"); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded box width limits flexibility for different terminal sizes.
The transaction box header and footer use a fixed character width of ~140 characters. If signatures or additional fields need more space (or users have narrower terminals), lines may wrap awkwardly.
Consider parameterizing the width or calculating it dynamically. This is a minor cosmetic concern that doesn't affect correctness.
Also applies to: 393-393
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder/src/formatter.rs` around lines 362 - 363, The
box header/footer currently use a hardcoded wide line (the writeln! that prints
"┌──... Transaction #{} ──...┐" using self.colors.gray and self.colors.reset),
which can overflow or wrap; modify the transaction box rendering to compute a
width dynamically (e.g., query terminal width with a crate like terminal_size or
derive width from the longest content such as signature lines and tx_number) and
then generate the border strings to match that width (with minimum/maximum
clamps to avoid too-small or absurdly large boxes). Apply the same change to the
corresponding footer code (the similar writeln! near the footer) so header and
footer widths stay in sync and adapt to terminal/content size.
| impl BorshDeserialize for InvokeWrapper { | ||
| fn deserialize_reader<R: std::io::Read>(reader: &mut R) -> std::io::Result<Self> { | ||
| // Skip 4-byte Anchor vec length prefix | ||
| let mut prefix = [0u8; 4]; | ||
| reader.read_exact(&mut prefix)?; | ||
| // Deserialize the actual data | ||
| let inner = InstructionDataInvoke::deserialize_reader(reader)?; | ||
| Ok(InvokeWrapper(inner)) | ||
| } | ||
| } | ||
|
|
||
| /// Wrapper type for InvokeCpi instruction that handles the 4-byte Anchor prefix. | ||
| #[derive(Debug)] | ||
| pub struct InvokeCpiWrapper(pub InstructionDataInvokeCpi); | ||
|
|
||
| impl BorshDeserialize for InvokeCpiWrapper { | ||
| fn deserialize_reader<R: std::io::Read>(reader: &mut R) -> std::io::Result<Self> { | ||
| // Skip 4-byte Anchor vec length prefix | ||
| let mut prefix = [0u8; 4]; | ||
| reader.read_exact(&mut prefix)?; | ||
| // Deserialize the actual data | ||
| let inner = InstructionDataInvokeCpi::deserialize_reader(reader)?; | ||
| Ok(InvokeCpiWrapper(inner)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Skipping the 4-byte prefix without validation could mask malformed data.
The BorshDeserialize implementations for InvokeWrapper and InvokeCpiWrapper unconditionally skip 4 bytes. If someone passes non-Anchor data (or data shorter than 4 bytes + the expected payload), the deserialization will either fail cryptically or produce garbage.
Consider validating the prefix bytes or at minimum checking that sufficient bytes remain. The docstrings mention the prefix but callers might not realize the fragility.
♻️ Suggested defensive check
impl BorshDeserialize for InvokeWrapper {
fn deserialize_reader<R: std::io::Read>(reader: &mut R) -> std::io::Result<Self> {
- // Skip 4-byte Anchor vec length prefix
let mut prefix = [0u8; 4];
reader.read_exact(&mut prefix)?;
+ // The 4 bytes are the Anchor vec length prefix - we don't validate the value
+ // but read_exact will fail if insufficient data is available
let inner = InstructionDataInvoke::deserialize_reader(reader)?;
Ok(InvokeWrapper(inner))
}
}🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder/src/programs/light_system.rs` around lines 600 -
624, The implementations of BorshDeserialize for InvokeWrapper and
InvokeCpiWrapper unconditionally skip 4 bytes which can mask malformed or
too-short inputs; update deserialize_reader for both InvokeWrapper and
InvokeCpiWrapper (the deserialize_reader methods) to validate the 4-byte Anchor
length prefix before proceeding: either require a BufRead (R: std::io::BufRead)
or wrap the provided reader in a std::io::BufReader, read the 4-byte prefix into
prefix, convert it to a u32 (little-endian) and check that the
buffered/available bytes are at least that length (or at minimum > 0) and return
an Err(std::io::Error::new(...)) if not; only then call
InstructionDataInvoke::deserialize_reader /
InstructionDataInvokeCpi::deserialize_reader and return the wrapper—this ensures
malformed or truncated inputs are rejected instead of producing garbage.
2 participants
Summary by CodeRabbit
New Features
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.