chore: remove vec length bytes from new instructions#1802
chore: remove vec length bytes from new instructions#1802ananas-block merged 12 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe updates adjust instruction deserialization offsets, discriminator lengths, and function signatures for typed instruction data across several Solana-related Rust programs. Manual instruction construction and invocation replace higher-level CPI abstractions in one test program. Serialization steps are streamlined and conditionalized, while feature flags are enabled for certain tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProgramA
participant ProgramB
Caller->>ProgramA: Call invoke_cpi_with_read_only(ctx, typed_inputs)
ProgramA->>ProgramB: Construct Instruction with 8-byte discriminator + typed data
ProgramA->>ProgramB: Invoke instruction (possibly with signer seeds)
ProgramB-->>ProgramA: Process result
ProgramA-->>Caller: Return result
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
program-libs/compressed-account/src/indexer_event/parse.rs (1)
288-315: 🛠️ Refactor suggestionOffset-handling now mixed between variants – risk of silent deserialization bugs
After switching to
let instruction = instruction.split_at(8).1;you correctly drop the 8-byte discriminator, but the later branches treat the remaining buffer inconsistently:
DISCRIMINATOR_INVOKE/DISCRIMINATOR_INVOKE_CPIskip an additional 4 bytes (instruction[4..]).DISCRIMINATOR_INVOKE_CPI_WITH_READ_ONLY/INVOKE_CPI_WITH_ACCOUNT_INFO_INSTRUCTIONpass the buffer unshifted.That implicit “4-byte length prefix” assumption is now replicated three times and can easily drift again. Consider:
- let instruction = instruction.split_at(8).1; + const LEN_PREFIX_BYTES: usize = 4; + let payload = &instruction[8..];and then slice explicitly with
&payload[LEN_PREFIX_BYTES..]in the affected branches (or zero in the others). Even better: introduce a small helper (fn payload_after_len(buf: &[u8]) -> Result<&[u8], ParseError>) to centralise the rule.This removes magic numbers, hard-to-spot divergence, and future-proofs the parser.
🧹 Nitpick comments (2)
program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (1)
3022-3025: Shorten instruction discriminators to 8 bytes and fix typo.You’ve updated both discriminator arrays to 8 bytes to align with the new deserialization offset.
- Confirm that there are no remaining 12-byte discriminators elsewhere in the codebase.
- Correct the inline comment typo from “INSTRUCTIOM” to “INSTRUCTION.”
program-tests/create-address-test-program/src/lib.rs (1)
152-157: Minor: drop secondanchor_lang::solana_program::log::sol_log_compute_units()You log compute units both before and after instruction creation; the second call (immediately before the invoke) is still useful, but the first call (line 121) already prints the pre-CPI value. If both are required, consider naming them (
"before_cpi"/"after_cpi") to ease log parsing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
anchor-programs/system/src/lib.rs(2 hunks)program-libs/compressed-account/src/indexer_event/parse.rs(2 hunks)program-tests/create-address-test-program/src/lib.rs(1 hunks)program-tests/sdk-pinocchio-test/tests/test.rs(1 hunks)program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs(1 hunks)programs/compressed-token/src/process_transfer.rs(1 hunks)programs/system/src/lib.rs(0 hunks)
💤 Files with no reviewable changes (1)
- programs/system/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
programs/compressed-token/src/process_transfer.rs (3)
anchor-programs/system/src/lib.rs (1)
invoke_cpi_with_read_only(35-40)programs/system/src/lib.rs (1)
invoke_cpi_with_read_only(133-147)program-libs/compressed-account/src/instruction_data/with_readonly.rs (1)
from(49-68)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: test-2-foresters
- GitHub Check: test-double-registration
- GitHub Check: test-state-photon-batched
- GitHub Check: test-address-batched
- GitHub Check: test-e2e
- GitHub Check: test-state-batched
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs
- GitHub Check: lint
- GitHub Check: stateless-js
🔇 Additional comments (3)
program-tests/sdk-pinocchio-test/tests/test.rs (1)
1-1: Enable test module behindtest-sbffeature.You’ve uncommented the crate-level attribute to gate these tests under the
test-sbffeature. Please verify that:
- The
test-sbffeature is declared in the Cargo.toml for this workspace.- CI/test runners enable
--features test-sbfwhen executing these tests.programs/compressed-token/src/process_transfer.rs (1)
458-458: Anchor CPI now uses typed inputs – confirm feature parityGood catch updating the call to
light_system_program::cpi::invoke_cpi_with_read_only(cpi_ctx, inputs_struct).
Ensure that every caller of this function in the codebase migrated from the oldVec<u8>signature; otherwise you will have build-time type errors that CI will catch.anchor-programs/system/src/lib.rs (1)
10-13: 👍 Moved to strongly-typed CPI inputsSwitching to explicit
InstructionDataInvokeCpiWithReadOnly/...WithAccountInfoimproves type-safety and avoids the previousVec<u8>foot-gun. No further concerns here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
program-tests/system-cpi-test/src/create_pda.rs (1)
377-441: Avoid shadowinginputs_structfor readabilityInside the read-only branch you 1) build a mutable
inputs_struct: InstructionDataInvokeCpi(lines 271-280) and 2) immediately shadow it with a newinputs_struct: InstructionDataInvokeCpiWithReadOnly(line 378).
Shadowing two structs with the same name but different types increases cognitive load.- let inputs_struct = InstructionDataInvokeCpiWithReadOnly { + let read_only_inputs = InstructionDataInvokeCpiWithReadOnly { @@ - light_system_program::cpi::invoke_cpi_with_read_only(cpi_ctx, inputs_struct)?; + light_system_program::cpi::invoke_cpi_with_read_only(cpi_ctx, read_only_inputs)?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/compressed-token/src/types.ts(0 hunks)program-tests/system-cpi-test/src/create_pda.rs(1 hunks)programs/compressed-token/src/process_transfer.rs(1 hunks)
💤 Files with no reviewable changes (1)
- js/compressed-token/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- programs/compressed-token/src/process_transfer.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
program-tests/system-cpi-test/src/create_pda.rs (2)
anchor-programs/system/src/lib.rs (1)
invoke_cpi_with_read_only(35-40)programs/system/src/lib.rs (1)
invoke_cpi_with_read_only(133-147)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: test-state-photon-batched
- GitHub Check: test-2-foresters
- GitHub Check: test-state-batched
- GitHub Check: test-double-registration
- GitHub Check: test-e2e
- GitHub Check: test-address-batched
- GitHub Check: lint
- GitHub Check: stateless-js
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs
- GitHub Check: Test concurrent-merkle-tree
🔇 Additional comments (1)
program-tests/system-cpi-test/src/create_pda.rs (1)
441-441:Details
✅ Verification successful
Confirm new CPI helper signature & serialization path
The call now forwards the typed
InstructionDataInvokeCpiWithReadOnlystruct instead of a serializedVec<u8>.
This is correct only if every re-export (light_system_program::cpi::invoke_cpi_with_read_only) has already been updated to accept the typed value. The on-chainprograms/systemhelper still takes&[u8], while the Anchor‐generated wrapper takes the struct, so a mismatch in crate versions will surface as a compile error.
🏁 Script executed:
#!/bin/bash # Locate all `invoke_cpi_with_read_only` definitions and verify their parameter types rg -A4 -n $'fn invoke_cpi_with_read_only' | sed -n '1,120p'Length of output: 792
#!/bin/bash
List top‐level directories to locate the light_system_program crate
find . -maxdepth 2 -type d
Search for all references to light_system_program in the codebase
rg -n "light_system_program" || echo "No references found"
#!/bin/bash
Locate workspace and member crates
find . -maxdepth 3 -type f -name "Cargo.toml"
Find workspace reference to light_system_program
grep -R "light_system_program" -n Cargo.toml
Inspect the light_system_program crate root for cpi module declaration
sed -n '1,200p' crates/light_system_program/src/lib.rs
Inspect the cpi module file for invoke_cpi_with_read_only signature
if [[ -f crates/light_system_program/src/cpi.rs ]]; then
sed -n '1,200p' crates/light_system_program/src/cpi.rs
else
sed -n '1,200p' crates/light_system_program/src/cpi/mod.rs
fi
🏁 Script executed:
#!/bin/bash # List all references to light_system_program to locate the crate path and usages rg -n "light_system_program" -n . # Inspect the cpi helper in the light_system_program crate (adjust path if needed) if [[ -f crates/light-system-program/src/cpi.rs ]]; then sed -n '1,200p' crates/light-system-program/src/cpi.rs elif [[ -d crates/light-system-program/src/cpi ]]; then sed -n '1,200p' crates/light-system-program/src/cpi/mod.rs fiLength of output: 36608
CPI helper signature confirmed – no changes needed
- In
anchor-programs/system/src/lib.rs,invoke_cpi_with_read_onlynow takes a typedInstructionDataInvokeCpiWithReadOnly.- The on-chain helper in
programs/system/src/lib.rsstill expects raw&[u8], and the Anchor wrapper serializes the struct under the hood.- Your call in
program-tests/system-cpi-test/src/create_pda.rs:441correctly passes the typed struct to the wrapper.- Just ensure your on-chain program and client (Anchor wrapper) dependencies stay on matching versions to avoid compilation mismatches.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
program-tests/system-cpi-v2-test/tests/event.rs (2)
5-5: UnusedDiscriminatorimport – please remove or use
Discriminatoris imported but never referenced in this module, which will trigger theunused_importslint (or a hard error if#![deny(warnings)]is active anywhere in the workspace).
Drop the import or leverageInvokeCpiWithReadOnly::discriminator()instead of hard-coding the byte array below.
725-729: Avoid extra allocations &unwrap()in instruction-data assembly
[vec1, vec2].concat()allocates twice: once for eachVec::to_vec()and once more forconcat().ix_data.try_to_vec().unwrap()will panic on serialization failure, masking bugs.A small refactor eliminates both issues:
- [ - light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(), - ix_data.try_to_vec().unwrap(), - ] - .concat(), + { + // start with the 8-byte discriminator and append the serialized payload + let mut data = + light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(); + data.extend( + ix_data + .try_to_vec() + .expect("`ix_data` Borsh serialization should not fail"), + ); + data + },Benefits: single allocation, clearer failure message, and it gives you an obvious hook (
?) if you later choose to propagate the error instead of panicking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
program-tests/create-address-test-program/src/lib.rs(1 hunks)program-tests/system-cpi-v2-test/tests/event.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- program-tests/create-address-test-program/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: test-2-foresters
- GitHub Check: test-double-registration
- GitHub Check: test-state-batched
- GitHub Check: test-state-photon-batched
- GitHub Check: test-e2e
- GitHub Check: test-address-batched
- GitHub Check: stateless-js
- GitHub Check: Test program-libs
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: lint
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
js/stateless.js/tests/unit/utils/conversion.test.ts (1)
84-88: Prefer using the real discriminator constant instead of hard-coded bytesThe first eight bytes in the fixture represent the discriminator but are currently hard-coded as
[1, 0, 0, 0, 1, 0, 0, 0], which does not match the canonical
INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATOR([86, 47, 163, 166, 21, 223, 92, 8]).
Using the actual constant makes the test self-documenting and ensures it breaks if the discriminator ever changes.- // first 8 bytes are skipped. - const data = [ - 1, 0, 0, 0, 1, 0, 0, 0, + // prepend the actual discriminator, then the serialized body + const data = [ + ...INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATOR,(remember to
import { INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATOR } from '../../../src/constants';)forester/tests/address_v2_test.rs (1)
398-403: Reduce needless allocations when prefixing the discriminator
[ … ].concat()allocates two intermediateVec<u8>instances (one from
DISCRIMINATOR.to_vec(), one fromconcat()).
You can build the vector in a single pass and avoid the extra copy:- [ - light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(), - ix_data.try_to_vec()?, - ] - .concat(), + { + let mut bytes = Vec::with_capacity( + light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.len() + + ix_data.try_to_vec()?.len(), + ); + bytes.extend_from_slice( + &light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR, + ); + bytes.extend(ix_data.try_to_vec()?); + bytes + },Not critical for tests, but keeps memory churn down if this path is exercised often.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
anchor-programs/system/src/lib.rs(2 hunks)forester/tests/address_v2_test.rs(1 hunks)js/stateless.js/src/programs/system/layout.ts(2 hunks)js/stateless.js/tests/unit/utils/conversion.test.ts(1 hunks)program-tests/utils/src/e2e_test_env.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- program-tests/utils/src/e2e_test_env.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- anchor-programs/system/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
js/stateless.js/src/programs/system/layout.ts (1)
js/stateless.js/src/constants.ts (1)
INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATOR(46-48)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: test-2-foresters
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: test-double-registration
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: test-address-batched
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: test-state-batched
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: test-state-photon-batched
- GitHub Check: test-e2e
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: stateless-js
- GitHub Check: Test sdk-libs
- GitHub Check: lint
- GitHub Check: Test program-libs
- GitHub Check: Test concurrent-merkle-tree
| instruction_data: &[u8], | ||
| ) -> Result<()> { | ||
| msg!("invoke_cpi_with_account_info"); | ||
| let instruction_data = &instruction_data[4..]; |
| data.extend_from_slice( | ||
| &light_compressed_account::discriminators::DISCRIMINATOR_INVOKE_CPI_WITH_READ_ONLY, | ||
| ); | ||
| data.extend_from_slice(&(inputs.len() as u32).to_le_bytes()); |
| let accounts = accounts.split_at(11).1; | ||
| let data = crate::instruction_data::invoke_cpi::InstructionDataInvokeCpi::deserialize( | ||
| &mut &instruction[..], | ||
| &mut &instruction[4..], |
There was a problem hiding this comment.
critical for indexing.
Here we are eliminating skip vec size bytes for the new instructions and keeping it for the old instructions.
| accounts: &[AccountInfo], | ||
| instruction_data: &[u8], | ||
| ) -> Result<()> { | ||
| let instruction_data = &instruction_data[4..]; |
Co-authored-by: Swen Schäferjohann <42959314+SwenSchaeferjohann@users.noreply.github.com>
2 participants
Issue
invoke_cpi_with_read_onlyandinvoke_cpi_with_account_infoskipped 4 borsh vector length bytes of the instruction data. This is an unnecessary inefficiency.Solution
instruction_data[0..4]statements in the system program and adjust indexing (parse_light_transaction) accordingly.Summary by CodeRabbit
Bug Fixes
Tests
Chores
Style