fix: instruction decoder account parsing and naming for light token a…#2201
Conversation
…nd system program
📝 WalkthroughWalkthroughAdds dynamic account-name resolvers for instruction variants and captures per-account pre/post state snapshots to enrich enhanced transaction logs and formatter output. Changes
Sequence Diagram(s)sequenceDiagram
actor ProgramTest
participant PreCtx as Pre-Execution<br/>LiteSVM Context
participant Executor as Transaction<br/>Executor
participant PostCtx as Post-Execution<br/>LiteSVM Context
participant Logger as Enhanced<br/>Logger
participant Formatter as Transaction<br/>Formatter
ProgramTest->>PreCtx: Snapshot account states
ProgramTest->>Executor: Execute transaction
Executor->>Executor: Modify accounts (lamports, data)
Executor->>PostCtx: Capture final state
ProgramTest->>Logger: log_transaction_enhanced(pre, post)
Logger->>Logger: Build AccountStateSnapshot map (deltas per account)
Logger->>Logger: Populate EnhancedTransactionLog.account_states
Logger->>Formatter: Format instructions with account_states
Formatter->>Formatter: Render 7-column outer account table
Formatter-->>ProgramTest: Enhanced transaction view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 8
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/programs/light_system.rs (1)
873-879: Address naming inconsistency between instruction decoder and SDK implementations.The instruction-decoder now uses
log_programin the account_names array, but the broader SDK and program implementations still extensively referencenoop_program(106 occurrences across SDK libs, tests, and programs). This creates a naming mismatch where the decoded account name (log_program) differs from the actual account field names (noop_program) used throughout the system.Either propagate the
log_programnaming consistently across sdk-libs and related instructions, or revert the decoder to usenoop_programfor alignment with the rest of the codebase. Documentation (e.g.,programs/compressed-token/program/docs/) also describes this account as "Noop program for event emission," which aligns with thenoop_programnaming convention.
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder-derive/src/builder.rs`:
- Around line 273-294: The generated code passes an empty string as the
DecodedField name (in the fields_code block for both the custom formatter path
and the Debug path) which reduces readability; update both branches that call
light_instruction_decoder::DecodedField::new("", ...) to supply a meaningful
name (e.g., "params" or derive a name from the params identifier/type available
in the macro input), using the same identifier used elsewhere (params) or a
value pulled from variant_args/variant metadata so the field label matches
attribute_impl.rs changes; ensure both the branch where `#formatter_path`(¶ms,
accounts) is used and the branch that uses format!("{:#?}", params) pass the
chosen non-empty name.
In `@sdk-libs/instruction-decoder/src/formatter.rs`:
- Around line 671-776: The duplicated account-name resolution in formatter.rs
(used in the outer_rows loop and the account_rows loop when building tables)
should be extracted into a helper method (e.g., resolve_account_name) on the
same impl so both branches call it; implement a method like fn
resolve_account_name(&self, instruction: &EnhancedInstructionLog, idx: usize,
pubkey: &Pubkey) -> String that performs the decoded_instruction lookup, filters
out empty names, and falls back to self.get_account_name(pubkey), then replace
the inline resolution logic in both places (the loops that build OuterAccountRow
and AccountRow) with calls to this helper.
- Around line 697-714: The subtraction casting lamports_after and
lamports_before to i64 (in the block that computes change for
states.get(&account.pubkey)) can overflow; change the computation to use a
wider/safe integer or saturating/checked arithmetic (e.g., promote to i128
before subtracting or use checked_sub/saturating_sub) and then convert/clamp the
result to the signed type expected by format_signed_with_thousands_separator,
ensuring you preserve the existing formatting logic (the change_str branches and
the use of format_signed_with_thousands_separator remain the same).
In `@sdk-libs/instruction-decoder/src/programs/ctoken.rs`:
- Around line 208-521: The resolve_transfer2_account_names function is large;
extract the packed-role construction into a helper to simplify it. Create a new
helper fn build_transfer2_packed_roles(data:
&CompressedTokenInstructionDataTransfer2) -> HashMap<u8, String> that contains
the packed_roles HashMap and all counters (owner_count, mint_count,
delegate_count, in_merkle_count, in_queue_count, compress_mint_count,
compress_source_count, compress_auth_count) and the logic that fills
packed_roles from data.output_queue, data.in_token_data, data.out_token_data,
and data.compressions; return the populated HashMap. In
resolve_transfer2_account_names replace the inline packed-role setup with a call
to build_transfer2_packed_roles(data) and remove the moved local variables,
keeping the subsequent loop that iterates packed_idx and uses packed_roles,
known_pubkeys, names, and idx unchanged. Ensure the helper is cfg-gated the same
way and preserves types and semantics.
- Around line 667-697: The calculate_mint_action_packed_accounts_start function
currently returns fixed offsets and misses accounts that precede
LightSystemAccounts; update it to compute the start dynamically by mirroring
resolve_mint_action_account_names: begin with 1 for light_system_program, add 1
if data.create_mint is true for mint_signer, add 1 for authority, then add 1
each for optional compressible_config, cmint, and rent_sponsor (presentness
determined from data), then add 6 for the LightSystemAccounts and finally +1 if
data.cpi_context.is_some() (unless cpi_context_write_mode is true, in which case
keep returning 3); use these fields (create_mint, compressible_config, cmint,
rent_sponsor, cpi_context) and the function
calculate_mint_action_packed_accounts_start and
resolve_mint_action_account_names to locate and implement the corrected
computation.
In `@sdk-libs/instruction-decoder/src/programs/light_system.rs`:
- Around line 668-742: The two functions
resolve_invoke_cpi_account_info_account_names and
resolve_invoke_cpi_readonly_account_names duplicate the same account-name
resolution logic; extract a shared helper (e.g.,
resolve_light_system_account_names_internal) that accepts
cpi_context_write_mode: bool and accounts: &[AccountMeta] and returns
Vec<String>, move the common indexing/naming logic there (including the
fixed-name sequence and the 3-account CPI-write path) and have both resolver
functions compute their cpi_context_write_mode from their respective
data.cpi_context fields and call the helper, removing the duplicated add_name
closure and repeated code.
- Around line 549-623: The function resolve_invoke_cpi_readonly_account_names
contains dead scaffolding: remove the unused HashMap known_pubkeys and all
references to it; update the add_name closure signature to drop the known: &mut
HashMap parameter and remove the known.insert(...) call inside the closure, and
update all calls to add_name to stop passing &mut known_pubkeys so the function
only computes and returns names as before; also remove the unused
std::collections::HashMap import.
In `@sdk-libs/program-test/src/logging/mod.rs`:
- Around line 249-257: The current mapping of pre_account/post_account to
(lamports, data_len) loses whether the account existed; update the snapshot
representation used in logging (e.g., replace the tuple mapping with
Option<AccountStateSnapshot> or extend AccountStateSnapshot with an exists:
bool) so pre_account.as_ref() and post_account.as_ref() produce Either
Some(AccountStateSnapshot { exists: true, lamports, data_len }) or None for
non‑existent accounts; adjust the code paths that read
lamports_before/data_len_before and lamports_after/data_len_after to handle the
Option<AccountStateSnapshot> (or the new exists field) when formatting logs so
"account doesn't exist" is distinct from "0 lamports, 0 data".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-libs/instruction-decoder/src/formatter.rs (1)
200-231: Clarify pre/post state in the outer account table.Line 711-713 formats
*_beforevalues, but the headers in Line 224-227 read as generic “Data Len/Lamports,” which can look like current state (e.g., newly created accounts appear as zero). Either display the*_aftervalues or make the headers explicit. A minimal label fix:✏️ Suggested label clarification
- #[tabled(rename = "Data Len")] + #[tabled(rename = "Data Len (pre)")] ... - #[tabled(rename = "Lamports")] + #[tabled(rename = "Lamports (pre)")]Also applies to: 671-739
🤖 Fix all issues with AI agents
In `@sdk-libs/program-test/src/logging/mod.rs`:
- Around line 237-242: The Rust doc comment for EnhancedTransactionLog
references a non-existent helper; update the doc so it points to the real helper
function name. Locate the comment block around EnhancedTransactionLog and
replace the incorrect function reference with the correct helper name
capture_pre_account_states in the sentence that currently reads "Use `...`
before and after sending the transaction." to ensure the doc links to the actual
snapshot helper.
♻️ Duplicate comments (1)
sdk-libs/program-test/src/logging/mod.rs (1)
243-283: Account existence ambiguity remains in snapshots.
Line 266 defaults missing accounts to(0, 0), which is indistinguishable from a real zero‑lamport account. This was already flagged in a prior review (suggested:Option<AccountStateSnapshot>or anexistsflag).Also applies to: 352-352
| /// Create EnhancedTransactionLog from LiteSVM transaction result | ||
| /// | ||
| /// If pre_states and post_states are provided, captures account state snapshots | ||
| /// for all accounts in the transaction. | ||
| /// | ||
| /// Use `capture_pre_account_states` before and after sending the transaction. |
There was a problem hiding this comment.
Fix doc reference to the snapshot helper.
Line 242 references a function name that doesn’t exist, which can mislead readers.
📝 Suggested fix
-/// Use `capture_pre_account_states` before and after sending the transaction.
+/// Use `capture_account_states` before and after sending the transaction.🤖 Prompt for AI Agents
In `@sdk-libs/program-test/src/logging/mod.rs` around lines 237 - 242, The Rust
doc comment for EnhancedTransactionLog references a non-existent helper; update
the doc so it points to the real helper function name. Locate the comment block
around EnhancedTransactionLog and replace the incorrect function reference with
the correct helper name capture_pre_account_states in the sentence that
currently reads "Use `...` before and after sending the transaction." to ensure
the doc links to the actual snapshot helper.
There was a problem hiding this comment.
Actionable comments posted: 3
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/program_test/rpc.rs (1)
562-615: Consider applying consistent logging patterns across transaction methods.
_create_and_send_transaction_with_eventuses basicmaybe_print_logs(), while_send_transaction_with_batched_eventcaptures pre/post account states and uses enhanced logging. Since both methods are test utilities that execute transactions and maintain context snapshots, applying the same logging approach would improve debugging consistency.If you decide to add enhanced logging here, follow the pattern in
_send_transaction_with_batched_event: capture account states viacrate::logging::capture_account_states()before and after execution, then callcrate::logging::log_transaction_enhanced()with the captured states. Alternatively, if this method is intentionally kept lightweight for performance reasons, a brief comment explaining the design choice would help future maintainers understand the trade-off.sdk-libs/instruction-decoder/src/formatter.rs (1)
1172-1202: Remove SDK lib interdependency or relocate shared types.The
get_account_namefunction depends onlight_sdk_types::constants(line 1174), but the architectural guideline is clear: SDK libs must depend only onprogram-libs,light-prover-client, and external crates. Sincelight-sdk-typesis itself an SDK lib, this violates that constraint.Per the protocol architecture, shared type definitions and constants should reside in
program-libs(seelight-token-interfacepattern) so SDKs and clients can import them without pulling in SDK-specific dependencies. Consider either:
- Moving the protocol constants to a
program-libslocation, or- Refactoring to access constants through a re-exported program-libs dependency instead of directly from light-sdk-types
🤖 Fix all issues with AI agents
In `@sdk-libs/instruction-decoder/src/formatter.rs`:
- Around line 1238-1247: Add a unit test covering the i64::MIN edge case to
document that format_signed_with_thousands_separator (which relies on
unsigned_abs()) handles the minimum i64 correctly: create a new test function
(e.g., test_format_signed_edge_cases) that asserts
format_signed_with_thousands_separator(i64::MIN) == "-9,223,372,036,854,775,808"
so the behavior is explicitly verified.
- Around line 676-685: Extract the duplicated AccountAccess determination into a
single helper and replace both inline blocks with calls to it: add a method like
AccountAccess::from_flags(is_signer: bool, is_writable: bool) (or a free fn
account_access_from_flags) that returns the correct variant (SignerWritable,
Signer, Writable, Readonly), then update the loops over instruction.accounts
(the block that currently builds `access` in formatter.rs and the similar block
at the other location) to call this helper instead of duplicating the if/else
logic.
In `@sdk-libs/program-test/src/logging/mod.rs`:
- Around line 237-243: The doc comment on from_transaction_result incorrectly
references capture_pre_account_states; update the comment to reference the
actual helper name capture_account_states (or any other correct exported
function name) and ensure phrasing matches the API (e.g., "Use
`capture_account_states` before and after sending the transaction."). Locate the
comment above the pub fn from_transaction_result and replace the incorrect
symbol so docs and examples match the real function name.
♻️ Duplicate comments (1)
sdk-libs/instruction-decoder/src/formatter.rs (1)
687-694: Consider extracting duplicate account resolution logic.The account name resolution pattern appears twice (lines 687-694 for outer rows and 756-761 for inner rows). A helper method would centralize this logic:
fn resolve_account_name( &self, instruction: &EnhancedInstructionLog, idx: usize, pubkey: &Pubkey, ) -> String { instruction .decoded_instruction .as_ref() .and_then(|decoded| decoded.account_names.get(idx).cloned()) .filter(|name| !name.is_empty()) .unwrap_or_else(|| self.get_account_name(pubkey)) }Also applies to: 756-761
| for (idx, account) in instruction.accounts.iter().enumerate() { | ||
| let access = if account.is_signer && account.is_writable { | ||
| AccountAccess::SignerWritable | ||
| } else if account.is_signer { | ||
| AccountAccess::Signer | ||
| } else if account.is_writable { | ||
| AccountAccess::Writable | ||
| } else { | ||
| AccountAccess::Readonly | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Duplicated access type determination.
The AccountAccess determination logic (lines 676-685 and 744-752) is duplicated. While simple, you could extract this to a helper or use a method on AccountAccess like from_flags(is_signer, is_writable) if this pattern appears elsewhere in the codebase.
🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder/src/formatter.rs` around lines 676 - 685,
Extract the duplicated AccountAccess determination into a single helper and
replace both inline blocks with calls to it: add a method like
AccountAccess::from_flags(is_signer: bool, is_writable: bool) (or a free fn
account_access_from_flags) that returns the correct variant (SignerWritable,
Signer, Writable, Readonly), then update the loops over instruction.accounts
(the block that currently builds `access` in formatter.rs and the similar block
at the other location) to call this helper instead of duplicating the if/else
logic.
| #[test] | ||
| fn test_format_signed_with_thousands_separator() { | ||
| assert_eq!(format_signed_with_thousands_separator(0), "0"); | ||
| assert_eq!(format_signed_with_thousands_separator(1234), "1,234"); | ||
| assert_eq!(format_signed_with_thousands_separator(-1234), "-1,234"); | ||
| assert_eq!( | ||
| format_signed_with_thousands_separator(-1000000), | ||
| "-1,000,000" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding edge case tests for signed formatter.
The current tests are good, but you might want to add coverage for i64::MIN to ensure unsigned_abs() handles it correctly (it does, but a test documents this guarantee):
💡 Additional test case
#[test]
fn test_format_signed_edge_cases() {
// i64::MIN is handled correctly by unsigned_abs()
assert_eq!(
format_signed_with_thousands_separator(i64::MIN),
"-9,223,372,036,854,775,808"
);
}🤖 Prompt for AI Agents
In `@sdk-libs/instruction-decoder/src/formatter.rs` around lines 1238 - 1247, Add
a unit test covering the i64::MIN edge case to document that
format_signed_with_thousands_separator (which relies on unsigned_abs()) handles
the minimum i64 correctly: create a new test function (e.g.,
test_format_signed_edge_cases) that asserts
format_signed_with_thousands_separator(i64::MIN) == "-9,223,372,036,854,775,808"
so the behavior is explicitly verified.
ananas-block
deleted the
jorrit/fix-instruction-decoder-account-parsing
branch
2 participants
…nd system program
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.