chore: remove unwraps system program#1803
Conversation
WalkthroughThis update enhances error handling across the system program by replacing panics with explicit error propagation, introducing new error variants, and unifying error code conversions. Several functions now use the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Function
participant ErrorEnum
Caller->>Function: Call function (e.g. zero-copy, borrow data)
Function->>Function: Perform operation
alt Operation fails
Function->>ErrorEnum: Map error to appropriate variant (e.g. BorrowingDataFailed)
Function-->>Caller: Return Err(SystemProgramError variant)
else Operation succeeds
Function-->>Caller: Return Ok(result)
end
Poem
✨ 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: 3
🧹 Nitpick comments (1)
programs/system/src/errors.rs (1)
190-191: Avoid silently collapsing largeProgramErrorcodes to zero
u32::try_from(e).unwrap_or(0)masks overflow by mapping every oversized value to0, which may already be a valid code (or reserved).
Consider surfacing an explicit sentinel (e.g.u32::MAX) or a dedicated variant so information isn’t lost.-ProgramError(e) => u32::try_from(e).unwrap_or(0), +ProgramError(e) => u32::try_from(e).unwrap_or(u32::MAX),(or return a specific custom code and log the overflow).
This keeps error codes unique and debuggable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
anchor-programs/system/src/errors.rs(1 hunks)js/compressed-token/src/types.ts(0 hunks)programs/system/src/account_compression_state/address.rs(1 hunks)programs/system/src/account_compression_state/state.rs(1 hunks)programs/system/src/accounts/remaining_account_checks.rs(5 hunks)programs/system/src/errors.rs(2 hunks)programs/system/src/invoke_cpi/process_cpi_context.rs(1 hunks)programs/system/src/lib.rs(2 hunks)
💤 Files with no reviewable changes (1)
- js/compressed-token/src/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
programs/system/src/lib.rs (2)
program-libs/compressed-account/src/instruction_data/compressed_proof.rs (1)
zero_copy_at(39-41)program-libs/compressed-account/src/instruction_data/zero_copy.rs (10)
zero_copy_at(37-50)zero_copy_at(104-107)zero_copy_at(178-188)zero_copy_at(222-238)zero_copy_at(288-305)zero_copy_at(404-419)zero_copy_at(492-519)zero_copy_at(678-708)zero_copy_at(713-726)zero_copy_at(741-743)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: test-state-photon-batched
- GitHub Check: test-double-registration
- GitHub Check: test-state-batched
- GitHub Check: test-2-foresters
- GitHub Check: test-address-batched
- GitHub Check: test-e2e
- 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: Test program-libs
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-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, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test sdk-libs
- GitHub Check: stateless-js
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: lint
🔇 Additional comments (7)
programs/system/src/lib.rs (2)
94-94: Good error propagation improvement.Replacing
.unwrap()with?properly propagates deserialization errors instead of panicking.
120-120: Consistent error handling improvement.The change from
.unwrap()to?maintains consistency with the error handling approach throughout the codebase.programs/system/src/invoke_cpi/process_cpi_context.rs (1)
108-109: Proper error mapping for deserialization failures.Good conversion from generic deserialization error to the specific
BorrowingDataFailederror type, improving error clarity.programs/system/src/accounts/remaining_account_checks.rs (3)
71-71: More specific error type for data borrowing failures.Using
BorrowingDataFailedinstead ofInvalidAccountprovides clearer error context when account data cannot be borrowed.
90-90: Consistent error propagation for tree account operations.Good replacement of
.unwrap()with?for all tree account info retrieval operations.Also applies to: 96-96, 112-112
147-148: Proper error wrapping for zero-copy deserialization.The zero-copy deserialization errors are appropriately wrapped in
SystemProgramError::ProgramError, maintaining error type consistency.Also applies to: 176-177
programs/system/src/account_compression_state/address.rs (1)
24-24: Consistent error handling in zero-copy operations.Replacing
.unwrap()with?in both functions ensures errors are properly propagated rather than causing panics.Also applies to: 32-32
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
programs/system/src/account_compression_state/state.rs (2)
55-60: Avoid duplicated header-size literals – extract a single constantThe hard-coded
8 + mem::size_of::<StateMerkleTreeAccount>()appears here and in the mutable variant below. Duplicating this calculation makes it easy for the two call-sites to drift if the header layout ever changes.+const HEADER_SIZE: usize = 8 + mem::size_of::<StateMerkleTreeAccount>(); ... - let required_size = 8 + mem::size_of::<StateMerkleTreeAccount>(); + let required_size = HEADER_SIZE;Placing
HEADER_SIZEnext toStateMerkleTreeAccount(e.g. as animpl-levelconst) keeps the magic number in one place and improves readability.
67-73: Deduplicate identical bounds-checking logic
state_merkle_tree_from_bytes_zero_copy_mutrepeats the exact same length guard and slicing logic as the immutable variant. Consider refactoring both functions to call a small helper that:
- Validates the header length once,
- Returns the post-header slice (mutable or immutable),
- Delegates to
ConcurrentMerkleTreeZeroCopy(_mut)::from_bytes_zero_copy(_mut).This reduces code repetition and guarantees the two paths stay in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
anchor-programs/system/src/errors.rs(1 hunks)programs/system/src/account_compression_state/address.rs(1 hunks)programs/system/src/account_compression_state/state.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- anchor-programs/system/src/errors.rs
- programs/system/src/account_compression_state/address.rs
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: lint
- GitHub Check: stateless-js
- 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 (random-e2e-test, ["cargo-test-sbf -p e2e-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 (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: test-double-registration
- GitHub Check: test-address-batched
- GitHub Check: test-state-batched
- GitHub Check: test-state-photon-batched
- GitHub Check: test-e2e
- GitHub Check: test-2-foresters
- GitHub Check: Test program-libs
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
🔇 Additional comments (1)
programs/system/src/account_compression_state/state.rs (1)
56-58: Confirm thatInvalidAccountis the correct error for a short data sliceThe previous review used
BorrowingDataFailed; the update switches toSystemProgramError::InvalidAccount.
Please double-check that other call-sites (and any on-chain error decoding) expectInvalidAccountfor this condition; otherwise the change can break error-handling logic upstream.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/stateless.js/tests/unit/utils/conversion.test.ts (1)
86-126: Hard-coding the full byte array is brittle – build it from the discriminator constant insteadAny future change to
INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATORwill silently break this test. Assemble the buffer dynamically so the test stays in sync with the layout:-import { decodeInstructionDataInvokeCpiWithReadOnly } from '../../../src/programs/system/layout'; +import { + decodeInstructionDataInvokeCpiWithReadOnly, +} from '../../../src/programs/system/layout'; +import { INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATOR } from '../../../src/constants'; @@ - const data = [ - 1, 0, 0, 0, 1, 0, 0, 0, 0, 148, 0, 0, /* …snip… */ 170, 1, 235, 59, - ]; + const instructionBytes = [ + 1, 0, 0, 0, 1, 0, 0, 0, 0, 148, 0, 0, /* …snip… */ 170, 1, 235, 59, + ]; + const data = [ + ...INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATOR, + ...instructionBytes, + ];This keeps the discriminator/source of truth in one place and improves test maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
anchor-programs/system/src/lib.rs(0 hunks)js/stateless.js/src/programs/system/layout.ts(2 hunks)js/stateless.js/tests/unit/utils/conversion.test.ts(1 hunks)program-tests/sdk-pinocchio-test/tests/test.rs(1 hunks)
💤 Files with no reviewable changes (1)
- anchor-programs/system/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- program-tests/sdk-pinocchio-test/tests/test.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 (20)
- GitHub Check: Test program-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test sdk-libs
- 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 (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 (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: stateless-js
- GitHub Check: test-state-photon-batched
- GitHub Check: test-state-batched
- GitHub Check: test-double-registration
- GitHub Check: test-2-foresters
- GitHub Check: test-e2e
- GitHub Check: test-address-batched
- GitHub Check: lint
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
js/stateless.js/tests/unit/utils/conversion.test.ts (1)
84-84: Comment header now correctly states “8 bytes”, good catchThe test comment was updated to match the new slice logic in
layout.ts.
No action required.js/stateless.js/src/programs/system/layout.ts (2)
26-26: Importing the new discriminator is correct and scoped
INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATORis only used in this module, which keeps external surface area minimal.
LGTM.
234-237: Offset fix removes the stale 4-byte length prefix – verify encoder symmetrySwitching to
buffer.slice(INVOKE_CPI_WITH_READ_ONLY_DISCRIMINATOR.length)matches the Rust side and the updated tests, eliminating the previous off-by-four bug.Double-check that any encoding path for the same instruction (if/when added) does not prepend the 4-byte length, or you’ll introduce an asymmetry.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
program-tests/utils/src/e2e_test_env.rs (1)
73-73:Discriminatorimport appears unusedThe trait is not referenced anywhere in this file – the associated constant is accessed through the type (
InvokeCpiWithReadOnly::DISCRIMINATOR), which does not require the trait in scope.
Unless you add an explicit call that needs the trait (e.g.InvokeCpiWithReadOnly::discriminator()), remove the import to avoid anunused importwarning.-use anchor_lang::{prelude::AccountMeta, AnchorSerialize, Discriminator}; +use anchor_lang::{prelude::AccountMeta, AnchorSerialize};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
forester/tests/address_v2_test.rs(1 hunks)program-tests/system-cpi-v2-test/tests/event.rs(1 hunks)program-tests/utils/src/e2e_test_env.rs(2 hunks)
⏰ 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 (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-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-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: test-state-photon-batched
- GitHub Check: test-double-registration
- GitHub Check: test-state-batched
- GitHub Check: test-2-foresters
- GitHub Check: test-address-batched
- GitHub Check: test-e2e
- GitHub Check: stateless-js
- GitHub Check: lint
forester/tests/address_v2_test.rs
Outdated
| let instruction = create_invoke_cpi_instruction( | ||
| payer.pubkey(), | ||
| ix_data.try_to_vec()?, | ||
| [ | ||
| light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(), | ||
| ix_data.try_to_vec()?, | ||
| ] | ||
| .concat(), |
There was a problem hiding this comment.
Potential ? type-mismatch – try_to_vec() error may not convert into RpcError
ix_data.try_to_vec()? produces Result<Vec<u8>, std::io::Error>, while the surrounding
create_v2_addresses function returns Result<(), light_client::rpc::RpcError>.
Unless impl From<std::io::Error> for RpcError exists, this will not compile.
Suggested fix (keeps panic-free behaviour and makes the conversion explicit):
- ix_data.try_to_vec()?,
+ ix_data
+ .try_to_vec()
+ .map_err(light_client::rpc::RpcError::from)?,or pre-serialize once to avoid re-allocation:
let ix_bytes = ix_data.try_to_vec().map_err(RpcError::from)?;
let instruction = create_invoke_cpi_instruction(
payer.pubkey(),
[
light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(),
ix_bytes,
]
.concat(),
remaining_accounts,
None,
);🤖 Prompt for AI Agents
In forester/tests/address_v2_test.rs around lines 396 to 402, the call to
ix_data.try_to_vec()? returns a Result with std::io::Error, which does not
automatically convert to RpcError expected by the function's Result type. To fix
this, explicitly map the error using map_err(RpcError::from) when calling
try_to_vec(), and pre-serialize ix_data into a variable before concatenation to
avoid re-allocation. Then use this serialized byte vector in
create_invoke_cpi_instruction.
| [ | ||
| light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(), | ||
| ix_data.try_to_vec().unwrap(), | ||
| ] | ||
| .concat(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Left-over unwrap() – inconsistent with the “remove unwraps” objective
The new block still forces a panic on serialization failure:
ix_data.try_to_vec().unwrap()This contradicts the PR’s stated goal of replacing unwrap()s with proper error handling.
Either propagate or explicitly handle the error, e.g.:
- ix_data.try_to_vec().unwrap(),
+ ix_data
+ .try_to_vec()
+ .map_err(light_test_utils::RpcError::from)?,(or use expect() with a clear message if you intentionally want the test to abort).
🤖 Prompt for AI Agents
In program-tests/system-cpi-v2-test/tests/event.rs around lines 725 to 729, the
code uses unwrap() on the result of try_to_vec(), which can cause a panic on
serialization failure. To fix this, replace unwrap() with proper error handling
by either propagating the error using the ? operator or handling it explicitly
with a match statement. If the test should abort on failure, use expect() with a
clear error message instead of unwrap().
| let instruction = create_invoke_cpi_instruction( | ||
| user.pubkey(), | ||
| ix_data.try_to_vec().unwrap(), | ||
| [ | ||
| light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(), | ||
| ix_data.try_to_vec().unwrap(), | ||
| ] | ||
| .concat(), | ||
| remaining_accounts, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unwrap() with ? and avoid the extra allocation
- Using
unwrap()here can panic and undermines the broader effort of getting rid of panics. - Building a temporary two-element array just to call
.concat()performs a second heap allocation. You can preallocate and extend instead.
- let instruction = create_invoke_cpi_instruction(
- user.pubkey(),
- [
- light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(),
- ix_data.try_to_vec().unwrap(),
- ]
- .concat(),
- remaining_accounts,
- None,
- );
+ let mut data =
+ light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec();
+ data.extend(ix_data.try_to_vec()?); // propagate serialization errors
+
+ let instruction = create_invoke_cpi_instruction(
+ user.pubkey(),
+ data,
+ remaining_accounts,
+ None,
+ );This removes the potential panic, propagates the error up the existing Result chain, and avoids an extra copy.
(You’ll need to add -> Result<(), RpcError> to the surrounding closure if it doesn’t already bubble the error up.)
📝 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 instruction = create_invoke_cpi_instruction( | |
| user.pubkey(), | |
| ix_data.try_to_vec().unwrap(), | |
| [ | |
| light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(), | |
| ix_data.try_to_vec().unwrap(), | |
| ] | |
| .concat(), | |
| remaining_accounts, | |
| // Preallocate and serialize without panicking | |
| let mut data = | |
| light_system_program::instruction::InvokeCpiWithReadOnly::DISCRIMINATOR.to_vec(); | |
| data.extend(ix_data.try_to_vec()?); // propagate serialization errors | |
| let instruction = create_invoke_cpi_instruction( | |
| user.pubkey(), | |
| data, | |
| remaining_accounts, | |
| None, | |
| ); |
🤖 Prompt for AI Agents
In program-tests/utils/src/e2e_test_env.rs around lines 2795 to 2802, replace
the use of unwrap() on ix_data.try_to_vec() with the ? operator to propagate
errors instead of panicking. Also, avoid creating a temporary two-element array
and calling concat(), which causes an extra heap allocation; instead,
preallocate a vector with sufficient capacity and extend it with the
discriminator bytes and the serialized ix_data bytes. Ensure the surrounding
function signature includes -> Result<(), RpcError> to support error
propagation.
ananas-block
force-pushed
the
jorrit/chore-system-program-remove-unwraps
branch
from
bd3bee6 to
4bcb7bc
Compare
| pub fn address_merkle_tree_from_bytes_zero_copy_init( | ||
| data: &mut [u8], | ||
| height: usize, | ||
| canopy_depth: usize, | ||
| changelog_capacity: usize, | ||
| roots_capacity: usize, | ||
| indexed_changelog_capacity: usize, | ||
| ) -> Result<IndexedMerkleTreeZeroCopyMut<Poseidon, usize, 26, 16>> { | ||
| let data = &mut data[8 + mem::size_of::<AddressMerkleTreeAccount>()..]; | ||
| let merkle_tree = IndexedMerkleTreeZeroCopyMut::from_bytes_zero_copy_init( | ||
| data, | ||
| height, | ||
| canopy_depth, | ||
| changelog_capacity, | ||
| roots_capacity, | ||
| indexed_changelog_capacity, | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
removed dead code
| pub fn state_merkle_tree_from_bytes_zero_copy_init( | ||
| data: &mut [u8], | ||
| height: usize, | ||
| canopy_depth: usize, | ||
| changelog_capacity: usize, | ||
| roots_capacity: usize, | ||
| ) -> Result<ConcurrentMerkleTreeZeroCopyMut<Poseidon, 26>> { | ||
| let data = &mut data[8 + mem::size_of::<StateMerkleTreeAccount>()..]; | ||
| let merkle_tree = ConcurrentMerkleTreeZeroCopyMut::from_bytes_zero_copy_init( | ||
| data, | ||
| height, | ||
| canopy_depth, | ||
| changelog_capacity, | ||
| roots_capacity, | ||
| ) | ||
| .unwrap(); | ||
| Ok(merkle_tree) | ||
| } | ||
|
|
There was a problem hiding this comment.
removed dead code
3 participants
Summary by CodeRabbit
Bug Fixes
Refactor
Style
New Features