test: random unit tests light program compress decompress#2303
Open
ananas-block wants to merge 4 commits intomainfrom
Open
test: random unit tests light program compress decompress#2303ananas-block wants to merge 4 commits intomainfrom
ananas-block wants to merge 4 commits intomainfrom
Conversation
Entire-Checkpoint: 90f73237f4da
Entire-Checkpoint: 34ad2a47e6b5
Contributor
📝 WalkthroughWalkthroughThis PR introduces a builder pattern for CPI (Cross-Program Invocation) data construction in the compression/decompression processor modules. New builder functions validate accounts and construct CPI data structures, which are then invoked by updated processor functions with idempotent behavior. Development dependencies are added to support testing. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Builder as build_compress_pda_cpi_data()
participant Processor as process_compress_pda_accounts_idempotent()
participant CPI as CPI Invocation
participant PdaClose as Close PDAs
Caller->>Builder: remaining_accounts, params, dispatch_fn
Builder->>Builder: Validate system_accounts_offset
Builder->>Builder: Validate config & rent_sponsor
Builder->>Builder: Dispatch for each PDA account
Builder->>Builder: Collect cpi_ix_data, cpi_accounts, pda_indices
Builder-->>Caller: Ok(Some(CompressPdaBuilt)) or Ok(None)
alt CompressPdaBuilt Present
Caller->>Processor: built_data, cpi_signer, program_id
Processor->>CPI: Invoke with cpi_accounts
CPI-->>Processor: Success
Processor->>PdaClose: Close PDAs at indices
Processor-->>Caller: Ok(())
else No CompressPdaBuilt (Non-compressible)
Caller-->>Caller: Skip CPI invocation
end
sequenceDiagram
participant Caller
participant PdaBuilder as build_decompress_pda_cpi_data()
participant AcctBuilder as build_decompress_accounts_cpi_data()
participant Processor as process_decompress_accounts_idempotent()
participant PdaCPI as PDA CPI
participant TokenCPI as Token CPI
Caller->>AcctBuilder: remaining_accounts, params, variant_fn
AcctBuilder->>AcctBuilder: Validate system_accounts_offset
AcctBuilder->>AcctBuilder: Split PDA vs Token accounts
AcctBuilder->>AcctBuilder: Build cpi_accounts, gather compressed_account_infos
AcctBuilder->>AcctBuilder: Collect token_data, tlv, token_seeds
AcctBuilder-->>Caller: Ok(DecompressAccountsBuilt)
Caller->>Processor: built_accounts_data, dispatch_fn, cpi_signer
Processor->>PdaBuilder: Validate PDA path exists
alt PDAs to Decompress
PdaBuilder-->>Processor: Ok(Some(DecompressPdaBuilt))
Processor->>PdaCPI: Invoke PDA decompression
PdaCPI-->>Processor: Success
else No PDAs (Idempotent)
PdaBuilder-->>Processor: Ok(None)
end
Processor-->>Caller: Ok(())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
Author
|
@CodeRabbit pls review despite the test keyword |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/sdk-types/src/interface/program/decompression/processor.rs (2)
136-246:⚠️ Potential issue | 🟠 MajorBuilder pattern for PDA decompression mirrors compression — same
system_accounts_offsetbounds gap applies.The
build_decompress_pda_cpi_datafunction has the same potential slice panic as the compression processor:system_accounts_offsetis validated againstremaining_accounts.len()(line 156), but not againsthot_accounts_start(line 178). At line 190,remaining_accounts[system_accounts_offset..hot_accounts_start]will panic ifsystem_accounts_offset > hot_accounts_start.Same fix applies: add a guard
system_accounts_offset <= hot_accounts_startafter computinghot_accounts_start.Proposed fix
let hot_accounts_start = remaining_accounts .len() .checked_sub(num_hot_accounts) .ok_or(LightSdkTypesError::NotEnoughAccountKeys)?; + if system_accounts_offset > hot_accounts_start { + return Err(LightSdkTypesError::InvalidInstructionData); + } let hot_account_infos = &remaining_accounts[hot_accounts_start..];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/sdk-types/src/interface/program/decompression/processor.rs` around lines 136 - 246, The function build_decompress_pda_cpi_data can still panic when creating cpi_accounts because system_accounts_offset is only validated against remaining_accounts.len() but not against hot_accounts_start; after computing hot_accounts_start (the start of hot_account_infos) add a guard verifying system_accounts_offset <= hot_accounts_start and return an appropriate error (e.g., LightSdkTypesError::InvalidInstructionData or NotEnoughAccountKeys) if it fails, before calling CpiAccounts::new and slicing remaining_accounts[system_accounts_offset..hot_accounts_start].
308-434:⚠️ Potential issue | 🟠 MajorSame bounds gap in
build_decompress_accounts_cpi_dataat line 359.The
remaining_accounts[system_accounts_offset..hot_accounts_start]slice at line 359 can panic ifsystem_accounts_offset > hot_accounts_start, same root cause. Add the same guard here.Proposed fix
let hot_accounts_start = remaining_accounts .len() .checked_sub(num_hot_accounts) .ok_or(LightSdkTypesError::NotEnoughAccountKeys)?; + if system_accounts_offset > hot_accounts_start { + return Err(LightSdkTypesError::InvalidInstructionData); + } let hot_account_infos = &remaining_accounts[hot_accounts_start..];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/sdk-types/src/interface/program/decompression/processor.rs` around lines 308 - 434, The slice remaining_accounts[system_accounts_offset..hot_accounts_start] in build_decompress_accounts_cpi_data can panic when system_accounts_offset > hot_accounts_start; add a bounds check after computing hot_accounts_start (e.g. if system_accounts_offset > hot_accounts_start { return Err(LightSdkTypesError::NotEnoughAccountKeys); }) and use that validated system_accounts_offset when constructing CpiAccounts::new_with_config to prevent the panic; reference symbols: build_decompress_accounts_cpi_data, system_accounts_offset, hot_accounts_start, CpiAccounts::new_with_config, LightSdkTypesError::NotEnoughAccountKeys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk-libs/sdk-types/src/interface/program/compression/processor.rs`:
- Around line 84-86: The current bounds check only ensures
system_accounts_offset <= remaining_accounts.len() but can still allow
system_accounts_offset > pda_start causing a panic when slicing; after computing
pda_start in the compression processor (the variable named pda_start used for
slicing remaining_accounts[system_accounts_offset..pda_start]), add a validation
that returns Err(LightSdkTypesError::InvalidInstructionData) if
system_accounts_offset > pda_start; likewise, in the decompression processor
after computing hot_accounts_start (the variable used for slicing with
system_accounts_offset), add the same check to prevent system_accounts_offset >
hot_accounts_start and return the same error.
In `@sdk-libs/sdk-types/tests/decompress_accounts_processor.rs`:
- Around line 618-763: The test_randomized_pda_and_token_decompression currently
runs a single random configuration via rand::thread_rng(), which reduces
coverage and makes failures hard to reproduce; update the test to (1) loop
multiple iterations (e.g., for _ in 0..N) around the existing body so it
exercises many random combos, (2) switch to a reproducible RNG by creating a
SeedableRng (e.g., StdRng::from_seed(seed)) and log or print the seed before
each run so failing cases are reproducible, and (3) include an explicit sub-case
to exercise n_pdas==0 && n_tokens==0 (or remove the clamp) to ensure that edge
path is tested; locate and modify the setup using rng, n_pdas, n_tokens, and the
test function name to implement these changes.
In `@sdk-libs/sdk-types/tests/decompress_processor.rs`:
- Around line 70-91: The helper make_valid_accounts (and related test-only types
like SkipVariant) are duplicated across decompress_processor.rs,
decompress_accounts_processor.rs, and compress_processor.rs; move these shared
helpers into common/mod.rs next to make_config_account and make_dummy_account,
export them (pub) from common, then update the three test files to remove the
duplicated definitions and import make_valid_accounts and SkipVariant from
common:: so tests reuse the single implementation.
---
Outside diff comments:
In `@sdk-libs/sdk-types/src/interface/program/decompression/processor.rs`:
- Around line 136-246: The function build_decompress_pda_cpi_data can still
panic when creating cpi_accounts because system_accounts_offset is only
validated against remaining_accounts.len() but not against hot_accounts_start;
after computing hot_accounts_start (the start of hot_account_infos) add a guard
verifying system_accounts_offset <= hot_accounts_start and return an appropriate
error (e.g., LightSdkTypesError::InvalidInstructionData or NotEnoughAccountKeys)
if it fails, before calling CpiAccounts::new and slicing
remaining_accounts[system_accounts_offset..hot_accounts_start].
- Around line 308-434: The slice
remaining_accounts[system_accounts_offset..hot_accounts_start] in
build_decompress_accounts_cpi_data can panic when system_accounts_offset >
hot_accounts_start; add a bounds check after computing hot_accounts_start (e.g.
if system_accounts_offset > hot_accounts_start { return
Err(LightSdkTypesError::NotEnoughAccountKeys); }) and use that validated
system_accounts_offset when constructing CpiAccounts::new_with_config to prevent
the panic; reference symbols: build_decompress_accounts_cpi_data,
system_accounts_offset, hot_accounts_start, CpiAccounts::new_with_config,
LightSdkTypesError::NotEnoughAccountKeys.
Entire-Checkpoint: 7c9c24f71fe9
ananas-block
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Refactor
Tests
Chores