feat: zero-copy-derive #1849
feat: zero-copy-derive #1849
Conversation
WalkthroughThis update introduces a new procedural macro crate, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RustStruct
participant MacroDerive
participant ZeroCopyStruct
participant ByteBuffer
User->>RustStruct: Define struct with #[derive(ZeroCopy, ZeroCopyMut, ZeroCopyEq, ZeroCopyConfig)]
RustStruct->>MacroDerive: Macro expansion (build, test)
MacroDerive->>ZeroCopyStruct: Generate meta struct, zero-copy struct, config struct, trait impls
User->>ByteBuffer: Serialize struct with Borsh
User->>ZeroCopyStruct: Create zero-copy view (immutable/mutable) from bytes
ZeroCopyStruct->>User: Access fields, mutate if mutable, compare with original
User->>ZeroCopyStruct: Convert zero-copy view back to owned struct (From impl)
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)warning: failed to write cache, path: /usr/local/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/pi/no/pinocchio, error: Permission denied (os error 13) Caused by: ✨ Finishing Touches
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. 🪧 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 (
|
ananas-block
force-pushed
the
jorrit/feat-borsh-zero-copy-macro3
branch
from
1a77ac8 to
da948af
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Nitpick comments (16)
program-libs/zero-copy-derive/Cargo.toml (1)
24-26: Note the circular dev-dependency.The dev-dependency on
light-zero-copywith the "derive" feature creates a circular dependency (since the derive feature depends on this crate). While this is acceptable for dev-dependencies and testing, be aware of potential build complexities.This pattern is common and generally safe for testing derive macros, but document this circular dependency if it causes any build issues.
program-libs/zero-copy-derive/README.md (1)
18-18: Provide more details about Option limitationsThe documentation mentions that "Derivation for Options is not robust and may not compile" but doesn't explain what the specific issues are or provide workarounds.
Would you like me to help document the specific limitations and potential workarounds for Option derivations?
program-libs/zero-copy-derive/tests/from_test.rs (1)
66-66: Remove redundant clone() callThe
into()method consumes the value, so callingclone()before it is unnecessary and adds overhead.- let converted: NumericStruct = zero_copy.clone().into(); + let converted: NumericStruct = zero_copy.into();program-libs/zero-copy-derive/src/from_impl.rs (2)
21-25: Remove unused variableThe variable
_z_struct_meta_nameis created but never used in the function.- let _z_struct_meta_name = if MUT { - format_ident!("{}MetaMut", z_struct_name) - } else { - format_ident!("{}Meta", z_struct_name) - };
101-103: Remove implementation-specific commentThe comment mentions "compressed proofs" which appears to be specific to a particular use case and shouldn't be in generic procedural macro code.
- // Create a clone of the Some value - for compressed proofs and other structs - // For instruction_data.rs, we just need to clone the value directly + // Convert and clone the inner valueprogram-libs/zero-copy-derive/src/config.rs (1)
117-128: Inefficient VecU8 initializationThe code creates a
ZeroCopySliceMutBorshinstance but discards it, only using it for its side effect of writing the length prefix. This is inefficient and unclear.Consider using the returned slice directly or add a comment explaining why this approach is necessary:
- // Initialize the length prefix but don't use the returned ZeroCopySliceMut - { - light_zero_copy::slice_mut::ZeroCopySliceMutBorsh::<u8>::new_at( - config.#field_name.into(), - bytes - )?; - } - // Split off the length prefix (4 bytes) and get the slice - let (_, bytes) = bytes.split_at_mut(4); - let (#field_name, bytes) = bytes.split_at_mut(config.#field_name as usize); + // Write length prefix and get the data slice + use light_zero_copy::little_endian::U32; + use light_zero_copy::Ref; + let (mut len_ref, bytes) = Ref::<&mut [u8], U32>::from_prefix(bytes)?; + *len_ref = U32::new(config.#field_name); + let (#field_name, bytes) = bytes.split_at_mut(config.#field_name as usize);program-libs/zero-copy-derive/src/deserialize_impl.rs (1)
216-584: Consider removing or relocating commented test code.Large blocks of commented code reduce maintainability. If these tests are valuable, consider moving them to a proper test file in the
tests/directory. Otherwise, remove them to keep the codebase clean.program-libs/zero-copy-derive/src/partial_eq_impl.rs (1)
102-138: Unreachable Option handling code.This Option handling code is unreachable due to the panic at line 69. Consider removing it to avoid confusion.
Since Options are explicitly not supported (line 69 panics), remove this unreachable code to improve clarity.
program-libs/zero-copy-derive/src/lib.rs (2)
49-55: Address or track TODO items.The TODO section lists several unfinished tasks that should be addressed before the code is production-ready.
Would you like me to create GitHub issues to track these TODO items? The tasks include:
- Test and fix boolean support for mut derivation
- Add more tests for mut derivation
- Rename deserialize traits
- Manual code review of generated code
- Fix partial eq generation for options
250-250: Clarify hasher implementation status.The comment says "Keep consistent with ZeroCopy macro" but it's unclear if hasher support is planned or permanently disabled.
Add a more descriptive comment:
- let hasher = false; // Keep consistent with ZeroCopy macro + // TODO: Hasher support is disabled - will be implemented in future version + let hasher = false;program-libs/zero-copy-derive/tests/random.rs (3)
2-2: Remove unnecessary import.The
assert_eq!macro is available without importing from std.-use std::assert_eq; - use borsh::BorshDeserialize;
40-300: Consider refactoring repetitive copying logic.The
populate_invoke_cpi_zero_copyandpopulate_invoke_zero_copyfunctions contain very similar, repetitive copying logic that could be refactored.Consider extracting common patterns into helper macros or generic functions. For example:
macro_rules! copy_option_field { ($src:expr, $dst:expr, $field:ident) => { if let (Some(src_val), Some(dst_val)) = (&$src.$field, &mut $dst.$field) { *dst_val = src_val.clone(); } }; } macro_rules! copy_slice_field { ($src:expr, $dst:expr, $field:ident) => { $dst.$field.copy_from_slice(&$src.$field); }; }This would reduce code duplication and make the functions more maintainable.
605-605: Consider reducing iteration count for regular test runs.Running 1,000,000 iterations might be excessive for regular CI runs and could cause timeouts.
Consider using an environment variable to control iteration count:
- let num_iters = 1_000_000; + let num_iters = std::env::var("FUZZ_ITERATIONS") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(1000); // Default to 1000 for regular runsThis allows thorough testing when needed while keeping regular test runs fast.
program-libs/zero-copy-derive/tests/instruction_data.rs (3)
59-61: Remove or clarify misleading commentThe comment states "We should not implement DeserializeMut for primitive types directly" but Pubkey is not a primitive type - it's a newtype wrapper around
[u8; 32].-// We should not implement DeserializeMut for primitive types directly -// The implementation should be in the zero-copy crate - +// Manual implementation for Pubkey as it's a fundamental type +// used throughout the codebase
106-191: Remove large blocks of commented-out codeThis large block of commented manual implementation appears to be replaced by the
ZeroCopyConfigderive macro. Keeping it reduces code readability.Consider removing this entire commented block since the functionality is now provided by the derive macro. If needed for reference, it should be moved to documentation or a separate example file.
1178-1294: Consider generating PartialEq implementations with derive macroThis manual PartialEq implementation is quite complex and error-prone. Since similar patterns appear throughout the codebase, consider extending the derive macros to generate these comparison implementations automatically.
Would you like me to help design a derive macro that could generate these PartialEq implementations between owned and zero-copy types?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
Cargo.toml(2 hunks)program-libs/zero-copy-derive/Cargo.toml(1 hunks)program-libs/zero-copy-derive/README.md(1 hunks)program-libs/zero-copy-derive/src/byte_len_derive.rs(1 hunks)program-libs/zero-copy-derive/src/config.rs(1 hunks)program-libs/zero-copy-derive/src/deserialize_impl.rs(1 hunks)program-libs/zero-copy-derive/src/from_impl.rs(1 hunks)program-libs/zero-copy-derive/src/lib.rs(1 hunks)program-libs/zero-copy-derive/src/meta_struct.rs(1 hunks)program-libs/zero-copy-derive/src/partial_eq_impl.rs(1 hunks)program-libs/zero-copy-derive/src/utils.rs(1 hunks)program-libs/zero-copy-derive/src/z_struct.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_struct_inner.rs(1 hunks)program-libs/zero-copy-derive/tests/config_test.rs(1 hunks)program-libs/zero-copy-derive/tests/from_test.rs(1 hunks)program-libs/zero-copy-derive/tests/instruction_data.rs(1 hunks)program-libs/zero-copy-derive/tests/random.rs(1 hunks)program-libs/zero-copy/Cargo.toml(1 hunks)program-libs/zero-copy/README.md(0 hunks)program-libs/zero-copy/src/borsh.rs(3 hunks)program-libs/zero-copy/src/borsh_mut.rs(1 hunks)program-libs/zero-copy/src/init_mut.rs(1 hunks)program-libs/zero-copy/src/lib.rs(1 hunks)program-libs/zero-copy/src/slice_mut.rs(1 hunks)program-libs/zero-copy/tests/borsh.rs(1 hunks)program-libs/zero-copy/tests/borsh_2.rs(1 hunks)
💤 Files with no reviewable changes (1)
- program-libs/zero-copy/README.md
🧰 Additional context used
🧬 Code Graph Analysis (5)
program-libs/zero-copy-derive/src/config.rs (2)
program-libs/zero-copy-derive/src/utils.rs (1)
get_vec_inner_type(85-98)program-libs/zero-copy/src/init_mut.rs (34)
config(237-240)new_zero_copy(28-31)new_zero_copy(53-69)new_zero_copy(81-87)new_zero_copy(98-104)new_zero_copy(115-121)new_zero_copy(132-138)new_zero_copy(149-155)new_zero_copy(172-178)new_zero_copy(190-195)new_zero_copy(206-211)new_zero_copy(222-227)new_zero_copy(243-263)size_of(78-78)size_of(95-95)size_of(112-112)size_of(129-129)size_of(146-146)size_of(169-169)size_of(187-187)size_of(203-203)size_of(219-219)byte_len(23-23)byte_len(42-51)byte_len(77-79)byte_len(94-96)byte_len(111-113)byte_len(128-130)byte_len(145-147)byte_len(168-170)byte_len(186-188)byte_len(202-204)byte_len(218-220)byte_len(235-241)
program-libs/zero-copy-derive/src/byte_len_derive.rs (4)
program-libs/zero-copy-derive/src/z_struct.rs (2)
analyze_struct_fields(56-123)field(136-143)program-libs/zero-copy-derive/src/utils.rs (1)
is_bool_type(132-139)program-libs/zero-copy/src/init_mut.rs (21)
size_of(78-78)size_of(95-95)size_of(112-112)size_of(129-129)size_of(146-146)size_of(169-169)size_of(187-187)size_of(203-203)size_of(219-219)byte_len(23-23)byte_len(42-51)byte_len(77-79)byte_len(94-96)byte_len(111-113)byte_len(128-130)byte_len(145-147)byte_len(168-170)byte_len(186-188)byte_len(202-204)byte_len(218-220)byte_len(235-241)program-libs/zero-copy/src/borsh_mut.rs (3)
size_of(44-44)size_of(66-66)size_of(69-69)
program-libs/zero-copy-derive/tests/from_test.rs (2)
program-libs/zero-copy-derive/tests/instruction_data.rs (1)
zero_copy_at(43-45)program-libs/zero-copy/src/borsh.rs (15)
zero_copy_at(20-20)zero_copy_at(27-30)zero_copy_at(36-49)zero_copy_at(58-64)zero_copy_at(90-93)zero_copy_at(99-111)zero_copy_at(139-149)zero_copy_at(341-344)zero_copy_at(399-403)zero_copy_at(456-461)zero_copy_at(538-553)zero_copy_at(594-597)zero_copy_at(631-634)zero_copy_at(709-713)zero_copy_at(768-772)
program-libs/zero-copy-derive/src/partial_eq_impl.rs (4)
program-libs/zero-copy-derive/src/z_struct.rs (2)
analyze_struct_fields(56-123)name(32-52)program-libs/zero-copy-derive/tests/instruction_data.rs (9)
from(630-637)from(641-648)eq(63-65)eq(652-673)eq(702-723)eq(1179-1287)eq(1291-1293)eq(1299-1364)eq(1370-1422)program-libs/zero-copy/src/slice_mut.rs (2)
as_slice(147-149)eq(263-265)program-libs/zero-copy/src/borsh_mut.rs (3)
eq(441-447)eq(806-812)eq(898-900)
program-libs/zero-copy-derive/src/lib.rs (8)
program-libs/zero-copy-derive/src/from_impl.rs (4)
syn(97-97)generate_from_impl(9-182)generate_from_impl(208-208)generate_from_impl(240-240)program-libs/zero-copy-derive/src/z_struct.rs (4)
name(32-52)generate_z_struct(347-448)generate_z_struct(538-538)analyze_struct_fields(56-123)program-libs/zero-copy-derive/src/utils.rs (2)
process_input(16-40)process_fields(42-61)program-libs/zero-copy-derive/src/meta_struct.rs (1)
generate_meta_struct(9-56)program-libs/zero-copy-derive/src/zero_copy_struct_inner.rs (1)
generate_zero_copy_struct_inner(6-25)program-libs/zero-copy-derive/src/deserialize_impl.rs (1)
generate_deserialize_impl(155-214)program-libs/zero-copy-derive/src/partial_eq_impl.rs (1)
generate_partial_eq_impl(220-251)program-libs/zero-copy-derive/src/config.rs (3)
generate_config_struct(81-111)generate_field_initialization(114-257)generate_byte_len_calculation(260-368)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: cli-v2
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v2
- GitHub Check: lint
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
🔇 Additional comments (37)
program-libs/zero-copy/Cargo.toml (3)
14-15: LGTM! Well-structured feature flags.The feature flag setup correctly enables the derive functionality with proper feature propagation for the
mutfeature.
22-22: LGTM! Proper optional dependency setup.The derive crate is correctly added as an optional workspace dependency.
27-27: LGTM! Appropriate dev-dependency addition.Adding borsh as a dev-dependency is reasonable for testing serialization compatibility.
Cargo.toml (2)
16-16: LGTM! Proper workspace member addition.The new derive crate is correctly added to the workspace members list.
171-171: LGTM! Consistent workspace dependency setup.The workspace dependency follows the established pattern with proper version and path specification.
program-libs/zero-copy/src/slice_mut.rs (1)
280-291: LGTM! Clean trait implementation with proper delegation.The
DeserializeMutimplementation correctly delegates to the existingfrom_bytes_atmethod, preserving error handling and lifetime semantics. The feature gating is appropriate.program-libs/zero-copy-derive/src/zero_copy_struct_inner.rs (1)
6-25: Approve code: Trait paths verifiedThe traits referenced by the generated code exist and match the expected locations:
ZeroCopyStructInnerfound inprogram-libs/zero-copy/src/borsh.rsZeroCopyStructInnerMutfound inprogram-libs/zero-copy/src/borsh_mut.rsLGTM—merging.
program-libs/zero-copy-derive/Cargo.toml (2)
12-13: LGTM! Proper proc-macro crate setup.The
[lib]section correctly configures this as a procedural macro crate.
15-19: LGTM! Standard proc-macro dependencies with appropriate features.The dependency versions and syn features ("full", "extra-traits") are well-suited for comprehensive proc-macro functionality.
program-libs/zero-copy/src/lib.rs (1)
13-32: Module integration looks good!The new modules and re-exports are properly feature-gated and follow the existing pattern in the crate.
program-libs/zero-copy/src/borsh.rs (1)
85-94: Array deserialization implementation is correct.The implementation properly delegates to
Ref::from_prefixfor fixed-size arrays.program-libs/zero-copy-derive/src/byte_len_derive.rs (1)
10-132: Well-documented ByteLen derive implementation!The implementation correctly handles all field types with appropriate special cases for booleans and options. The extensive documentation clearly explains the rules and rationale.
program-libs/zero-copy-derive/src/z_struct.rs (2)
319-344: Boolean accessor methods are correctly implemented.The accessor methods properly handle the conversion from stored
u8values toboolreturn types, with appropriate handling for both mutable and immutable variants.
450-624: Comprehensive fuzz testing!The fuzz test effectively validates the zero-copy struct generation with randomized inputs, ensuring robustness of the implementation.
program-libs/zero-copy/src/init_mut.rs (1)
6-264: Clean and extensible ZeroCopyNew trait implementation!The trait design and implementations are well-structured:
- Configuration-based initialization allows flexible buffer allocation
- Proper error handling throughout
- Consistent handling of endianness with little-endian types
- Vec implementation correctly handles length prefix and sequential initialization
program-libs/zero-copy-derive/tests/config_test.rs (9)
1-1: LGTM! Feature guard appropriately used.The feature flag ensures these tests only run when mutable zero-copy support is enabled.
7-24: Well-structured test struct for Vec configuration.The struct correctly uses
#[repr(C)]for predictable memory layout and derives all necessary traits for zero-copy functionality.
25-40: Clear and focused config generation test.The test effectively validates that the derive macro generates the expected configuration struct with appropriate field types.
41-99: Comprehensive test for zero-copy initialization and mutation.The test thoroughly validates buffer sizing, initialization, field mutation, and round-trip deserialization. The exact buffer size check ensures memory efficiency.
110-160: Proper testing of Option field when enabled.The test correctly validates Option field initialization and mutation, with appropriate safety checks before accessing Option values.
161-199: Good coverage of Option field when disabled.The test validates that disabled Options remain None and consume minimal buffer space.
212-279: Excellent test combining Vec and Option fields.The test thoroughly validates mixed field types with proper configuration and mutation testing.
326-395: Thorough validation of byte length calculations.The test provides excellent coverage of memory layout calculations with clear expected size breakdowns. This ensures efficient buffer allocation.
396-444: Practical demonstration of dynamic buffer allocation.The test effectively shows the real-world usage pattern of calculating exact buffer sizes for zero-copy initialization.
program-libs/zero-copy-derive/src/deserialize_impl.rs (3)
10-141: Well-structured field deserialization code generation.The function correctly handles all field types with appropriate mutable/immutable variants. The special optimization for
Vec<u8>using dedicated slice functions is a good performance consideration.
143-152: Simple and correct field initialization helper.The function appropriately generates field names for struct construction.
154-214: Comprehensive trait implementation generation.The function correctly generates complete Deserialize/DeserializeMut implementations with proper handling of meta structs and mutable variants.
program-libs/zero-copy/tests/borsh_2.rs (2)
1-30: Excellent documentation of zero-copy transformation rules.The clearly documented rules provide a valuable specification for the expected behavior of zero-copy struct generation.
31-560: Comprehensive test coverage with progressive complexity.The test suite excellently covers various struct patterns from simple to complex, providing a solid reference implementation for the derive macro behavior. Each test validates both the zero-copy deserialization and data integrity.
program-libs/zero-copy-derive/src/partial_eq_impl.rs (2)
65-70: Options are explicitly not supported but panic could be clearer.The unimplemented panic provides a clear message, but consider using a compile-time error via trait bounds or a more descriptive error message.
219-252: Well-structured PartialEq implementation generation.The function correctly generates complete PartialEq implementations with proper handling of meta fields presence.
program-libs/zero-copy-derive/src/lib.rs (1)
477-477: Use fully qualified path for Ref.The code uses
Refwithout the full path, which could fail ifzerocopyis not in scope.- use zerocopy::Ref; + use light_zero_copy::Ref;Or use the fully qualified path directly:
- let (__meta, bytes) = Ref::<&mut [u8], #z_meta_name>::from_prefix(bytes)?; + let (__meta, bytes) = light_zero_copy::Ref::<&mut [u8], #z_meta_name>::from_prefix(bytes)?;Likely an incorrect or invalid review comment.
program-libs/zero-copy-derive/src/utils.rs (1)
289-408: Well-structured tests for Copy trait detection.The tests comprehensively cover various struct configurations and properly clear the global cache before testing.
program-libs/zero-copy-derive/tests/random.rs (1)
302-502: Well-structured random data generation.The helper functions for generating random test data are comprehensive and well-organized, providing good coverage for property-based testing.
program-libs/zero-copy/tests/borsh.rs (1)
1-335: Comprehensive test coverage!The test suite thoroughly exercises the zero-copy derive macros across various data structures including primitives, vectors, nested structs, options, and arrays. The round-trip serialization tests and mutable access tests provide good confidence in the implementation.
program-libs/zero-copy-derive/tests/instruction_data.rs (2)
72-74: Clean implementation for fixed-size typeThe byte_len implementation correctly returns the constant size for Pubkey.
776-828: Well-structured test for CompressedAccountData initializationThe test thoroughly validates:
- Exact buffer size calculation
- Mutable field access
- Round-trip deserialization
- Field value persistence
Excellent test coverage!
| lazy_static::lazy_static! { | ||
| static ref COPY_IMPL_CACHE: Arc<Mutex<HashMap<String, bool>>> = Arc::new(Mutex::new(HashMap::new())); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider potential struct name collisions in global cache.
The cache uses only the struct name as the key, which could lead to collisions if different modules define structs with the same name.
Consider using a more unique key that includes module path or crate information to avoid collisions. Alternatively, document this limitation clearly.
Also applies to: 222-223, 231-233
🏁 Script executed:
#!/bin/bash
# Check if there are any duplicate struct names across different modules
fd -e rs | xargs ast-grep --pattern 'struct $STRUCT_NAME' | grep -E "^struct " | awk '{print $2}' | sort | uniq -dLength of output: 116
I’ll re-run a more reliable scan for duplicate struct names:
🏁 Script executed:
#!/bin/bash
# Find duplicate struct names across all Rust files
rg --only-matching --no-line-number --regexp 'struct\s+[A-Za-z_]\w*' --glob '*.rs' \
| awk '{print $2}' \
| sort \
| uniq -dLength of output: 2575
Potential cache key collisions detected in global COPY_IMPL_CACHE
A repository-wide scan for duplicate struct definitions revealed many structs sharing the same name across different modules (e.g. AccountProofInputs, AddressProofInputs, MyStruct, TestStruct, … and dozens more). Using only the struct’s bare name as the cache key will allow these to collide.
Recommendations:
- Switch the cache key to something fully qualified, for example:
std::any::type_name::<T>()- A manually constructed
"module::StructName"string TypeIdor a hash of the type’s module path + name
- Ensure that wherever you call
process_input, you use the same fully qualified key. - Update the documentation (or add a comment) to explain the key format so future contributors don’t introduce accidental collisions.
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/src/utils.rs around lines 11 to 13, the cache
key for COPY_IMPL_CACHE uses only the struct's bare name, causing collisions for
structs with the same name in different modules. To fix this, change the cache
key to a fully qualified type identifier such as std::any::type_name::<T>(), or
a manually constructed "module::StructName" string, or use TypeId. Also, update
all calls to process_input to use this fully qualified key and add a comment
explaining the key format to prevent future collisions.
| let (num_slices, mut bytes) = Ref::<&mut [u8], u8>::from_prefix(bytes)?; | ||
| let num_slices = u32::from(*num_slices) as usize; |
There was a problem hiding this comment.
Fix incorrect type casting in VecU8 deserialization
The code reads a u8 but then casts it to u32, which seems incorrect for a VecU8 type that should use single-byte length encoding.
-let (num_slices, mut bytes) = Ref::<&mut [u8], u8>::from_prefix(bytes)?;
-let num_slices = u32::from(*num_slices) as usize;
+let (num_slices, mut bytes) = Ref::<&mut [u8], u8>::from_prefix(bytes)?;
+let num_slices = *num_slices as usize;📝 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 (num_slices, mut bytes) = Ref::<&mut [u8], u8>::from_prefix(bytes)?; | |
| let num_slices = u32::from(*num_slices) as usize; | |
| let (num_slices, mut bytes) = Ref::<&mut [u8], u8>::from_prefix(bytes)?; | |
| let num_slices = *num_slices as usize; |
🤖 Prompt for AI Agents
In program-libs/zero-copy/src/borsh_mut.rs at lines 200-201, the code
incorrectly casts a u8 value to u32 when deserializing VecU8 length. To fix
this, remove the cast to u32 and use the u8 value directly as the length,
ensuring the length is handled as a single byte without unnecessary type
conversion.
| // TODO: rename to ZeroCopy, can be used as <StructName as DeserializeMut>::ZeroCopy | ||
| type Output; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the TODO: Consider renaming Output to ZeroCopy
The TODO suggests renaming the associated type to ZeroCopy for better clarity. This would make the usage pattern <StructName as DeserializeMut>::ZeroCopy more intuitive.
🏁 Script executed:
#!/bin/bash
# Check if this Output type is used elsewhere in the codebase
rg -A 2 "DeserializeMut.*Output" --type rustLength of output: 4256
Rename associated type Output to ZeroCopy across all usages
The Output associated type is referenced in multiple locations; updating it will require coordinated changes in both the zero-copy library and the derive crate:
• program-libs/zero-copy/src/borsh_mut.rs (trait definition)
• program-libs/zero-copy/src/init_mut.rs (implementations)
• program-libs/zero-copy-derive/src/lib.rs (derive macro expansion)
• program-libs/zero-copy-derive/tests/instruction_data.rs (tests)
• Any other occurrences of <Struct as DeserializeMut>::Output
Suggested diff for the trait:
--- a/program-libs/zero-copy/src/borsh_mut.rs
+++ b/program-libs/zero-copy/src/borsh_mut.rs
@@ trait DeserializeMut<'a> {
- // TODO: rename to ZeroCopy, can be used as <StructName as DeserializeMut>::ZeroCopy
- type Output;
+ /// Renamed for clarity: use `<StructName as DeserializeMut>::ZeroCopy`
+ type ZeroCopy;
}And update all usages accordingly:
- <Self as DeserializeMut<'a>>::Output
+ <Self as DeserializeMut<'a>>::ZeroCopyMake sure to update the derive implementation and tests to reference the new associated type name.
🤖 Prompt for AI Agents
In program-libs/zero-copy/src/borsh_mut.rs at lines 18-19, rename the associated
type `Output` to `ZeroCopy` in the trait definition. Then, update all references
to this associated type throughout the codebase, including implementations in
program-libs/zero-copy/src/init_mut.rs, the derive macro expansions in
program-libs/zero-copy-derive/src/lib.rs, and relevant tests in
program-libs/zero-copy-derive/tests/instruction_data.rs. Also, search for and
replace all occurrences of `<Struct as DeserializeMut>::Output` with `<Struct as
DeserializeMut>::ZeroCopy` to maintain consistency and ensure the code compiles
and tests pass.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (22)
program-libs/zero-copy-derive/src/meta_struct.rs (2)
9-17: Consider simplifying the mutable name generation logic.The current approach clones the identifier and conditionally modifies it. This could be streamlined for better readability.
- let mut z_struct_meta_name = z_struct_meta_name.clone(); - if MUT { - z_struct_meta_name = format_ident!("{}Mut", z_struct_meta_name); - } + let z_struct_meta_name = if MUT { + format_ident!("{}Mut", z_struct_meta_name) + } else { + z_struct_meta_name.clone() + };
20-39: Review the attribute handling logic for potential improvements.The attribute processing creates an empty quote when hasher is false, which works but could be more explicit about the intent.
- let attributes = if hasher { - field - .attrs - .iter() - .map(|attr| { - let path = attr; - quote! { #path } - }) - .collect::<Vec<_>>() - } else { - vec![quote! {}] - }; + let attributes = if hasher { + field.attrs.iter().map(|attr| quote! { #attr }).collect::<Vec<_>>() + } else { + Vec::new() + };program-libs/zero-copy-derive/tests/from_test.rs (1)
64-66: Unnecessary cloning in the test.The
clone()call on the zero-copy instance is unnecessary since theFromtrait should work with owned values.- let converted: NumericStruct = zero_copy.clone().into(); + let converted: NumericStruct = zero_copy.into();program-libs/zero-copy-derive/src/byte_len_derive.rs (2)
69-96: Consider consolidating repetitive field type handling.The match arms for most field types are identical, delegating to
byte_len(). This could be simplified for better maintainability.- FieldType::VecU8(field_name) - | FieldType::VecCopy(field_name, _) - | FieldType::VecNonCopy(field_name, _) - | FieldType::Array(field_name, _) - | FieldType::Option(field_name, _) - | FieldType::Pubkey(field_name) - | FieldType::IntegerU64(field_name) - | FieldType::IntegerU32(field_name) - | FieldType::IntegerU16(field_name) - | FieldType::IntegerU8(field_name) - | FieldType::Copy(field_name, _) - | FieldType::NonCopy(field_name, _) => { - quote! { self.#field_name.byte_len() } - }, + field_type if matches!(field_type, + FieldType::VecU8(_) | FieldType::VecCopy(_, _) | FieldType::VecNonCopy(_, _) | + FieldType::Array(_, _) | FieldType::Option(_, _) | FieldType::Pubkey(_) | + FieldType::IntegerU64(_) | FieldType::IntegerU32(_) | FieldType::IntegerU16(_) | + FieldType::IntegerU8(_) | FieldType::Copy(_, _) | FieldType::NonCopy(_, _) + ) => { + let field_name = field_type.field_name(); // Assuming this method exists + quote! { self.#field_name.byte_len() } + },
99-122: Complex token combination logic could benefit from helper functions.The logic for combining meta and struct field byte lengths is correct but complex. Consider extracting helper functions for better readability.
Consider extracting the token combination logic into helper functions:
fn combine_tokens(tokens: impl Iterator<Item = TokenStream>) -> TokenStream { tokens.fold(quote!(), |acc, item| { if acc.is_empty() { item } else { quote! { #acc + #item } } }) }program-libs/zero-copy-derive/README.md (1)
18-18: Add note about derive macro ordering requirements.Consider adding information about whether the order of derive macros matters, as this is often a source of confusion for users.
Add a note about derive macro ordering:
- Derivation for Options<struct> is not robust and may not compile. - When using multiple derive macros, ensure `ZeroCopy` is derived before `ZeroCopyEq`.program-libs/zero-copy-derive/src/from_impl.rs (2)
21-25: Remove dead code
_z_struct_meta_nameis computed but never used, triggering an unused-variable warning and needlessly cloning anIdent.
Delete it to keep the macro clean.
95-95: Remove strayprintln!Debug printing inside a proc-macro leaks to build logs and slows compilation.
program-libs/zero-copy/src/borsh.rs (2)
140-147: Length prefix inconsistency
VecU8::zero_copy_atreads the length as a singleu8, while the genericVec<T>implementation uses a 4-byte LEU32.
Consider standardising on the same 4-byte prefix (or document why this special-case is safe) to avoid confusion and divergent encoding rules.
178-180:'staticlifetime on array inner may over-constrain callers
type ZeroCopyInner = Ref<&'static [u8], [u8; N]>;Using
'staticprevents this helper from being used in generic contexts that expect the lifetime to flow from the outer struct.
Switch to an unconstrained lifetime parameter:type ZeroCopyInner<'a> = Ref<&'a [u8], [u8; N]>;and add a corresponding lifetime parameter to the trait if needed.
program-libs/zero-copy-derive/src/deserialize_impl.rs (1)
18-28: Minor duplication intrait_pathconstruction
trait_pathis recomputed inside several match arms (VecCopy,VecNonCopy, …). A tiny helper would cut repetition and lower cognitive load.program-libs/zero-copy-derive/src/partial_eq_impl.rs (1)
21-38: Redundant double-cast when comparing meta integersExample:
if other.foo != u64::from(meta.foo) as u32 { … }
meta.foois already a little-endian wrapper; usingu32::from(meta.foo)(oru64::fromfor 64-bit) avoids the extra cast and potential truncation accidents.program-libs/zero-copy-derive/src/z_struct.rs (1)
427-444: Accessor helpers miss mutable variant for meta bool fieldsMutable derivation adds
DerefMutfor__meta, but accessor helpers for meta booleans are always generated withMUT = false, soZStructMutlacksset_*or even read helpers that use the correct*self.field > 0pattern. Consider invokinggenerate_bool_accessor_methods::<true>for meta fields whenMUTis enabled.program-libs/zero-copy-derive/tests/random.rs (1)
605-616: 10 000 iterations is excessive for a unit-testRunning 10k randomized iterations per CI job noticeably increases wall-clock time and can lead to flakiness. Consider lowering the count (e.g. 1-2k) or gate the long run behind an env-flag /
#[cfg(feature = "stress_test")].program-libs/zero-copy-derive/src/utils.rs (1)
172-205: Attribute parsing is brittle
struct_has_copy_derivefalls back to string-contains scanning (content.contains("Copy")).
This can yield false positives (e.g.NonCopy) and incurs unnecessary token-stream allocation.
Preferattr.meta()?.require_list()?.nested.iter().any(...)fromsynfor robust matching.program-libs/zero-copy/tests/borsh_2.rs (3)
3-3: Remove the unnecessaryvecimport.
Vecis already in the prelude and the module itself is never referenced.-use std::{ops::Deref, vec}; +use std::ops::Deref;
56-62: Abstract repeatedDerefimpls with a helper macro.Every
ZStruct*re-implements the sameDerefboilerplate.
Consider a small helper macro (or the upcoming derive) to eliminate this duplication and keep the tests concise.
266-269: Use a single helper (zero_copy_at) for slice deserialization to stay consistent.Earlier structs call
ZeroCopySliceBorsh::zero_copy_at; here you switched tofrom_bytes_at.
Unless both names are strictly required, stick to one to avoid reader confusion:- let (vec, bytes) = ZeroCopySliceBorsh::from_bytes_at(bytes)?; + let (vec, bytes) = ZeroCopySliceBorsh::zero_copy_at(bytes)?; ... - let (vec_2, bytes) = ZeroCopySliceBorsh::from_bytes_at(bytes)?; + let (vec_2, bytes) = ZeroCopySliceBorsh::zero_copy_at(bytes)?;program-libs/zero-copy-derive/tests/config_test.rs (3)
25-39: Basic test looks good but could validate more config propertiesThe test verifies that the configuration struct is generated and basic field access works, but could be more comprehensive.
Consider adding validation that the generated config struct has the expected type and structure:
#[test] fn test_simple_config_generation() { // This test verifies that the ZeroCopyConfig derive macro generates the expected config struct // and ZeroCopyNew implementation // The config should have been generated as SimpleVecStructConfig let config = SimpleVecStructConfig { vec: 10, // Vec<u8> should have u32 config (length) }; // Test that we can create a configuration assert_eq!(config.vec, 10); + + // Verify the config has the expected type + assert_eq!(std::mem::size_of::<SimpleVecStructConfig>(), std::mem::size_of::<u32>()); println!("Config generation test passed!"); }
68-84: Direct access to __meta field exposes implementation detailsThe tests directly access the
__metafield, which appears to be an internal implementation detail of the zero-copy derive macro. This creates tight coupling between tests and implementation.Consider providing a more encapsulated API for accessing meta fields, or document that
__metais part of the public API if intentional:// Instead of: simple_struct.__meta.a = 42; // Consider: simple_struct.set_meta_a(42); or simple_struct.meta().a = 42;
140-142: Complex nested dereferencing pattern for Option valuesThe pattern
**opt_val = 98765u64.into()with double dereferencing and conversion is complex and error-prone.Consider providing a more ergonomic API for Option field manipulation:
// Test that option is Some and we can set its value assert!(simple_struct.option.is_some()); -if let Some(ref mut opt_val) = simple_struct.option { - **opt_val = 98765u64.into(); -} +if let Some(opt_val) = simple_struct.option.as_mut() { + opt_val.set(98765u64); +}program-libs/zero-copy/tests/borsh.rs (1)
90-90: Field access requiring explicit conversion suggests type wrapper overheadThe pattern
u64::from(*zero_copy.c)indicates that fieldcis wrapped in a type that requires explicit conversion, which could impact ergonomics.Consider whether the conversion overhead is necessary or if the API could be made more ergonomic:
// Current: u64::from(*zero_copy.c) // Possible: zero_copy.c.get() or zero_copy.c.value()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/rust.yml(1 hunks)Cargo.toml(2 hunks)program-libs/zero-copy-derive/Cargo.toml(1 hunks)program-libs/zero-copy-derive/README.md(1 hunks)program-libs/zero-copy-derive/src/byte_len_derive.rs(1 hunks)program-libs/zero-copy-derive/src/config.rs(1 hunks)program-libs/zero-copy-derive/src/deserialize_impl.rs(1 hunks)program-libs/zero-copy-derive/src/from_impl.rs(1 hunks)program-libs/zero-copy-derive/src/lib.rs(1 hunks)program-libs/zero-copy-derive/src/meta_struct.rs(1 hunks)program-libs/zero-copy-derive/src/partial_eq_impl.rs(1 hunks)program-libs/zero-copy-derive/src/utils.rs(1 hunks)program-libs/zero-copy-derive/src/z_struct.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_struct_inner.rs(1 hunks)program-libs/zero-copy-derive/tests/config_test.rs(1 hunks)program-libs/zero-copy-derive/tests/from_test.rs(1 hunks)program-libs/zero-copy-derive/tests/instruction_data.rs(1 hunks)program-libs/zero-copy-derive/tests/random.rs(1 hunks)program-libs/zero-copy/Cargo.toml(1 hunks)program-libs/zero-copy/README.md(0 hunks)program-libs/zero-copy/src/borsh.rs(3 hunks)program-libs/zero-copy/src/borsh_mut.rs(1 hunks)program-libs/zero-copy/src/init_mut.rs(1 hunks)program-libs/zero-copy/src/lib.rs(1 hunks)program-libs/zero-copy/src/slice_mut.rs(1 hunks)program-libs/zero-copy/tests/borsh.rs(1 hunks)program-libs/zero-copy/tests/borsh_2.rs(1 hunks)
💤 Files with no reviewable changes (1)
- program-libs/zero-copy/README.md
🧰 Additional context used
🧬 Code Graph Analysis (5)
program-libs/zero-copy-derive/tests/from_test.rs (2)
program-libs/zero-copy-derive/tests/instruction_data.rs (1)
zero_copy_at(43-45)program-libs/zero-copy/src/borsh.rs (15)
zero_copy_at(20-20)zero_copy_at(27-30)zero_copy_at(36-49)zero_copy_at(58-64)zero_copy_at(90-93)zero_copy_at(99-111)zero_copy_at(139-149)zero_copy_at(341-344)zero_copy_at(399-403)zero_copy_at(456-461)zero_copy_at(538-553)zero_copy_at(594-597)zero_copy_at(631-634)zero_copy_at(709-713)zero_copy_at(768-772)
program-libs/zero-copy-derive/src/config.rs (2)
program-libs/zero-copy-derive/src/utils.rs (1)
get_vec_inner_type(85-98)program-libs/zero-copy/src/init_mut.rs (25)
config(237-240)new_zero_copy(28-31)new_zero_copy(53-69)new_zero_copy(81-87)new_zero_copy(98-104)new_zero_copy(115-121)new_zero_copy(132-138)new_zero_copy(149-155)new_zero_copy(172-178)new_zero_copy(190-195)new_zero_copy(206-211)new_zero_copy(222-227)new_zero_copy(243-263)byte_len(23-23)byte_len(42-51)byte_len(77-79)byte_len(94-96)byte_len(111-113)byte_len(128-130)byte_len(145-147)byte_len(168-170)byte_len(186-188)byte_len(202-204)byte_len(218-220)byte_len(235-241)
program-libs/zero-copy-derive/src/lib.rs (8)
program-libs/zero-copy-derive/src/from_impl.rs (4)
syn(97-97)generate_from_impl(9-182)generate_from_impl(208-208)generate_from_impl(240-240)program-libs/zero-copy-derive/src/z_struct.rs (4)
name(32-52)generate_z_struct(347-448)generate_z_struct(538-538)analyze_struct_fields(56-123)program-libs/zero-copy-derive/src/utils.rs (2)
process_input(16-40)process_fields(42-61)program-libs/zero-copy-derive/src/meta_struct.rs (1)
generate_meta_struct(9-56)program-libs/zero-copy-derive/src/zero_copy_struct_inner.rs (1)
generate_zero_copy_struct_inner(6-25)program-libs/zero-copy-derive/src/deserialize_impl.rs (1)
generate_deserialize_impl(155-214)program-libs/zero-copy-derive/src/partial_eq_impl.rs (1)
generate_partial_eq_impl(220-251)program-libs/zero-copy-derive/src/config.rs (3)
generate_config_struct(81-111)generate_field_initialization(114-257)generate_byte_len_calculation(260-368)
program-libs/zero-copy/src/borsh_mut.rs (2)
program-libs/zero-copy/src/borsh.rs (14)
new(117-119)deref(124-126)deref(333-335)deref(391-393)deref(448-450)deref(530-532)deref(701-703)deref_mut(130-132)test_vecu8(189-195)eq(379-385)eq(689-695)eq(776-778)test_struct_7(717-743)test_struct_8(797-830)program-libs/zero-copy/src/slice_mut.rs (6)
zero_copy_at_mut(286-290)new(35-37)eq(263-265)to_vec(178-180)from_bytes_at(61-85)iter(246-248)
program-libs/zero-copy-derive/tests/instruction_data.rs (10)
program-libs/zero-copy/src/borsh.rs (19)
zero_copy_at(20-20)zero_copy_at(27-30)zero_copy_at(36-49)zero_copy_at(58-64)zero_copy_at(90-93)zero_copy_at(99-111)zero_copy_at(139-149)zero_copy_at(341-344)zero_copy_at(399-403)zero_copy_at(456-461)zero_copy_at(538-553)zero_copy_at(594-597)zero_copy_at(631-634)zero_copy_at(709-713)zero_copy_at(768-772)zero_copy_at(790-793)eq(379-385)eq(689-695)eq(776-778)program-libs/zero-copy/src/borsh_mut.rs (19)
zero_copy_at_mut(20-21)zero_copy_at_mut(29-34)zero_copy_at_mut(41-56)zero_copy_at_mut(65-71)zero_copy_at_mut(79-84)zero_copy_at_mut(90-104)zero_copy_at_mut(129-135)zero_copy_at_mut(142-148)zero_copy_at_mut(155-161)zero_copy_at_mut(197-209)zero_copy_at_mut(398-403)zero_copy_at_mut(461-467)zero_copy_at_mut(520-527)zero_copy_at_mut(558-563)zero_copy_at_mut(616-635)zero_copy_at_mut(705-712)eq(441-447)eq(806-812)eq(898-900)program-libs/zero-copy/src/slice_mut.rs (2)
eq(263-265)len(116-119)program-libs/zero-copy/tests/borsh_2.rs (3)
eq(105-111)eq(418-424)eq(505-507)program-libs/zero-copy/src/init_mut.rs (25)
byte_len(23-23)byte_len(42-51)byte_len(77-79)byte_len(94-96)byte_len(111-113)byte_len(128-130)byte_len(145-147)byte_len(168-170)byte_len(186-188)byte_len(202-204)byte_len(218-220)byte_len(235-241)new_zero_copy(28-31)new_zero_copy(53-69)new_zero_copy(81-87)new_zero_copy(98-104)new_zero_copy(115-121)new_zero_copy(132-138)new_zero_copy(149-155)new_zero_copy(172-178)new_zero_copy(190-195)new_zero_copy(206-211)new_zero_copy(222-227)new_zero_copy(243-263)config(237-240)js/stateless.js/src/state/types.ts (2)
InstructionDataInvoke(277-306)InstructionDataInvokeCpi(311-345)js/stateless.js/src/state/compressed-account.ts (4)
PackedCompressedAccountWithMerkleContext(204-221)CompressedAccount(29-51)MerkleContext(80-97)CompressedAccountWithMerkleContext(14-17)js/stateless.js/src/utils/address.ts (2)
NewAddressParamsPacked(64-83)NewAddressParams(43-62)forester-utils/src/account_zero_copy.rs (1)
deserialized(32-34)programs/system/src/processor/sol_compression.rs (1)
decompress_lamports(50-76)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: stateless-js-v1
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v2
- GitHub Check: cli-v1
- GitHub Check: lint
🔇 Additional comments (26)
.github/workflows/rust.yml (1)
56-57: LGTM! Proper CI integration for the new derive functionality.The CI workflow correctly adds test coverage for both the updated
light-zero-copycrate with new features and the newlight-zero-copy-derivecrate. The feature flags align with the new capabilities being introduced.Cargo.toml (2)
16-16: LGTM! Proper workspace member addition.The new
zero-copy-derivecrate is correctly added to the workspace members list.
171-171: LGTM! Consistent workspace dependency configuration.The workspace dependency for
light-zero-copy-deriveis properly configured with version "0.1.0" and correct path reference.program-libs/zero-copy/Cargo.toml (3)
14-15: LGTM! Well-structured feature configuration.The optional features are properly configured:
derivefeature enables the procedural macro functionalitymutfeature enables mutable zero-copy operations through the derive crate
22-22: LGTM! Proper optional dependency setup.The
light-zero-copy-derivedependency is correctly configured as optional, allowing users to opt into derive functionality.
27-27: LGTM! Appropriate dev-dependency addition.Adding
borshas a dev-dependency is appropriate for testing zero-copy deserialization functionality.program-libs/zero-copy-derive/src/zero_copy_struct_inner.rs (1)
1-25: LGTM! Well-implemented code generation function.The implementation correctly:
- Uses const generic
MUTparameter for compile-time trait selection- Generates appropriate trait implementations for both mutable and immutable variants
- Properly handles lifetime parameters with
'static- Follows standard procedural macro patterns with
quote!andTokenStreamThe conditional logic cleanly separates the mutable and immutable trait implementations.
program-libs/zero-copy-derive/Cargo.toml (1)
1-26: LGTM! Proper procedural macro crate configuration.The manifest is well-configured:
- Correctly marked as
proc-macro = true- Dependencies include all necessary tools for procedural macros (
proc-macro2,quote,syn)synfeatures"full"and"extra-traits"are appropriate for complex struct parsing- Dev-dependencies properly reference workspace crates with required features
- Version 0.1.0 is appropriate for a new crate
- License and description are properly specified
The
mutfeature flag allows for optional mutable functionality.program-libs/zero-copy/src/slice_mut.rs (1)
280-291: LGTM! Clean trait implementation with proper feature gating.The
DeserializeMutimplementation correctly delegates to the existingfrom_bytes_atmethod and maintains consistency with the pattern used elsewhere in the codebase. Thestdfeature gate is appropriate for this functionality.program-libs/zero-copy-derive/src/meta_struct.rs (1)
49-56: Derive trait list is complete and correctThe generated
#[derive(...)]inprogram-libs/zero-copy-derive/src/meta_struct.rsmatches the baseline inzero-copy/src/borsh_mut.rsand includes all required zerocopy traits (KnownLayout, Immutable, Unaligned, FromBytes, IntoBytes) along withDebugandPartialEq(plus the optional#hasherwhen specified). No changes needed.program-libs/zero-copy-derive/tests/from_test.rs (2)
1-1: Good use of feature gating for mutable functionality.The
#[cfg(feature = "mut")]attribute correctly gates the entire test module for mutable zero-copy functionality.
9-13: Comprehensive derive macro usage for test structs.The struct derives all necessary traits for thorough testing of the zero-copy functionality including serialization, deserialization, and equality comparison.
program-libs/zero-copy-derive/src/byte_len_derive.rs (2)
10-38: Excellent documentation of implementation rules.The comprehensive documentation clearly explains the byte length calculation rules and exceptions, making the code maintainable and understandable.
52-57: Boolean handling logic is consistent with documentation.The special case for boolean fields using
size_of::<u8>()instead ofbyte_len()correctly implements the documented exception.program-libs/zero-copy-derive/README.md (2)
1-19: Clear and comprehensive crate documentation.The introduction and feature overview effectively explain the purpose and capabilities of the derive macros.
20-32: Well-structured rules documentation.The rules section clearly explains the zero-copy deserialization behavior, which will help users understand when and how to use the derive macros effectively.
program-libs/zero-copy/src/lib.rs (1)
21-26: Possibly over-restrictive feature gating
ZeroCopyMutis re-exported with#[cfg(all(feature = "derive", feature = "mut"))], not requiringstd, whereasZeroCopyConfigasks forall(feature = "derive", feature = "std", feature = "mut").
IfZeroCopyConfigdoes not actually depend onstd, consider dropping that gate; otherwise document clearly that the config derive is unavailable inno_std+mutbuilds.program-libs/zero-copy-derive/src/lib.rs (1)
274-282: Documentation promisesbyte_len(); implementation silently omits itThe docstring for
ZeroCopyMutlists automaticbyte_len()generation, but the macro passesquote! {}forbyte_len_impl, so the method is never emitted. Either generate the implementation (e.g. via the oldbyte_lenhelper) or adjust the docs.Likely an incorrect or invalid review comment.
program-libs/zero-copy/src/borsh_mut.rs (1)
200-208:VecU8length prefix diverges from Borsh (u8 vs u32)
VecU8::<T>::zero_copy_at_mutparses a single-byte length, yet Borsh encodes vectors with
a LEu32. Double-check that this is intentional; otherwise data produced by standard
try_to_vec()will not round-trip.program-libs/zero-copy-derive/tests/config_test.rs (2)
1-6: LGTM - Proper feature gating and importsThe feature gating with
#![cfg(feature = "mut")]appropriately restricts this test to when mutable zero-copy functionality is enabled. The imports are well-organized and include all necessary traits and derives.
335-337: Ignore padding concerns – serialized format is contiguous with no alignment
TheByteLenimplementation for this struct uses a custom, packed serialization (no padding), summing each field’s byte length:
a: u8→ 1 bytevec: Vec<u8>→ 4-byte length prefix + 10 elements × 1 byte = 14 bytesb: u16→ 2 bytes
Total = 17 bytes, matching the derivedbyte_len. No changes required.Likely an incorrect or invalid review comment.
program-libs/zero-copy/tests/borsh.rs (5)
1-6: Comprehensive feature gating ensures proper test isolationThe multiple feature requirements (
std,derive,mut) properly ensure these tests only run when all necessary functionality is available. The imports are well-organized.
27-30: Mutable zero-copy modification pattern demonstrates key functionalityThe test correctly demonstrates that mutable zero-copy views can modify the underlying byte buffer, and these changes persist when deserializing again. The pattern of accessing the mutable view, modifying fields, then re-deserializing validates the core zero-copy mutable functionality.
209-235: Excellent test coverage for Option field edge casesThis test properly validates both
SomeandNonevariants of Option fields, ensuring the zero-copy deserialization handles both cases correctly. The pattern of testing both states in a single test function is thorough.
286-301: Array struct test validates fixed-size array zero-copy accessThe test correctly validates that fixed-size arrays can be zero-copy deserialized and accessed directly without additional allocation. The equality check confirms the zero-copy view matches the original data exactly.
331-331: Remove redundant commented assertion inborsh.rs
The line// assert_eq!(zero_copy.data, compressed_account_data.data.as_slice());can be safely removed. The subsequent
assert_eq!(zero_copy, compressed_account_data);already verifies the
datafield (alongside all other fields) via thePartialEqimplementation.
| #[test] | ||
| fn test_simple_from_implementation() { | ||
| // Create an instance of our struct | ||
| let original = SimpleStruct { | ||
| a: 42, | ||
| b: vec![1, 2, 3, 4, 5], | ||
| }; | ||
|
|
||
| // Serialize it | ||
| let bytes = original.try_to_vec().unwrap(); | ||
| // byte_len not available for non-mut derivations | ||
| // assert_eq!(bytes.len(), original.byte_len()); | ||
|
|
||
| // Test From implementation for immutable struct | ||
| let (zero_copy, _) = SimpleStruct::zero_copy_at(&bytes).unwrap(); | ||
| let converted: SimpleStruct = zero_copy.into(); | ||
| assert_eq!(converted.a, 42); | ||
| assert_eq!(converted.b, vec![1, 2, 3, 4, 5]); | ||
| assert_eq!(converted, original); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error case testing for robustness.
The tests currently only cover the happy path. Consider adding tests for invalid byte sequences or malformed data to ensure proper error handling.
#[test]
fn test_simple_from_implementation_invalid_data() {
// Test with insufficient bytes
let bytes = vec![1, 2]; // Too short for valid serialization
let result = SimpleStruct::zero_copy_at(&bytes);
assert!(result.is_err());
// Test with corrupted length prefix for vector
let original = SimpleStruct { a: 42, b: vec![1, 2, 3] };
let mut bytes = original.try_to_vec().unwrap();
// Corrupt the vector length (assuming it starts after the first byte)
if bytes.len() > 4 {
bytes[1] = 255; // Set unrealistic vector length
let result = SimpleStruct::zero_copy_at(&bytes);
assert!(result.is_err());
}
}🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/tests/from_test.rs around lines 27 to 46, the
test only covers the successful case of converting bytes to the struct. Add a
new test function that checks error handling by passing invalid byte sequences,
such as too short byte arrays and corrupted serialized data, to
SimpleStruct::zero_copy_at. Verify that these cases return errors using
assertions to ensure robustness against malformed input.
| ```rust | ||
| use borsh::{BorshDeserialize, BorshSerialize}; | ||
| use light_zero_copy_derive::ZeroCopy; | ||
| use light_zero_copy::{borsh::Deserialize, borsh_mut::DeserializeMut}; | ||
|
|
||
| #[repr(C)] | ||
| #[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy)] | ||
| pub struct MyStruct { | ||
| pub a: u8, | ||
| pub b: u16, | ||
| pub vec: Vec<u8>, | ||
| pub c: u64, | ||
| } | ||
| let my_struct = MyStruct { | ||
| a: 1, | ||
| b: 2, | ||
| vec: vec![1u8; 32], | ||
| c: 3, | ||
| }; | ||
| // Use the struct with zero-copy deserialization | ||
| let mut bytes = my_struct.try_to_vec().unwrap(); | ||
|
|
||
| // Immutable zero-copy deserialization | ||
| let (zero_copy, _remaining) = MyStruct::zero_copy_at(&bytes).unwrap(); | ||
|
|
||
| // Convert back to original struct using From implementation | ||
| let converted: MyStruct = zero_copy.clone().into(); | ||
| assert_eq!(converted, my_struct); | ||
|
|
||
| // Mutable zero-copy deserialization with modification | ||
| let (mut zero_copy_mut, _remaining) = MyStruct::zero_copy_at_mut(&mut bytes).unwrap(); | ||
| zero_copy_mut.a = 42; | ||
|
|
||
| // The change is reflected when we convert back to the original struct | ||
| let modified: MyStruct = zero_copy_mut.into(); | ||
| assert_eq!(modified.a, 42); | ||
|
|
||
| // And also when we deserialize directly from the modified bytes | ||
| let borsh = MyStruct::try_from_slice(&bytes).unwrap(); | ||
| assert_eq!(borsh.a, 42u8); | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the code examples compile and run correctly.
The usage examples look comprehensive, but it's important to ensure they actually compile and demonstrate the intended functionality.
🏁 Script executed:
#!/bin/bash
# Create a temporary test file to verify the README examples compile
cat > /tmp/readme_test.rs << 'EOF'
use borsh::{BorshDeserialize, BorshSerialize};
use light_zero_copy_derive::ZeroCopy;
use light_zero_copy::{borsh::Deserialize, borsh_mut::DeserializeMut};
#[repr(C)]
#[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy)]
pub struct MyStruct {
pub a: u8,
pub b: u16,
pub vec: Vec<u8>,
pub c: u64,
}
fn main() {
let my_struct = MyStruct {
a: 1,
b: 2,
vec: vec![1u8; 32],
c: 3,
};
let mut bytes = my_struct.try_to_vec().unwrap();
let (zero_copy, _remaining) = MyStruct::zero_copy_at(&bytes).unwrap();
let converted: MyStruct = zero_copy.clone().into();
assert_eq!(converted, my_struct);
}
EOF
# Try to check if the syntax is valid (this is a basic check)
rustc --edition 2021 --crate-type bin /tmp/readme_test.rs --extern borsh --extern light_zero_copy_derive --extern light_zero_copy 2>&1 || echo "Example may need syntax adjustments"Length of output: 2546
Use Cargo to compile and run the zero-copy examples
The Rust snippets in your README won’t compile with a plain rustc invocation because the external crates and Borsh traits aren’t in scope. To verify both the immutable and mutable zero-copy flows:
• Add the required dependencies to your Cargo.toml (adjust versions/paths as needed):
+# Cargo.toml
+[dependencies]
+borsh = "0.10"
+light_zero_copy = { path = "program-libs/zero-copy" }
+light_zero_copy_derive = { path = "program-libs/zero-copy-derive" }• Move the code sample into an example file at examples/zero_copy.rs (wrap it in fn main() { … }).
• Run the example and doctests:
cargo run --example zero_copy
cargo test --docThis will pull in the Borsh traits (try_to_vec), the derive macros, and ensure that zero_copy_at / zero_copy_at_mut compile, execute, and round-trip correctly.
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/README.md between lines 37 and 77, the Rust
code snippet is not directly runnable because it lacks the necessary Cargo.toml
dependencies and is not wrapped in a main function. To fix this, add the
required external crate dependencies (borsh, light_zero_copy_derive,
light_zero_copy) to Cargo.toml with appropriate versions, move the code into a
new example file at examples/zero_copy.rs, wrap the code inside a fn main()
function, and then run it using cargo run --example zero_copy and cargo test
--doc to verify compilation and execution.
| fn extract_inner(s: &str) -> Option<&str> { | ||
| s.strip_prefix("Option <")?.strip_suffix(">") | ||
| } | ||
| use quote::ToTokens; | ||
| let string = field_type.to_token_stream().to_string(); | ||
| println!("option string {}", string); | ||
| let cleaned_type = extract_inner(&string).unwrap(); | ||
| let field_type = syn::parse_str::<Type>(cleaned_type).unwrap(); | ||
| // For Option types, use a direct copy of the value when possible |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fragile Option handling may panic and breaks on formatting changes
• Relies on stringification & manual trimming (strip_prefix("Option <")) to get the inner type – this fails if syn prints without the exact spaces (e.g. "Option <" vs "Option<").
• Multiple unwrap()s will panic at compile-time for perfectly valid input that happens not to match the hard-coded pattern.
Use syn::Type::Path inspection (or the existing utils::get_option_inner_type) instead of string slicing.
Example sketch:
if let Some(inner) = utils::get_option_inner_type(&field_type) {
quote! { #field_name: value.#field_name.as_ref().map(|v| (*v).clone().into()), }
}🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/src/from_impl.rs around lines 90 to 98, the
code extracts the inner type of an Option by converting the type to a string and
manually trimming it, which is fragile and can panic due to formatting
differences or unwrap calls. Replace this string-based extraction with proper
syn::Type::Path inspection or use the existing utils::get_option_inner_type
function to safely and reliably get the inner type without panics. Adjust the
code to handle the Option type by checking if the inner type exists via
utils::get_option_inner_type and then generate the appropriate quote code
accordingly.
| FieldType::VecNonCopy(_, vec_type) => { | ||
| if let Some(inner_type) = utils::get_vec_inner_type(vec_type) { | ||
| quote! { Vec<<#inner_type as light_zero_copy::init_mut::ZeroCopyNew<'static>>::Config> } | ||
| } else { | ||
| panic!("Could not determine inner type for VecNonCopy config"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
panic! during code-gen → hard build failure
Falling back to panic!("Could not determine…") aborts the entire compile if a user accidentally writes an unsupported Vec type.
Emit a compile_error! with a helpful message instead so the error points to the user’s code, e.g.:
quote! { compile_error!("light-zero-copy: unsupported Vec inner type"); }🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/src/config.rs around lines 44 to 50, replace
the panic! call inside the FieldType::VecNonCopy match arm with a quote! macro
that emits a compile_error! macro containing a helpful error message. This
change will cause the compiler to produce a user-facing error pointing to the
problematic code instead of aborting the entire build.
| FieldType::OptionU64(field_name) => { | ||
| quote! { | ||
| let (#field_name, bytes) = <Option<u64> as light_zero_copy::init_mut::ZeroCopyNew>::new_zero_copy( | ||
| bytes, | ||
| (config.#field_name, ()) | ||
| )?; | ||
| } | ||
| } | ||
|
|
||
| FieldType::OptionU32(field_name) => { | ||
| quote! { | ||
| let (#field_name, bytes) = <Option<u32> as light_zero_copy::init_mut::ZeroCopyNew>::new_zero_copy( | ||
| bytes, | ||
| (config.#field_name, ()) | ||
| )?; | ||
| } | ||
| } | ||
|
|
||
| FieldType::OptionU16(field_name) => { | ||
| quote! { | ||
| let (#field_name, bytes) = <Option<u16> as light_zero_copy::init_mut::ZeroCopyNew>::new_zero_copy( | ||
| bytes, | ||
| (config.#field_name, ()) | ||
| )?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing lifetime parameter breaks compilation
ZeroCopyNew is generic over a lifetime: ZeroCopyNew<'a>.
Calls for Option<u64/u32/u16> omit it:
<Option<u64> as light_zero_copy::init_mut::ZeroCopyNew>::new_zero_copy(...)Rust will error: wrong number of lifetime arguments supplied.
Add the lifetime:
<Option<u64> as light_zero_copy::init_mut::ZeroCopyNew<'a>>::new_zero_copy(...)Update the corresponding byte_len invocations as well.
Also applies to: 231-236, 239-246
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/src/config.rs between lines 221 and 246, the
trait ZeroCopyNew is used without specifying its required lifetime parameter,
causing compilation errors. Fix this by adding the lifetime parameter 'a to all
ZeroCopyNew trait usages for Option<u64>, Option<u32>, and Option<u16> in both
new_zero_copy and byte_len calls, changing them to ZeroCopyNew<'a>. Ensure all
such invocations in the specified line ranges are updated accordingly.
| let (num_slices, mut bytes) = Ref::<&mut [u8], U32>::from_prefix(bytes)?; | ||
| let num_slices = u32::from(*num_slices) as usize; | ||
| // TODO: add check that remaining data is enough to read num_slices | ||
| // This prevents agains invalid data allocating a lot of heap memory | ||
| let mut slices = Vec::with_capacity(num_slices); | ||
| for _ in 0..num_slices { | ||
| let (slice, _bytes) = T::zero_copy_at_mut(bytes)?; | ||
| bytes = _bytes; | ||
| slices.push(slice); | ||
| } | ||
| Ok((slices, bytes)) |
There was a problem hiding this comment.
Unchecked length allows DoS-sized allocations
Vec::<T>::zero_copy_at_mut reads num_slices but never verifies that the buffer actually
contains num_slices elements before allocating. A crafted length can allocate huge
memory and still fail later.
Add an upfront bound check:
- let mut slices = Vec::with_capacity(num_slices);
- for _ in 0..num_slices {
+ if bytes.len() < num_slices * core::mem::size_of::<T>() {
+ return Err(ZeroCopyError::InsufficientMemoryAllocated(
+ bytes.len() + 4,
+ num_slices * core::mem::size_of::<T>() + 4,
+ ));
+ }
+ let mut slices = Vec::with_capacity(num_slices);
+ for _ in 0..num_slices {📝 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 (num_slices, mut bytes) = Ref::<&mut [u8], U32>::from_prefix(bytes)?; | |
| let num_slices = u32::from(*num_slices) as usize; | |
| // TODO: add check that remaining data is enough to read num_slices | |
| // This prevents agains invalid data allocating a lot of heap memory | |
| let mut slices = Vec::with_capacity(num_slices); | |
| for _ in 0..num_slices { | |
| let (slice, _bytes) = T::zero_copy_at_mut(bytes)?; | |
| bytes = _bytes; | |
| slices.push(slice); | |
| } | |
| Ok((slices, bytes)) | |
| let (num_slices, mut bytes) = Ref::<&mut [u8], U32>::from_prefix(bytes)?; | |
| let num_slices = u32::from(*num_slices) as usize; | |
| // TODO: add check that remaining data is enough to read num_slices | |
| // This prevents agains invalid data allocating a lot of heap memory | |
| if bytes.len() < num_slices * core::mem::size_of::<T>() { | |
| return Err(ZeroCopyError::InsufficientMemoryAllocated( | |
| bytes.len() + 4, | |
| num_slices * core::mem::size_of::<T>() + 4, | |
| )); | |
| } | |
| let mut slices = Vec::with_capacity(num_slices); | |
| for _ in 0..num_slices { | |
| let (slice, _bytes) = T::zero_copy_at_mut(bytes)?; | |
| bytes = _bytes; | |
| slices.push(slice); | |
| } | |
| Ok((slices, bytes)) |
🤖 Prompt for AI Agents
In program-libs/zero-copy/src/borsh_mut.rs around lines 93 to 103, the code
reads num_slices from the input but does not verify if the remaining buffer is
large enough to hold num_slices elements before allocating the vector. To fix
this, add a check after reading num_slices to ensure the remaining bytes length
is sufficient to contain all slices, preventing excessive memory allocation from
malicious input. If the check fails, return an error early to avoid potential
DoS.
| impl PartialEq<CompressedAccount> for ZCompressedAccount<'_> { | ||
| fn eq(&self, other: &CompressedAccount) -> bool { | ||
| if self.address.is_some() | ||
| && other.address.is_some() | ||
| && *self.address.unwrap() != other.address.unwrap() | ||
| { | ||
| return false; | ||
| } | ||
| if self.address.is_some() || other.address.is_some() { | ||
| return false; | ||
| } | ||
| if self.data.is_some() | ||
| && other.data.is_some() | ||
| && self.data.as_ref().unwrap() != other.data.as_ref().unwrap() | ||
| { | ||
| return false; | ||
| } | ||
| if self.data.is_some() || other.data.is_some() { | ||
| return false; | ||
| } | ||
|
|
||
| self.owner == other.owner && self.lamports == other.lamports | ||
| } | ||
| } |
There was a problem hiding this comment.
PartialEq always returns false when optional fields are Some
The pattern
if self.address.is_some() && other.address.is_some() && *... != ... { return false }
if self.address.is_some() || other.address.is_some() { return false }makes the comparison fail even when both Some hold identical values (second condition
fires). Remove the second guard or rewrite with a match:
- if self.address.is_some() || other.address.is_some() {
- return false;
- }
+ match (&self.address, &other.address) {
+ (Some(a), Some(b)) if a != b => return false,
+ (None, None) => {}
+ (Some(_), None) | (None, Some(_)) => return false,
+ }The same issue is repeated for the data checks and in the symmetric impls below.
Also applies to: 702-724
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/tests/instruction_data.rs around lines 651 to
674, the PartialEq implementation incorrectly returns false when both optional
fields are Some with identical values due to the second condition that returns
false if either is Some. Remove the second condition that checks for one being
Some and the other None, and instead use a match or direct comparison that
correctly handles the cases where both are Some or both are None. Apply the same
fix to the data field checks and also to the symmetric implementations around
lines 702 to 724.
Summary by CodeRabbit
New Features
Enhancements
Documentation
Chores