feat: zero copy macro #1851
feat: zero copy macro #1851
Conversation
WalkthroughA new procedural macro crate, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Struct
participant Macro (ZeroCopy/Mut/Eq)
participant ByteBuffer
participant ZeroCopyRef
User->>Struct: #[derive(ZeroCopy, ZeroCopyMut, ZeroCopyEq)]
Macro->>Struct: Analyze fields/types
Macro->>Struct: Generate meta struct (Copy fields)
Macro->>Struct: Generate zero-copy struct (wrappers for Vec/Option)
Macro->>Struct: Implement Deserialize/DeserializeMut
User->>ByteBuffer: Serialize Struct (e.g., with Borsh)
User->>ZeroCopyRef: ZeroCopyStruct::zero_copy_at(&bytes)
ZeroCopyRef-->>User: Zero-copy reference to struct fields
User->>ZeroCopyRef: Mutate via ZeroCopyMut (if mutable)
User->>Struct: Convert ZeroCopyRef into owned Struct (via From)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (19)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
♻️ Duplicate comments (1)
program-libs/zero-copy/src/borsh.rs (1)
125-126: Security: Add validation to prevent memory exhaustionThis TODO identifies the same DoS vector as in borsh_mut.rs - malicious input could cause excessive memory allocation.
🧹 Nitpick comments (17)
.gitignore (1)
88-91: Consider making the ignore pattern recursive
expand.rswill be ignored only at the repository root.
If the macro expansion ever produces the same file name inside a nested crate (e.g.target/expand.rsorprogram-libs/zero-copy-derive/expand.rs), it will still be tracked.- expand.rs + **/expand.rsA glob keeps the intent while covering future placements at no extra cost.
.github/workflows/rust.yml (1)
56-58: Use--all-featuresor drop the spaces after commas for cleaner CLICargo accepts both comma- and space-separated feature lists, but the current mix (
"std, mut, derive") yields tokens with leading whitespace that must be trimmed internally. Two cleaner options:- cargo test -p light-zero-copy --features "std, mut, derive" + cargo test -p light-zero-copy --all-featuresor
- cargo test -p light-zero-copy --features "std, mut, derive" + cargo test -p light-zero-copy --features "std,mut,derive"Either improves readability and sidesteps any future string-processing surprises.
program-libs/zero-copy/Cargo.toml (1)
14-16: Feature namemutmay invite tooling frictionA bare feature called
mutis perfectly legal in Cargo, yet:
- It collides with the Rust keyword in logs and search queries.
- Shell auto-completion and some editors colour-highlight it oddly.
- Quoting is required whenever it is referenced from bash‐style scripts (
cargo build --features "mut").Renaming to something explicit such as
mut-zcorzero-copy-mutavoids these annoyances while remaining semver-compatible via thealiasesmechanism if necessary.No blocker—just a polish suggestion.
program-libs/zero-copy-derive/Cargo.toml (1)
27-27: Remove trailing empty line-zerocopy = { workspace = true, features = ["derive"] } - +zerocopy = { workspace = true, features = ["derive"] }program-libs/zero-copy-derive/src/shared/meta_struct.rs (1)
21-34: Simplify attribute handling logicThe current logic creates an unnecessary empty vector when
hasheris false. Consider simplifying this.let meta_fields_with_converted_types = meta_fields.iter().map(|field| { let field_name = &field.ident; - let attributes = if hasher { - field - .attrs - .iter() - .map(|attr| { - quote! { #attr } - }) - .collect::<Vec<_>>() - } else { - vec![quote! {}] - }; + let attributes = if hasher { + field.attrs.iter().map(|attr| quote! { #attr }).collect::<Vec<_>>() + } else { + vec![] + }; let field_type = convert_to_zerocopy_type(&field.ty); quote! { #(#attributes)* pub #field_name: #field_type } });program-libs/zero-copy-derive/tests/from_test.rs (1)
2-2: Remove unnecessary Vec import.
Vecis part of Rust's prelude and doesn't need to be explicitly imported.-use std::vec::Vec; -program-libs/zero-copy-derive/src/zero_copy_eq.rs (1)
109-120: Address the TODO for proper Option field comparison.The TODO indicates that different field types require different dereference patterns for comparison. This incomplete implementation could cause compilation errors.
This TODO suggests that the Option comparison logic needs type-specific handling. Would you like me to implement a proper solution that handles different dereference patterns based on the field type?
program-libs/zero-copy-derive/src/shared/z_struct.rs (1)
295-300: Remove commented-out code.This commented-out Bool field type handling appears to be obsolete since bool is now handled as a Primitive type. Consider removing it to keep the code clean.
- // FieldType::Bool(field_name) => { - // quote! { - // #(#attributes)* - // pub #field_name: <bool as #trait_name<'a>>::Output - // } - // }program-libs/zero-copy-derive/src/zero_copy.rs (1)
197-564: Remove or restore commented test codeThis large block of commented test code should either be removed if obsolete or uncommented if still needed.
Consider moving these tests to a separate test file if they're still valuable, or remove them entirely to improve code maintainability.
program-libs/zero-copy/src/borsh_mut.rs (1)
18-19: Consider the TODO: Rename Output typeThe TODO suggests renaming
OutputtoZeroCopyfor better clarity. This would be a breaking change but might improve API consistency.Would you like me to help create a migration plan for this rename across the codebase?
program-libs/zero-copy/src/borsh.rs (2)
350-352: Remove incomplete commented codeThis appears to be an incomplete function stub that should be removed or completed.
Either implement the hash function or remove these lines to keep the code clean.
311-867: Consider moving test module to separate fileThis 550+ line test module makes the main source file quite large.
Consider moving these tests to
tests/borsh_manual.rsor similar to improve maintainability and reduce compilation time for the main module.program-libs/zero-copy-derive/src/shared/utils.rs (1)
28-29: Document the cache population side effect.The comment mentions populating the cache, but it's not clear why this is done here rather than when actually needed. Consider clarifying the purpose.
- // Populate the cache by checking if this struct implements Copy + // Populate the cache early to ensure consistent results when this struct is referenced by other structs let _ = struct_implements_copy(input);program-libs/zero-copy/tests/borsh_2.rs (1)
40-42: Remove commented-out code.The commented function stub doesn't provide value and should be removed.
-// pub fn data_hash_struct_1(a: u8, b: u16) -> [u8; 32] { - -// } -program-libs/zero-copy-derive/tests/random.rs (2)
41-174: Consider extracting common population logic.The
populate_invoke_cpi_zero_copyandpopulate_invoke_zero_copyfunctions share significant duplicate code. Consider extracting the common logic into helper functions.Example of extracting common proof population:
fn populate_proof(src: &Option<CompressedProof>, dst: &mut Option<ZCompressedProofMut>) { if let (Some(src_proof), Some(dst_proof)) = (src, dst) { dst_proof.a.copy_from_slice(&src_proof.a); dst_proof.b.copy_from_slice(&src_proof.b); dst_proof.c.copy_from_slice(&src_proof.c); } }
514-514: Consider consistent iteration counts or document the difference.The first test uses 1,000 iterations while the second uses 10,000. If the difference is intentional (e.g., due to complexity), consider adding a comment explaining why.
- let num_iters = 1000; + let num_iters = 1000; // Lower iterations due to selective input/output generationAlso applies to: 605-605
program-libs/zero-copy-derive/tests/instruction_data.rs (1)
27-32: Thread safety concern in new_unique().Using
thread_rng()creates a new RNG instance each time. For better performance in multi-threaded contexts, consider using a thread-local RNG.pub fn new_unique() -> Self { use rand::{thread_rng, Rng}; let mut rng = thread_rng(); let bytes = rng.gen::<[u8; 32]>(); Pubkey(bytes) }
📜 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 (30)
.github/workflows/rust.yml(1 hunks).gitignore(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/lib.rs(1 hunks)program-libs/zero-copy-derive/src/shared/from_impl.rs(1 hunks)program-libs/zero-copy-derive/src/shared/meta_struct.rs(1 hunks)program-libs/zero-copy-derive/src/shared/mod.rs(1 hunks)program-libs/zero-copy-derive/src/shared/utils.rs(1 hunks)program-libs/zero-copy-derive/src/shared/z_struct.rs(1 hunks)program-libs/zero-copy-derive/src/shared/zero_copy_new.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_eq.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_mut.rs(1 hunks)program-libs/zero-copy-derive/test_debug.rs(1 hunks)program-libs/zero-copy-derive/tests/config_test.rs(1 hunks)program-libs/zero-copy-derive/tests/cross_crate_copy.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(4 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 (8)
program-libs/zero-copy-derive/src/shared/mod.rs (3)
program-libs/zero-copy-derive/src/zero_copy_eq.rs (1)
from_impl(259-259)program-libs/zero-copy-derive/src/zero_copy.rs (1)
meta_struct(602-602)program-libs/zero-copy-derive/src/zero_copy_mut.rs (2)
meta_struct(26-26)z_struct(31-31)
program-libs/zero-copy-derive/src/shared/meta_struct.rs (2)
program-libs/zero-copy-derive/src/shared/utils.rs (1)
convert_to_zerocopy_type(170-211)program-libs/zero-copy-derive/src/shared/z_struct.rs (1)
field(180-186)
program-libs/zero-copy-derive/tests/from_test.rs (1)
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(56-62)zero_copy_at(69-75)zero_copy_at(113-116)zero_copy_at(122-134)zero_copy_at(162-172)zero_copy_at(377-380)zero_copy_at(435-439)zero_copy_at(492-497)zero_copy_at(574-589)zero_copy_at(630-633)zero_copy_at(667-670)zero_copy_at(745-749)
program-libs/zero-copy/src/init_mut.rs (5)
program-libs/zero-copy/src/lib.rs (1)
size_of(36-36)program-libs/zero-copy/src/borsh.rs (6)
size_of(37-37)size_of(57-57)size_of(60-60)size_of(70-70)size_of(73-73)new(140-142)program-libs/zero-copy/src/borsh_mut.rs (17)
size_of(44-44)zero_copy_at_mut(20-21)zero_copy_at_mut(29-34)zero_copy_at_mut(41-56)zero_copy_at_mut(63-67)zero_copy_at_mut(74-78)zero_copy_at_mut(86-91)zero_copy_at_mut(97-111)zero_copy_at_mut(180-192)zero_copy_at_mut(399-404)zero_copy_at_mut(462-470)zero_copy_at_mut(523-530)zero_copy_at_mut(561-566)zero_copy_at_mut(619-638)zero_copy_at_mut(708-715)zero_copy_at_mut(749-754)new(158-160)program-libs/zero-copy/src/slice_mut.rs (4)
size_of(97-97)size_of(107-107)len(116-119)new(35-37)program-libs/zero-copy-derive/tests/instruction_data.rs (3)
byte_len(72-74)new_zero_copy(76-81)zero_copy_at_mut(52-56)
program-libs/zero-copy-derive/tests/config_test.rs (2)
program-libs/zero-copy/src/init_mut.rs (25)
config(238-241)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(146-148)byte_len(169-171)byte_len(187-189)byte_len(203-205)byte_len(219-221)byte_len(236-242)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(150-156)new_zero_copy(173-179)new_zero_copy(191-196)new_zero_copy(207-212)new_zero_copy(223-228)new_zero_copy(244-264)program-libs/zero-copy/src/borsh_mut.rs (15)
zero_copy_at_mut(20-21)zero_copy_at_mut(29-34)zero_copy_at_mut(41-56)zero_copy_at_mut(63-67)zero_copy_at_mut(74-78)zero_copy_at_mut(86-91)zero_copy_at_mut(97-111)zero_copy_at_mut(180-192)zero_copy_at_mut(399-404)zero_copy_at_mut(462-470)zero_copy_at_mut(523-530)zero_copy_at_mut(561-566)zero_copy_at_mut(619-638)zero_copy_at_mut(708-715)zero_copy_at_mut(749-754)
program-libs/zero-copy/src/borsh_mut.rs (3)
program-libs/zero-copy/src/borsh.rs (17)
new(140-142)deref(147-149)deref(369-371)deref(427-429)deref(484-486)deref(566-568)deref(737-739)deref_mut(153-155)test_vecu8(225-231)test_vecu8_deref(293-301)test_struct_1(384-390)eq(415-421)eq(725-731)eq(812-814)test_struct_2(443-456)test_struct_3(501-516)test_struct_7(753-779)program-libs/zero-copy/src/slice_mut.rs (6)
new(35-37)eq(263-265)len(116-119)from_bytes(56-58)to_vec(178-180)from_bytes_at(61-85)program-libs/zero-copy-derive/src/zero_copy_mut.rs (2)
zero_copy(39-39)zero_copy(44-44)
program-libs/zero-copy-derive/tests/random.rs (4)
js/stateless.js/src/state/types.ts (6)
CompressedAccountData(199-212)CompressedCpiContext(353-368)CompressedProof(373-386)InstructionDataInvoke(277-306)InstructionDataInvokeCpi(311-345)OutputCompressedAccountWithPackedContext(170-173)program-libs/zero-copy-derive/tests/instruction_data.rs (6)
rng(30-30)new_unique(27-32)default(342-348)byte_len(72-74)new_zero_copy(76-81)zero_copy_at(43-45)program-libs/zero-copy/src/init_mut.rs (25)
config(238-241)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(146-148)byte_len(169-171)byte_len(187-189)byte_len(203-205)byte_len(219-221)byte_len(236-242)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(150-156)new_zero_copy(173-179)new_zero_copy(191-196)new_zero_copy(207-212)new_zero_copy(223-228)new_zero_copy(244-264)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(56-62)zero_copy_at(69-75)zero_copy_at(113-116)zero_copy_at(122-134)zero_copy_at(162-172)zero_copy_at(377-380)zero_copy_at(435-439)zero_copy_at(492-497)zero_copy_at(574-589)zero_copy_at(630-633)zero_copy_at(667-670)zero_copy_at(745-749)
program-libs/zero-copy/tests/borsh_2.rs (3)
program-libs/zero-copy/src/borsh.rs (3)
eq(415-421)eq(725-731)eq(812-814)program-libs/zero-copy/src/borsh_mut.rs (3)
eq(442-448)eq(809-815)eq(901-903)program-libs/zero-copy/src/slice_mut.rs (4)
eq(263-265)to_vec(178-180)from_bytes_at(61-85)iter(246-248)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: lint
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v1
- GitHub Check: Test program-libs-fast
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs-slow
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v2
🔇 Additional comments (22)
Cargo.toml (2)
16-17: Workspace member addition looks good
program-libs/zero-copy-deriveis correctly registered as a new workspace member.
No further action required.
171-172: Dependency wire-up verified
light-zero-copy-deriveis added as a workspace dependency; path and version align with the member entry above.
Looks correct.program-libs/zero-copy-derive/src/shared/mod.rs (1)
1-6: Module skeleton is straightforward and correctThe shared hub cleanly exposes the sub-modules and gates
zero_copy_newbehind themutfeature. Compiles fine as-is.program-libs/zero-copy/Cargo.toml (2)
22-23: Optional dependency correctly marked
light-zero-copy-deriveis set asoptional = true, ensuring consumers don’t pull in the proc-macro crate unless they opt intoderive/mut.
Looks good.
27-28: Dev-dependency addition is harmlessAdding
borshfor test coverage matches the new derive tests. No issues spotted.program-libs/zero-copy/src/slice_mut.rs (1)
280-291: LGTM!The
DeserializeMuttrait implementation is correctly feature-gated and properly delegates to the existingfrom_bytes_atmethod. The trait bounds ensure type safety for mutable zero-copy deserialization.program-libs/zero-copy-derive/tests/from_test.rs (1)
35-38: Clarify the byte_len availability issue.The comments state "byte_len not available for non-mut derivations", but this file is compiled with
cfg(feature = "mut"). Either:
- The
byte_lenmethod should be available and these assertions should work, or- There's another condition preventing
byte_lenfrom being available that should be documentedPlease either uncomment and fix these assertions or remove the commented code entirely.
Also applies to: 59-62
program-libs/zero-copy/src/lib.rs (1)
13-30: LGTM! Clean module organization and proper feature gating.The new modules and re-exports are well-organized and correctly feature-gated. The separation between
std-only features and derive-specific features is clear and follows Rust best practices.program-libs/zero-copy-derive/tests/cross_crate_copy.rs (1)
1-51: Well-structured test cases for Copy trait identification.The test structs effectively cover different field ordering scenarios and type combinations to validate the zero-copy derive macro's field classification logic.
program-libs/zero-copy/src/init_mut.rs (2)
6-32: Well-designed trait for configuration-based initialization.The
ZeroCopyNewtrait provides a clean and intuitive interface for initializing zero-copy structures with configuration parameters. The separation ofbyte_lencalculation andnew_zero_copyinitialization is a good design choice.
231-265: Correct Vec implementation with proper length encoding.The implementation properly handles Vec serialization with a 4-byte length prefix followed by elements. The use of individual configs per element provides fine-grained control over initialization.
program-libs/zero-copy-derive/src/shared/z_struct.rs (2)
7-151: Comprehensive field classification system.The
FieldTypeenum and classification functions provide excellent coverage of different field types with appropriate special handling for common patterns likeVec<u8>and integer options. The recursive type analysis is well-structured.
513-628: Excellent fuzz testing coverage.The fuzz test with 10,000 iterations provides strong confidence in the code generation logic. The assertions validate key aspects of the generated structs including proper derives, Deref implementations, and field presence.
program-libs/zero-copy-derive/src/shared/zero_copy_new.rs (1)
102-103: Stale TODO comment – no action neededI couldn’t find a
TODOat lines 102–103 (or elsewhere) inprogram-libs/zero-copy-derive/src/shared/zero_copy_new.rs. It looks like that comment has already been removed. No changes are required to address it.Likely an incorrect or invalid review comment.
program-libs/zero-copy/tests/borsh.rs (1)
1-336: Well-structured test coverageThe test suite provides comprehensive coverage for zero-copy deserialization functionality, including:
- Simple and complex struct scenarios
- Mutable operations verification
- Nested structures and vectors
- Array handling
- Edge cases with options
program-libs/zero-copy/src/borsh.rs (1)
65-76: Verify bool deserialization returns u8The bool type deserializes to u8 rather than bool, which might be unexpected for users.
This design choice should be documented, as it differs from typical bool deserialization expectations. Is this intentional for zero-copy performance?
program-libs/zero-copy-derive/src/shared/utils.rs (2)
300-304: Good handling of array types for Copy trait detection.The recursive check for array element types correctly implements Rust's rule that arrays are Copy if their element type is Copy.
323-325: Good test isolation with cache clearing.Clearing the cache at the start of tests ensures proper test isolation and prevents interference between tests.
program-libs/zero-copy/tests/borsh_2.rs (2)
104-112: Correct endianness handling in PartialEq implementation.The comparison correctly converts the little-endian
U16value to native endianness before comparing with the u16 field.
73-80: Well-structured zero-copy deserialization tests.The tests consistently verify:
- Successful deserialization
- Correct field values including endianness conversion
- All bytes are consumed (remaining slice is empty)
This provides good coverage of the zero-copy deserialization functionality.
Also applies to: 132-146, 190-206
program-libs/zero-copy-derive/tests/random.rs (1)
505-593: Good use of seed-based randomization for reproducible tests.The test prints the seed value which allows reproducing failures. This is a best practice for randomized testing.
program-libs/zero-copy-derive/tests/instruction_data.rs (1)
1149-1160: Correct Option comparison pattern using match expressions.This PartialEq implementation correctly uses match expressions to handle all Option cases, avoiding the unwrap issues seen in other implementations.
Also applies to: 1211-1219
| use borsh::{BorshDeserialize, BorshSerialize}; | ||
| use light_zero_copy_derive::ZeroCopy; | ||
|
|
||
| #[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy)] | ||
| pub struct SimpleTest { | ||
| pub id: u64, | ||
| pub active: bool, | ||
| pub values: Vec<u32>, | ||
| } | ||
|
|
||
| fn main() {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert to proper test module with assertions
This appears to be a test file based on its path, but it only contains a struct definition without any actual tests. Consider adding test functions to verify the behavior of the ZeroCopy derive macro.
use borsh::{BorshDeserialize, BorshSerialize};
use light_zero_copy_derive::ZeroCopy;
#[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy)]
pub struct SimpleTest {
pub id: u64,
pub active: bool,
pub values: Vec<u32>,
}
-fn main() {}
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn test_zero_copy_derive() {
+ // Add test assertions here to verify ZeroCopy behavior
+ // Example: test serialization/deserialization round-trip
+ }
+}📝 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.
| use borsh::{BorshDeserialize, BorshSerialize}; | |
| use light_zero_copy_derive::ZeroCopy; | |
| #[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy)] | |
| pub struct SimpleTest { | |
| pub id: u64, | |
| pub active: bool, | |
| pub values: Vec<u32>, | |
| } | |
| fn main() {} | |
| use borsh::{BorshDeserialize, BorshSerialize}; | |
| use light_zero_copy_derive::ZeroCopy; | |
| #[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy)] | |
| pub struct SimpleTest { | |
| pub id: u64, | |
| pub active: bool, | |
| pub values: Vec<u32>, | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| #[test] | |
| fn test_zero_copy_derive() { | |
| // Add test assertions here to verify ZeroCopy behavior | |
| // Example: test serialization/deserialization round-trip | |
| } | |
| } |
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/test_debug.rs lines 1 to 11, the file currently
only defines a struct without any test functions. Convert this into a proper
test module by adding a #[cfg(test)] mod with test functions that create
instances of SimpleTest and include assertions to verify the behavior of the
ZeroCopy derive macro, such as serialization and deserialization correctness.
| pub fn derive_zero_copy_mut_impl(fn_input: TokenStream) -> syn::Result<proc_macro2::TokenStream> { | ||
| // Parse the input DeriveInput | ||
| let input: DeriveInput = syn::parse(fn_input.clone())?; | ||
|
|
||
| let hasher = false; | ||
|
|
||
| // Process the input to extract struct information | ||
| let (name, z_struct_name, z_struct_meta_name, fields) = utils::process_input(&input)?; | ||
|
|
||
| // Process the fields to separate meta fields and struct fields | ||
| let (meta_fields, struct_fields) = utils::process_fields(fields); | ||
|
|
||
| let meta_struct_def_mut = if !meta_fields.is_empty() { | ||
| meta_struct::generate_meta_struct::<true>(&z_struct_meta_name, &meta_fields, hasher)? | ||
| } else { | ||
| quote! {} | ||
| }; | ||
|
|
||
| let z_struct_def_mut = z_struct::generate_z_struct::<true>( | ||
| &z_struct_name, | ||
| &z_struct_meta_name, | ||
| &struct_fields, | ||
| &meta_fields, | ||
| hasher, | ||
| )?; | ||
|
|
||
| let zero_copy_struct_inner_impl_mut = zero_copy::generate_zero_copy_struct_inner::<true>( | ||
| name, | ||
| &format_ident!("{}Mut", z_struct_name), | ||
| )?; | ||
|
|
||
| let deserialize_impl_mut = zero_copy::generate_deserialize_impl::<true>( | ||
| name, | ||
| &z_struct_name, | ||
| &z_struct_meta_name, | ||
| &struct_fields, | ||
| meta_fields.is_empty(), | ||
| quote! {}, | ||
| )?; | ||
|
|
||
| // Parse the input DeriveInput | ||
| let input: DeriveInput = syn::parse(fn_input)?; | ||
|
|
||
| // Process the input to extract struct information | ||
| let (name, _z_struct_name, _z_struct_meta_name, fields) = utils::process_input(&input)?; | ||
|
|
||
| // Use the same field processing logic as other derive macros for consistency | ||
| let (meta_fields, struct_fields) = utils::process_fields(fields); | ||
|
|
||
| // Process ALL fields uniformly by type (no position dependency for config generation) | ||
| let all_fields: Vec<&syn::Field> = meta_fields | ||
| .iter() | ||
| .chain(struct_fields.iter()) | ||
| .cloned() | ||
| .collect(); | ||
| let all_field_types = analyze_struct_fields(&all_fields)?; | ||
|
|
||
| // Generate configuration struct based on all fields that need config (type-based) | ||
| let config_struct = generate_config_struct(name, &all_field_types)?; | ||
|
|
||
| // Generate ZeroCopyNew implementation using the existing field separation | ||
| let init_mut_impl = generate_init_mut_impl(name, &meta_fields, &struct_fields)?; |
There was a problem hiding this comment.
Eliminate duplicate parsing and processing
The input is parsed twice and fields are processed twice with identical logic. This is inefficient and makes the code harder to maintain.
pub fn derive_zero_copy_mut_impl(fn_input: TokenStream) -> syn::Result<proc_macro2::TokenStream> {
// Parse the input DeriveInput
let input: DeriveInput = syn::parse(fn_input.clone())?;
let hasher = false;
// Process the input to extract struct information
let (name, z_struct_name, z_struct_meta_name, fields) = utils::process_input(&input)?;
// Process the fields to separate meta fields and struct fields
let (meta_fields, struct_fields) = utils::process_fields(fields);
let meta_struct_def_mut = if !meta_fields.is_empty() {
meta_struct::generate_meta_struct::<true>(&z_struct_meta_name, &meta_fields, hasher)?
} else {
quote! {}
};
let z_struct_def_mut = z_struct::generate_z_struct::<true>(
&z_struct_name,
&z_struct_meta_name,
&struct_fields,
&meta_fields,
hasher,
)?;
let zero_copy_struct_inner_impl_mut = zero_copy::generate_zero_copy_struct_inner::<true>(
name,
&format_ident!("{}Mut", z_struct_name),
)?;
let deserialize_impl_mut = zero_copy::generate_deserialize_impl::<true>(
name,
&z_struct_name,
&z_struct_meta_name,
&struct_fields,
meta_fields.is_empty(),
quote! {},
)?;
- // Parse the input DeriveInput
- let input: DeriveInput = syn::parse(fn_input)?;
-
- // Process the input to extract struct information
- let (name, _z_struct_name, _z_struct_meta_name, fields) = utils::process_input(&input)?;
-
- // Use the same field processing logic as other derive macros for consistency
- let (meta_fields, struct_fields) = utils::process_fields(fields);
-
// Process ALL fields uniformly by type (no position dependency for config generation)
let all_fields: Vec<&syn::Field> = meta_fields
.iter()
.chain(struct_fields.iter())
.cloned()
.collect();
let all_field_types = analyze_struct_fields(&all_fields)?;
// Generate configuration struct based on all fields that need config (type-based)
let config_struct = generate_config_struct(name, &all_field_types)?;
// Generate ZeroCopyNew implementation using the existing field separation
let init_mut_impl = generate_init_mut_impl(name, &meta_fields, &struct_fields)?;📝 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.
| pub fn derive_zero_copy_mut_impl(fn_input: TokenStream) -> syn::Result<proc_macro2::TokenStream> { | |
| // Parse the input DeriveInput | |
| let input: DeriveInput = syn::parse(fn_input.clone())?; | |
| let hasher = false; | |
| // Process the input to extract struct information | |
| let (name, z_struct_name, z_struct_meta_name, fields) = utils::process_input(&input)?; | |
| // Process the fields to separate meta fields and struct fields | |
| let (meta_fields, struct_fields) = utils::process_fields(fields); | |
| let meta_struct_def_mut = if !meta_fields.is_empty() { | |
| meta_struct::generate_meta_struct::<true>(&z_struct_meta_name, &meta_fields, hasher)? | |
| } else { | |
| quote! {} | |
| }; | |
| let z_struct_def_mut = z_struct::generate_z_struct::<true>( | |
| &z_struct_name, | |
| &z_struct_meta_name, | |
| &struct_fields, | |
| &meta_fields, | |
| hasher, | |
| )?; | |
| let zero_copy_struct_inner_impl_mut = zero_copy::generate_zero_copy_struct_inner::<true>( | |
| name, | |
| &format_ident!("{}Mut", z_struct_name), | |
| )?; | |
| let deserialize_impl_mut = zero_copy::generate_deserialize_impl::<true>( | |
| name, | |
| &z_struct_name, | |
| &z_struct_meta_name, | |
| &struct_fields, | |
| meta_fields.is_empty(), | |
| quote! {}, | |
| )?; | |
| // Parse the input DeriveInput | |
| let input: DeriveInput = syn::parse(fn_input)?; | |
| // Process the input to extract struct information | |
| let (name, _z_struct_name, _z_struct_meta_name, fields) = utils::process_input(&input)?; | |
| // Use the same field processing logic as other derive macros for consistency | |
| let (meta_fields, struct_fields) = utils::process_fields(fields); | |
| // Process ALL fields uniformly by type (no position dependency for config generation) | |
| let all_fields: Vec<&syn::Field> = meta_fields | |
| .iter() | |
| .chain(struct_fields.iter()) | |
| .cloned() | |
| .collect(); | |
| let all_field_types = analyze_struct_fields(&all_fields)?; | |
| // Generate configuration struct based on all fields that need config (type-based) | |
| let config_struct = generate_config_struct(name, &all_field_types)?; | |
| // Generate ZeroCopyNew implementation using the existing field separation | |
| let init_mut_impl = generate_init_mut_impl(name, &meta_fields, &struct_fields)?; | |
| pub fn derive_zero_copy_mut_impl(fn_input: TokenStream) -> syn::Result<proc_macro2::TokenStream> { | |
| // Parse the input DeriveInput | |
| let input: DeriveInput = syn::parse(fn_input.clone())?; | |
| let hasher = false; | |
| // Process the input to extract struct information | |
| let (name, z_struct_name, z_struct_meta_name, fields) = utils::process_input(&input)?; | |
| // Process the fields to separate meta fields and struct fields | |
| let (meta_fields, struct_fields) = utils::process_fields(fields); | |
| let meta_struct_def_mut = if !meta_fields.is_empty() { | |
| meta_struct::generate_meta_struct::<true>(&z_struct_meta_name, &meta_fields, hasher)? | |
| } else { | |
| quote! {} | |
| }; | |
| let z_struct_def_mut = z_struct::generate_z_struct::<true>( | |
| &z_struct_name, | |
| &z_struct_meta_name, | |
| &struct_fields, | |
| &meta_fields, | |
| hasher, | |
| )?; | |
| let zero_copy_struct_inner_impl_mut = zero_copy::generate_zero_copy_struct_inner::<true>( | |
| name, | |
| &format_ident!("{}Mut", z_struct_name), | |
| )?; | |
| let deserialize_impl_mut = zero_copy::generate_deserialize_impl::<true>( | |
| name, | |
| &z_struct_name, | |
| &z_struct_meta_name, | |
| &struct_fields, | |
| meta_fields.is_empty(), | |
| quote! {}, | |
| )?; | |
| // Process ALL fields uniformly by type (no position dependency for config generation) | |
| let all_fields: Vec<&syn::Field> = meta_fields | |
| .iter() | |
| .chain(struct_fields.iter()) | |
| .cloned() | |
| .collect(); | |
| let all_field_types = analyze_struct_fields(&all_fields)?; | |
| // Generate configuration struct based on all fields that need config (type-based) | |
| let config_struct = generate_config_struct(name, &all_field_types)?; | |
| // Generate ZeroCopyNew implementation using the existing field separation | |
| let init_mut_impl = generate_init_mut_impl(name, &meta_fields, &struct_fields)?; | |
| // …rest of the function… | |
| } |
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/src/zero_copy_mut.rs between lines 13 and 74,
the input TokenStream is parsed twice and the fields are processed twice with
the same logic, causing redundancy and inefficiency. Refactor the function to
parse the input and process the fields only once, then reuse the parsed data and
processed fields for all subsequent operations to eliminate duplication and
improve maintainability.
| 2. `#[derive(ZeroCopyEq)]` - Adds equality comparison support: | ||
| - Compare zero-copy instances with regular struct instances | ||
| - Can be used alongside `ZeroCopy` for complete functionality | ||
| - Derivation for Options<struct> is not robust and may not compile. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the Option type limitation with specific examples.
The documentation mentions that "Derivation for Options is not robust and may not compile" but doesn't explain what patterns fail or why. Please provide specific examples of what works and what doesn't, along with any workarounds.
Consider expanding this line:
- - Derivation for Options<struct> is not robust and may not compile.
+ - Derivation for Options<struct> has limitations:
+ - Option<CustomStruct> may fail to compile due to dereference pattern issues
+ - Option<T> where T implements Copy typically works correctly
+ - Workaround: Consider using a different representation or manual implementation📝 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.
| - Derivation for Options<struct> is not robust and may not compile. | |
| - Derivation for Options<struct> has limitations: | |
| - Option<CustomStruct> may fail to compile due to dereference pattern issues | |
| - Option<T> where T implements Copy typically works correctly | |
| - Workaround: Consider using a different representation or manual implementation |
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/README.md at line 18, the note about the
limitation of deriving for Options<struct> lacks clarity and examples. Expand
this section by adding specific examples showing which Option patterns compile
successfully and which fail, explaining the reasons behind these failures. Also,
include any known workarounds or alternative approaches to handle these cases to
improve user understanding.
| ### With Equality Comparison | ||
|
|
||
| ```rust | ||
| use borsh::{BorshDeserialize, BorshSerialize}; | ||
| use light_zero_copy_derive::ZeroCopy; | ||
|
|
||
| #[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(); | ||
| let (zero_copy, _remaining) = MyStruct::zero_copy_at(&bytes).unwrap(); | ||
| assert_eq!(zero_copy, my_struct); | ||
| ``` |
There was a problem hiding this comment.
Fix the equality comparison example to actually demonstrate ZeroCopyEq.
The example titled "With Equality Comparison" doesn't actually demonstrate the ZeroCopyEq derive macro. It's missing the import and derive attribute for ZeroCopyEq.
Apply this diff to fix the example:
use borsh::{BorshDeserialize, BorshSerialize};
-use light_zero_copy_derive::ZeroCopy;
+use light_zero_copy_derive::{ZeroCopy, ZeroCopyEq};
#[repr(C)]
-#[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy)]
+#[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy, ZeroCopyEq)]
pub struct MyStruct {
pub a: u8,
pub b: u16,
pub vec: Vec<u8>,
pub c: u64,
}📝 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.
| ### With Equality Comparison | |
| ```rust | |
| use borsh::{BorshDeserialize, BorshSerialize}; | |
| use light_zero_copy_derive::ZeroCopy; | |
| #[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(); | |
| let (zero_copy, _remaining) = MyStruct::zero_copy_at(&bytes).unwrap(); | |
| assert_eq!(zero_copy, my_struct); | |
| ``` | |
| use borsh::{BorshDeserialize, BorshSerialize}; | |
| use light_zero_copy_derive::{ZeroCopy, ZeroCopyEq}; | |
| #[repr(C)] | |
| #[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, ZeroCopy, ZeroCopyEq)] | |
| 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(); | |
| let (zero_copy, _remaining) = MyStruct::zero_copy_at(&bytes).unwrap(); | |
| assert_eq!(zero_copy, my_struct); |
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/README.md between lines 79 and 103, the example
titled "With Equality Comparison" does not demonstrate the ZeroCopyEq derive
macro properly. To fix this, add the import for ZeroCopyEq from
light_zero_copy_derive and include ZeroCopyEq in the derive attribute list for
MyStruct. This will correctly show how to use the ZeroCopyEq macro for equality
comparison in the example.
ananas-block
force-pushed
the
jorrit/refactor-zero-copy-borsh
branch
2 times, most recently
from
f5870e8 to
bdf3cf6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
program-libs/zero-copy-derive/tests/config_test.rs (1)
22-30: Inconsistency between tests and macro implementation.The tests use
SimpleVecStructConfigand other config structs, but looking atlib.rs, theZeroCopyNewmacro is commented out (lines 130-166). This creates an inconsistency where tests expect functionality that appears to be disabled in the main macro implementation.Either:
- Enable the
ZeroCopyNewmacro inlib.rsif it's implemented- Manually define the config structs these tests expect
- Add feature gates to disable these tests when the macro is unavailable
#!/bin/bash # Verify if ZeroCopyNew is implemented elsewhere or config structs exist ast-grep --pattern $'pub struct $_ Config { $$$ }' # Check for ZeroCopyNew implementations rg -A 5 "impl.*ZeroCopyNew" # Check if ZeroCopyNew is re-exported from other modules rg "ZeroCopyNew" --exclude-dir=testsprogram-libs/zero-copy-derive/src/lib.rs (1)
130-166: Commented-out ZeroCopyNew macro conflicts with test expectations.The
ZeroCopyNewderive macro is commented out, butconfig_test.rstests actively use the generated config structs andZeroCopyNewtrait implementations. This creates a build inconsistency.To resolve this issue:
- Implement the macro by uncommenting and ensuring it works correctly with the existing test suite
- Remove dependent tests until the macro is ready
- Add feature gates to conditionally enable/disable the functionality
#!/bin/bash # Check if ZeroCopyNew functionality is implemented in the zero_copy_mut module rg -A 10 -B 5 "ZeroCopyNew" program-libs/zero-copy-derive/src/ # Verify test dependencies on commented-out functionality rg "ZeroCopyNew\|Config.*{" program-libs/zero-copy-derive/tests/config_test.rsprogram-libs/zero-copy-derive/src/shared/zero_copy_new.rs (3)
233-235: Critical: Integer overflow risk in split operationsConverting
config.#field_nametousizewithout bounds checking could cause issues if the config value exceedsusize::MAXon 32-bit systems.Consider using checked conversion:
- let (#field_name, bytes) = bytes.split_at_mut(config.#field_name as usize); + let len = usize::try_from(config.#field_name) + .map_err(|_| light_zero_copy::errors::ZeroCopyError::InvalidCapacity)?; + let (#field_name, bytes) = bytes.split_at_mut(len);
241-244: Critical: Unsafe integer conversion in Vec initializationThe
.into()conversion from u32 to the slice's length type happens without bounds checking.Replace with explicit conversion that handles potential overflow:
- let (#field_name, bytes) = light_zero_copy::slice_mut::ZeroCopySliceMutBorsh::<#inner_type>::new_at( - config.#field_name.into(), - bytes - )?; + let len = light_zero_copy::slice::Length::try_from(config.#field_name) + .map_err(|_| light_zero_copy::errors::ZeroCopyError::InvalidCapacity)?; + let (#field_name, bytes) = light_zero_copy::slice_mut::ZeroCopySliceMutBorsh::<#inner_type>::new_at( + len, + bytes + )?;
316-317: Critical: Potential arithmetic overflow in byte length calculationThe arithmetic operations could overflow if config values are large.
Use checked arithmetic:
- (4 + config.#field_name as usize) // 4 bytes for length + actual data + 4usize.checked_add(config.#field_name as usize) + .ok_or(light_zero_copy::errors::ZeroCopyError::InvalidCapacity)?program-libs/zero-copy-derive/src/shared/utils.rs (2)
83-99: Type detection correctly handles qualified paths.The functions properly use
path.segments.last()to check the final segment, correctly identifyingVecandOptiontypes regardless of qualification (e.g.,std::vec::Vec).
167-174: Hardcoded "Pubkey" detection remains inflexible.The hardcoded check for "Pubkey" could match unintended types or miss qualified paths. Consider using a configurable list of known Copy types or documenting this assumption clearly.
+// Known Copy types that should be treated specially +const KNOWN_COPY_TYPES: &[&str] = &["Pubkey"]; + pub fn is_pubkey_type(ty: &Type) -> bool { if let Type::Path(TypePath { path, .. }) = ty { if let Some(segment) = path.segments.last() { - return segment.ident == "Pubkey"; + return KNOWN_COPY_TYPES.contains(&segment.ident.to_string().as_str()); } } false }
🧹 Nitpick comments (12)
program-libs/zero-copy-derive/src/zero_copy.rs (2)
197-564: Consider removing commented test codeLarge blocks of commented test code reduce readability and may become outdated. If these tests are valuable, consider either:
- Moving them to a separate test file if they're still relevant
- Removing them entirely if they're no longer needed
56-60: Potential issue: Complex trait path generationThe trait path generation for
VecCopyfields is quite complex. Consider extracting this into a helper function for better maintainability.Example refactor:
+fn generate_copy_slice_trait_path<const MUT: bool>(inner_type: &syn::Type) -> TokenStream { + let inner_type = utils::convert_to_zerocopy_type(inner_type); + if MUT { + quote!(light_zero_copy::slice_mut::ZeroCopySliceMutBorsh::<'a, <#inner_type as light_zero_copy::borsh_mut::ZeroCopyStructInnerMut>::ZeroCopyInnerMut>) + } else { + quote!(light_zero_copy::slice::ZeroCopySliceBorsh::<'a, <#inner_type as light_zero_copy::borsh::ZeroCopyStructInner>::ZeroCopyInner>) + } +} let trait_path = if MUT { - quote!(light_zero_copy::slice_mut::ZeroCopySliceMutBorsh::<'a, <#inner_type as light_zero_copy::borsh_mut::ZeroCopyStructInnerMut>::ZeroCopyInnerMut>) + generate_copy_slice_trait_path::<true>(&inner_type) } else { - quote!(light_zero_copy::slice::ZeroCopySliceBorsh::<'a, <#inner_type as light_zero_copy::borsh::ZeroCopyStructInner>::ZeroCopyInner>) + generate_copy_slice_trait_path::<false>(&inner_type) };program-libs/zero-copy/src/borsh_mut.rs (1)
665-666: Consider more robust error handling innew_with_data.The method uses
unwrap()which will panic if deserialization fails. While acceptable for test code, consider returning aResultfor more robust error handling.- let (mut zero_copy, bytes) = - <Struct4 as DeserializeMut>::zero_copy_at_mut(bytes).unwrap(); + let (mut zero_copy, bytes) = + <Struct4 as DeserializeMut>::zero_copy_at_mut(bytes)?;program-libs/zero-copy/tests/borsh_2.rs (3)
40-43: Remove commented-out code.The commented-out function
data_hash_struct_1should be removed to keep the codebase clean.-// pub fn data_hash_struct_1(a: u8, b: u16) -> [u8; 32] { - -// }
320-327: Complex nested deserialization pattern.The
Vec::<ZeroCopySliceBorsh<...>>::zero_copy_atpattern is quite complex. Consider adding a comment explaining this nested zero-copy deserialization.fn zero_copy_at(bytes: &'a [u8]) -> Result<(Self::Output, &'a [u8]), ZeroCopyError> { + // Deserialize a vector of zero-copy slices for nested Vec<Vec<u8>> structure let (a, bytes) = Vec::<ZeroCopySliceBorsh<<u8 as ZeroCopyStructInner>::ZeroCopyInner>>::zero_copy_at( bytes, )?;
544-557: Reference comparison pattern repeated.The same reference-based comparison pattern is used here. Consider creating a helper function if this pattern is common.
Consider extracting a helper function for these reference-based vector comparisons to improve readability and reduce duplication.
program-libs/zero-copy-derive/tests/random.rs (6)
96-98: Inefficient byte-by-byte copying.The inner loop copies bytes one by one, which is inefficient. Use
copy_from_slicefor better performance.- for (src_byte, dst_byte) in src_data.data.iter().zip(dst_data.data.iter_mut()) { - *dst_byte = *src_byte; - } + let copy_len = src_data.data.len().min(dst_data.data.len()); + dst_data.data[..copy_len].copy_from_slice(&src_data.data[..copy_len]);
304-324: Hardcoded array size in random data generation.The hardcoded size of 64 for the
bfield should be defined as a constant to improve maintainability.+const COMPRESSED_PROOF_B_SIZE: usize = 64; + fn get_rnd_instruction_data_invoke_cpi(rng: &mut StdRng) -> InstructionDataInvokeCpi { InstructionDataInvokeCpi { proof: Some(CompressedProof { a: rng.gen(), - b: (0..64) + b: (0..COMPRESSED_PROOF_B_SIZE) .map(|_| rng.gen()) .collect::<Vec<u8>>() .try_into() .unwrap(),
337-337: Magic number for data size.The hardcoded value of 100 for data length should be configurable or defined as a constant.
+const TEST_DATA_SIZE: usize = 100; + fn get_rnd_test_account_data(rng: &mut StdRng) -> CompressedAccountData { CompressedAccountData { discriminator: rng.gen(), - data: (0..100).map(|_| rng.gen()).collect::<Vec<u8>>(), + data: (0..TEST_DATA_SIZE).map(|_| rng.gen()).collect::<Vec<u8>>(), data_hash: rng.gen(), } }
514-515: Large iteration counts may cause slow tests.The tests run 1,000 and 10,000 iterations respectively, which could make the test suite slow. Consider making these configurable via environment variables.
let num_iters = std::env::var("TEST_ITERATIONS") .map(|s| s.parse().unwrap_or(100)) .unwrap_or(100);Also applies to: 605-605
577-584: Missing error context in assertions.The assertion messages could be more descriptive to help with debugging when tests fail.
- assert_eq!(invoke_ref, borsh_ref); + assert_eq!( + invoke_ref, borsh_ref, + "Round-trip serialization/deserialization failed for iteration {}", i + );
588-591: Verbose test output.The test produces output for every successful iteration, which could spam test logs. Consider reducing verbosity or making it conditional.
- println!("✓ Successfully tested InstructionDataInvoke with {} inputs, {} outputs, {} new_addresses", - invoke_ref.input_compressed_accounts_with_merkle_context.len(), - invoke_ref.output_compressed_accounts.len(), - invoke_ref.new_address_params.len()); + if i % 100 == 0 { + println!("✓ Completed {} iterations", i + 1); + }
📜 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 (31)
.github/workflows/rust.yml(1 hunks).gitignore(1 hunks)Cargo.toml(2 hunks)program-libs/compressed-account/src/instruction_data/with_account_info.rs(1 hunks)program-libs/compressed-account/src/instruction_data/with_readonly.rs(1 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/lib.rs(1 hunks)program-libs/zero-copy-derive/src/shared/from_impl.rs(1 hunks)program-libs/zero-copy-derive/src/shared/meta_struct.rs(1 hunks)program-libs/zero-copy-derive/src/shared/mod.rs(1 hunks)program-libs/zero-copy-derive/src/shared/utils.rs(1 hunks)program-libs/zero-copy-derive/src/shared/z_struct.rs(1 hunks)program-libs/zero-copy-derive/src/shared/zero_copy_new.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_eq.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_mut.rs(1 hunks)program-libs/zero-copy-derive/tests/config_test.rs(1 hunks)program-libs/zero-copy-derive/tests/cross_crate_copy.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(4 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
✅ Files skipped from review due to trivial changes (2)
- program-libs/zero-copy-derive/src/shared/mod.rs
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (15)
- .gitignore
- .github/workflows/rust.yml
- program-libs/zero-copy-derive/src/zero_copy_mut.rs
- program-libs/zero-copy-derive/src/shared/meta_struct.rs
- program-libs/zero-copy-derive/Cargo.toml
- program-libs/zero-copy-derive/README.md
- program-libs/zero-copy/src/slice_mut.rs
- program-libs/zero-copy/Cargo.toml
- program-libs/zero-copy-derive/src/shared/from_impl.rs
- program-libs/zero-copy-derive/tests/from_test.rs
- program-libs/zero-copy-derive/src/zero_copy_eq.rs
- program-libs/zero-copy/src/lib.rs
- program-libs/zero-copy/src/init_mut.rs
- program-libs/zero-copy/tests/borsh.rs
- program-libs/zero-copy-derive/tests/instruction_data.rs
🧰 Additional context used
🧬 Code Graph Analysis (3)
program-libs/zero-copy-derive/tests/config_test.rs (3)
program-libs/zero-copy/src/init_mut.rs (25)
config(241-244)byte_len(23-23)byte_len(42-51)byte_len(81-83)byte_len(98-100)byte_len(115-117)byte_len(132-134)byte_len(149-151)byte_len(172-174)byte_len(190-192)byte_len(206-208)byte_len(222-224)byte_len(239-245)new_zero_copy(28-31)new_zero_copy(53-73)new_zero_copy(85-91)new_zero_copy(102-108)new_zero_copy(119-125)new_zero_copy(136-142)new_zero_copy(153-159)new_zero_copy(176-182)new_zero_copy(194-199)new_zero_copy(210-215)new_zero_copy(226-231)new_zero_copy(247-267)program-libs/zero-copy/src/slice_mut.rs (1)
len(116-119)program-libs/zero-copy/src/borsh_mut.rs (15)
zero_copy_at_mut(20-21)zero_copy_at_mut(29-34)zero_copy_at_mut(41-56)zero_copy_at_mut(63-67)zero_copy_at_mut(74-78)zero_copy_at_mut(86-91)zero_copy_at_mut(97-117)zero_copy_at_mut(186-197)zero_copy_at_mut(404-409)zero_copy_at_mut(467-478)zero_copy_at_mut(531-538)zero_copy_at_mut(569-574)zero_copy_at_mut(627-646)zero_copy_at_mut(716-723)zero_copy_at_mut(757-762)
program-libs/zero-copy/src/borsh_mut.rs (3)
program-libs/zero-copy/src/borsh.rs (16)
new(146-148)deref(153-155)deref(375-377)deref(433-435)deref(490-492)deref(572-574)deref(743-745)deref_mut(159-161)test_vecu8(231-237)test_vecu8_deref(299-307)eq(421-427)eq(731-737)eq(818-820)test_struct_4(599-619)test_struct_7(759-785)test_struct_8(839-872)program-libs/zero-copy/src/slice_mut.rs (7)
new(35-37)iter(246-248)eq(263-265)len(116-119)from_bytes(56-58)to_vec(178-180)from_bytes_at(61-85)program-libs/zero-copy-derive/src/zero_copy_mut.rs (2)
zero_copy(40-40)zero_copy(45-45)
program-libs/zero-copy/tests/borsh_2.rs (2)
program-libs/zero-copy/src/borsh_mut.rs (10)
deref(171-173)deref(390-392)deref(459-461)deref(523-525)deref(619-621)deref(829-831)test_struct_1(413-422)eq(447-453)eq(817-823)eq(909-911)program-libs/zero-copy/src/borsh.rs (21)
deref(153-155)deref(375-377)deref(433-435)deref(490-492)deref(572-574)zero_copy_at(20-20)zero_copy_at(27-30)zero_copy_at(36-49)zero_copy_at(56-62)zero_copy_at(69-75)zero_copy_at(113-116)zero_copy_at(122-140)zero_copy_at(168-178)zero_copy_at(383-386)zero_copy_at(441-445)zero_copy_at(498-503)zero_copy_at(580-595)test_struct_1(390-396)eq(421-427)eq(731-737)eq(818-820)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: cli-v2
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test sdk-libs
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: cli-v1
- GitHub Check: lint
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
🔇 Additional comments (31)
program-libs/compressed-account/src/instruction_data/with_readonly.rs (1)
350-357: Excellent security improvement to prevent heap exhaustion attacks.This validation correctly prevents potential DoS attacks by ensuring that
num_slicesdoesn't exceed the available data length before allocating a vector. The check is minimal yet effective - since each serialized element requires at least 1 byte, this prevents unrealistic allocation requests.program-libs/compressed-account/src/instruction_data/with_account_info.rs (1)
403-408: Consistent application of heap exhaustion prevention.This implements the same security validation as in
with_readonly.rs, ensuring consistent protection across both instruction data deserialization paths. The approach correctly prevents malicious inputs from causing excessive memory allocation.program-libs/zero-copy-derive/tests/cross_crate_copy.rs (1)
1-296: Comprehensive test coverage for Copy trait detection.This test module provides excellent coverage for validating the zero-copy derive macro's Copy trait detection logic. The tests effectively cover:
- Primitive Copy types in meta fields vs struct fields
- Array handling with different element types
- Vec field splitting behavior based on field ordering
- All derive macros (ZeroCopy, ZeroCopyEq, ZeroCopyMut)
- Cross-crate boundary Copy detection
The field ordering tests are particularly valuable for ensuring the macro correctly implements the "split at first Vec" rule described in the main macro documentation.
program-libs/zero-copy-derive/tests/config_test.rs (1)
32-89: Test logic demonstrates proper zero-copy mutable initialization.Despite the macro availability concern, the test logic correctly validates:
- Exact buffer size calculation via
byte_len()- Zero-copy mutable initialization via
new_zero_copy()- Field access patterns for meta fields vs struct fields
- Round-trip serialization/deserialization consistency
The test patterns are well-designed for validating zero-copy mutable struct functionality.
program-libs/zero-copy-derive/src/lib.rs (2)
57-64: Robust error handling in procedural macro implementation.The error handling correctly converts
syn::Errorresults into compile-time errors usingto_compile_error(), ensuring that macro parsing failures provide clear diagnostic messages to users.
86-128: Comprehensive documentation for ZeroCopyMut macro.The documentation clearly explains the macro's purpose, generated code, and usage patterns. The examples demonstrate both standalone usage and combination with other derive macros.
program-libs/zero-copy-derive/src/shared/z_struct.rs (4)
10-24: LGTM: Well-designed FieldType enum for zero-copy field classificationThe enum provides comprehensive coverage of different field types needed for zero-copy deserialization, with clear documentation about array limitations.
67-83: Good pattern matching for option type classificationThe classification logic correctly handles special cases for primitive integer options while falling back to generic option handling.
170-318: Complex but well-structured field generation logicThe generic const parameter approach for handling mutability is elegant, and the field type matching covers all necessary cases comprehensively.
514-629: Excellent fuzz testing implementationThe fuzz test with 10,000 iterations and comprehensive validation provides strong confidence in the code generation correctness. The use of seeded RNG for reproducibility is a good practice.
program-libs/zero-copy-derive/src/zero_copy.rs (1)
589-636: LGTM: Well-structured main derive implementationThe
derive_zero_copy_implfunction properly orchestrates all the code generation steps and combines them into a cohesive implementation.program-libs/zero-copy/src/borsh_mut.rs (3)
14-22: Well-designed trait for mutable zero-copy deserialization.The
DeserializeMuttrait provides a clean interface for zero-copy mutable deserialization with proper lifetime management and error handling.
102-109: Good bounds checking to prevent heap exhaustion.The implementation correctly validates that the remaining bytes are sufficient for the declared number of elements, providing protection against malicious input that could trigger excessive memory allocation.
336-366: Excellent comprehensive test coverage with clear patterns.The test module provides extensive coverage of zero-copy mutable deserialization scenarios and demonstrates best practices for implementing zero-copy structs manually. The documented rules (lines 349-357) are particularly valuable for understanding the design patterns.
program-libs/zero-copy/src/borsh.rs (5)
8-8: Good addition of signed integer support.The signed integer types are properly imported and implemented using the same macro pattern as unsigned integers, providing consistent zero-copy deserialization capabilities.
Also applies to: 97-106
108-117: Clean implementation for fixed-size array deserialization.The implementation correctly uses
Ref::from_prefixto provide zero-copy deserialization for fixed-size arrays, following the established patterns in the codebase.
125-132: Appropriate bounds checking for heap exhaustion protection.The validation ensures sufficient bytes are available before allocation, providing reasonable protection against malicious input attempting to trigger excessive memory allocation.
181-219: Well-designed trait for zero-copy type associations.The
ZeroCopyStructInnertrait provides a clean mapping between native types and their zero-copy representations, with comprehensive implementations for primitives, containers, and arrays.
317-347: Excellent comprehensive test framework.The test module provides extensive coverage with clear documentation of the rules and patterns for implementing zero-copy structs. This serves as both testing and valuable documentation for users.
program-libs/zero-copy-derive/src/shared/utils.rs (4)
15-19: Good solution for cache key uniqueness.Using span information in
create_unique_type_keyaddresses the namespace collision concern from previous reviews, providing better uniqueness than struct name alone.
247-265: Well-implemented Copy trait detection with proper caching.The implementation correctly detects
#[derive(Copy)]attributes and uses caching for performance. The documentation clearly explains the approach and limitations.
176-217: Solid type conversion for zero-copy compatibility.The function correctly maps primitive types to their little-endian zero-copy equivalents and handles container types recursively, providing the necessary type transformations for zero-copy deserialization.
317-437: Comprehensive test coverage for Copy trait detection.The test suite thoroughly covers various scenarios including edge cases, providing confidence in the correctness of the Copy trait detection logic. The tests are well-documented and clearly structured.
program-libs/zero-copy/tests/borsh_2.rs (7)
1-12: LGTM! Clean imports and feature gating.The feature gates and imports are well-organized and appropriate for the zero-copy functionality being tested.
14-30: Excellent documentation of zero-copy design rules.The comments clearly document the structural patterns and requirements for zero-copy deserialization, which will be valuable for understanding the derive macro implementation.
104-112: Good implementation of cross-type equality comparison.The
PartialEq<Struct2>implementation correctly handles the conversion fromU16tou16and compares vector slices appropriately.
381-391: Reference comparison in assertion.The test uses references in the expected vector (
&Struct2 { ... }). This seems unusual - verify this is the intended comparison pattern.The assertion compares
zero_copy.a.iter().collect::<Vec<_>>()(which creates a vector of references) with a vector of references toStruct2instances. This pattern should be documented or simplified.
423-423: Simple option comparison.Good that the option comparison works directly between zero-copy and owned types.
298-301: Verify U16 conversion invec_2comparison
- Location:
program-libs/zero-copy/tests/borsh_2.rs, lines 298–301- The test does:
assert_eq!( zero_copy.vec_2.to_vec(), vec![ZStruct4Nested { a: 1, b: 2.into() }; 32] );- Please confirm that
zerocopy::little_endian::U16implementsFrom<u16>(and thusInto<U16>) so that2.into()produces the correctU16.- If it does not, replace the conversion with an explicit call, e.g.
U16::new(2)orFrom::from(2).
77-78: ConfirmPartialEq<u16>forU16
The test inprogram-libs/zero-copy/tests/borsh_2.rs(lines 77–78) compares alittle_endian::U16value directly to au16. SinceU16is re-exported from the externalzerocopycrate, please verify that it implementsPartialEq<u16>(and that its little-endian conversion returns the expectedu16). If it doesn’t, consider one of the following adjustments:
- Change the assertion to compare the raw value, e.g.
assert_eq!(struct1.b.get(), 2u16);- Wrap the literal in the same type, e.g.
assert_eq!(struct1.b, U16::new(2u16));program-libs/zero-copy-derive/tests/random.rs (1)
1-11: Clean feature gating and imports.The feature guard and imports are appropriate for the mutable zero-copy functionality being tested.
| FieldType::Copy(field_name, field_type) => { | ||
| quote! { | ||
| let (#field_name, bytes) = <#field_type as light_zero_copy::init_mut::ZeroCopyNew>::new_zero_copy(bytes)?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue: Incomplete field initialization for Copy types
The FieldType::Copy case is missing the required config parameter in the new_zero_copy call.
Fix the function call signature:
- let (#field_name, bytes) = <#field_type as light_zero_copy::init_mut::ZeroCopyNew>::new_zero_copy(bytes)?;
+ let (#field_name, bytes) = <#field_type as light_zero_copy::init_mut::ZeroCopyNew>::new_zero_copy(bytes, ())?;🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/src/shared/zero_copy_new.rs around lines 301 to
305, the FieldType::Copy match arm calls new_zero_copy without the required
config parameter. Modify the call to include the config argument as the second
parameter to new_zero_copy, ensuring the function signature matches the expected
usage.
| fn zero_copy_at_mut( | ||
| bytes: &'a mut [u8], | ||
| ) -> Result<(Self::Output, &'a mut [u8]), ZeroCopyError> { | ||
| let (num_slices, mut bytes) = Ref::<&mut [u8], u8>::from_prefix(bytes)?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the intentional use of u8 length prefix for VecU8.
The VecU8 implementation uses a u8 length prefix, limiting it to 255 elements, while the regular Vec<T> implementation uses U32. If this size limitation is intentional for VecU8, consider documenting it clearly.
🏁 Script executed:
#!/bin/bash
# Check if VecU8's u8 length prefix limitation is documented or tested elsewhere
rg -A 5 -B 5 "VecU8.*255|255.*VecU8" program-libs/zero-copy/Length of output: 62
🏁 Script executed:
#!/bin/bash
# Locate the VecU8 definition and surrounding context
rg -n 'struct VecU8' -n program-libs/zero-copy/src/borsh_mut.rs
# Show the implementation and any doc comments around VecU8 (lines 160–240)
sed -n '160,240p' program-libs/zero-copy/src/borsh_mut.rsLength of output: 2145
Document VecU8’s 1-byte length prefix
The VecU8 zero-copy implementation always reads its element count from a single u8 (max 255), whereas the standard Vec uses a U32 prefix. If this 255-element cap is intentional, please add a doc comment on VecU8 to surface this constraint. Otherwise, consider switching to a 32-bit length prefix or renaming VecU8 to make the byte-length behavior clear.
Locations to update:
- program-libs/zero-copy/src/borsh_mut.rs: struct VecU8 (around line 162) – add documentation of the u8 length prefix and its 255-element cap.
- program-libs/zero-copy/src/borsh_mut.rs: impl DeserializeMut for VecU8 (around the zero_copy_at_mut method) – verify tests cover the boundary (255 vs. 256) and update docs or code accordingly.
🤖 Prompt for AI Agents
In program-libs/zero-copy/src/borsh_mut.rs around line 162, add a doc comment to
the VecU8<T> struct clearly stating that it uses a 1-byte u8 length prefix
limiting the element count to 255. Then, in the impl DeserializeMut for VecU8<T>
near the zero_copy_at_mut method, verify that tests cover the boundary condition
between 255 and 256 elements and update the documentation or code to reflect
this limitation or consider changing the length prefix to u32 if the 255-element
cap is not intended.
| let (vec, bytes) = ZeroCopySliceBorsh::from_bytes_at(bytes)?; | ||
| let (c, bytes) = | ||
| Ref::<&[u8], <u64 as ZeroCopyStructInner>::ZeroCopyInner>::from_prefix(bytes)?; | ||
| let (vec_2, bytes) = ZeroCopySliceBorsh::from_bytes_at(bytes)?; |
There was a problem hiding this comment.
Inconsistent use of zero_copy_at vs from_bytes_at.
Line 266 uses ZeroCopySliceBorsh::from_bytes_at while line 184 uses ZeroCopySliceBorsh::zero_copy_at. This inconsistency could indicate a bug or should be standardized.
- 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)?;📝 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 (vec, bytes) = ZeroCopySliceBorsh::from_bytes_at(bytes)?; | |
| let (c, bytes) = | |
| Ref::<&[u8], <u64 as ZeroCopyStructInner>::ZeroCopyInner>::from_prefix(bytes)?; | |
| let (vec_2, bytes) = ZeroCopySliceBorsh::from_bytes_at(bytes)?; | |
| let (vec, bytes) = ZeroCopySliceBorsh::zero_copy_at(bytes)?; | |
| let (c, bytes) = | |
| Ref::<&[u8], <u64 as ZeroCopyStructInner>::ZeroCopyInner>::from_prefix(bytes)?; | |
| let (vec_2, bytes) = ZeroCopySliceBorsh::zero_copy_at(bytes)?; |
🤖 Prompt for AI Agents
In program-libs/zero-copy/tests/borsh_2.rs around lines 266 to 269, the method
ZeroCopySliceBorsh::from_bytes_at is used inconsistently compared to
ZeroCopySliceBorsh::zero_copy_at used earlier at line 184. Review the intended
behavior and replace from_bytes_at with zero_copy_at on line 266 to maintain
consistency, or vice versa, ensuring the same method is used throughout for
similar operations.
| .owner | ||
| .copy_from_slice(&src_input.compressed_account.owner); | ||
| dst_input.compressed_account.lamports = src_input.compressed_account.lamports.into(); | ||
|
|
||
| // Copy address if present | ||
| if let (Some(src_addr), Some(dst_addr)) = ( | ||
| &src_input.compressed_account.address, | ||
| &mut dst_input.compressed_account.address, | ||
| ) { | ||
| dst_addr.copy_from_slice(src_addr); | ||
| } | ||
|
|
||
| // Copy data if present | ||
| if let (Some(src_data), Some(dst_data)) = ( | ||
| &src_input.compressed_account.data, | ||
| &mut dst_input.compressed_account.data, | ||
| ) { | ||
| dst_data | ||
| .discriminator | ||
| .copy_from_slice(&src_data.discriminator); | ||
| dst_data.data_hash.copy_from_slice(&src_data.data_hash); | ||
| for (src_byte, dst_byte) in src_data.data.iter().zip(dst_data.data.iter_mut()) { | ||
| *dst_byte = *src_byte; | ||
| } | ||
| } | ||
|
|
||
| // Copy merkle context | ||
| dst_input.merkle_context.merkle_tree_pubkey_index = | ||
| src_input.merkle_context.merkle_tree_pubkey_index; | ||
| dst_input.merkle_context.nullifier_queue_pubkey_index = | ||
| src_input.merkle_context.nullifier_queue_pubkey_index; | ||
| dst_input.merkle_context.leaf_index = src_input.merkle_context.leaf_index.into(); | ||
| dst_input.merkle_context.prove_by_index = if src_input.merkle_context.prove_by_index { | ||
| 1 | ||
| } else { | ||
| 0 | ||
| }; | ||
|
|
||
| *dst_input.root_index = src_input.root_index.into(); | ||
| *dst_input.read_only = if src_input.read_only { 1 } else { 0 }; | ||
| } | ||
|
|
||
| // Copy output_compressed_accounts | ||
| for (src_output, dst_output) in src | ||
| .output_compressed_accounts | ||
| .iter() | ||
| .zip(dst.output_compressed_accounts.iter_mut()) | ||
| { | ||
| // Copy compressed account | ||
| dst_output | ||
| .compressed_account | ||
| .owner | ||
| .copy_from_slice(&src_output.compressed_account.owner); | ||
| dst_output.compressed_account.lamports = src_output.compressed_account.lamports.into(); | ||
|
|
||
| // Copy address if present | ||
| if let (Some(src_addr), Some(dst_addr)) = ( | ||
| &src_output.compressed_account.address, | ||
| &mut dst_output.compressed_account.address, | ||
| ) { | ||
| dst_addr.copy_from_slice(src_addr); | ||
| } | ||
|
|
||
| // Copy data if present | ||
| if let (Some(src_data), Some(dst_data)) = ( | ||
| &src_output.compressed_account.data, | ||
| &mut dst_output.compressed_account.data, | ||
| ) { | ||
| dst_data | ||
| .discriminator | ||
| .copy_from_slice(&src_data.discriminator); | ||
| dst_data.data_hash.copy_from_slice(&src_data.data_hash); | ||
| for (src_byte, dst_byte) in src_data.data.iter().zip(dst_data.data.iter_mut()) { | ||
| *dst_byte = *src_byte; | ||
| } | ||
| } | ||
|
|
||
| *dst_output.merkle_tree_index = src_output.merkle_tree_index; | ||
| } | ||
|
|
||
| // Copy relay_fee if present | ||
| if let (Some(src_fee), Some(dst_fee)) = (&src.relay_fee, &mut dst.relay_fee) { | ||
| **dst_fee = (*src_fee).into(); | ||
| } | ||
|
|
||
| // Copy compress_or_decompress_lamports if present | ||
| if let (Some(src_lamports), Some(dst_lamports)) = ( | ||
| &src.compress_or_decompress_lamports, | ||
| &mut dst.compress_or_decompress_lamports, | ||
| ) { | ||
| **dst_lamports = (*src_lamports).into(); | ||
| } | ||
|
|
||
| // Copy cpi_context if present | ||
| if let (Some(src_ctx), Some(dst_ctx)) = (&src.cpi_context, &mut dst.cpi_context) { | ||
| dst_ctx.set_context = if src_ctx.set_context { 1 } else { 0 }; | ||
| dst_ctx.first_set_context = if src_ctx.first_set_context { 1 } else { 0 }; | ||
| dst_ctx.cpi_context_account_index = src_ctx.cpi_context_account_index; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Significant code duplication between populate functions.
The populate_invoke_cpi_zero_copy and populate_invoke_zero_copy functions are nearly identical with ~95% code duplication. This violates the DRY principle and makes maintenance difficult.
Consider extracting the common logic into a shared helper function or using generics to eliminate this duplication:
fn populate_common_fields<T, U>(
src_proof: &Option<CompressedProof>,
dst_proof: &mut Option<T>,
// ... other common parameters
) {
// Common population logic
}
fn populate_invoke_zero_copy(src: &InstructionDataInvoke, dst: &mut ZInstructionDataInvokeMut) {
populate_common_fields(&src.proof, &mut dst.proof, /* ... */);
// Handle invoke-specific fields
}
fn populate_invoke_cpi_zero_copy(src: &InstructionDataInvokeCpi, dst: &mut ZInstructionDataInvokeCpiMut) {
populate_common_fields(&src.proof, &mut dst.proof, /* ... */);
// Handle CPI-specific fields like cpi_context
}Also applies to: 176-300
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/tests/random.rs around lines 41 to 174, the
function populate_invoke_cpi_zero_copy contains nearly identical code to
populate_invoke_zero_copy, causing significant duplication. To fix this, extract
the shared logic for copying common fields such as proof, new_address_params,
compressed accounts, and other repeated structures into a generic helper
function that accepts source and destination references. Then, refactor both
populate_invoke_zero_copy and populate_invoke_cpi_zero_copy to call this helper
for the common parts and handle their specific fields separately, reducing
duplication and improving maintainability. Apply the same refactoring approach
to lines 176 to 300 where similar duplication exists.
bdf3cf6 to
95ebdc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (16)
program-libs/zero-copy-derive/src/zero_copy_eq.rs (1)
57-65: Remove unreachable Option handling code or fix the logic.The function returns an error if any
Optionfields are found (lines 57-65), making the Option handling code (lines 97-133) unreachable. This is a logic error that needs to be resolved.Either:
- Remove the early return to allow Option handling:
- if field_types - .iter() - .any(|x| matches!(x, FieldType::Option(_, _))) - { - return Err(syn::Error::new_spanned( - struct_fields[0], - "Options are not supported in ZeroCopyEq", - )); - }
- Or remove the unreachable Option handling code entirely if Options truly aren't supported.
program-libs/zero-copy-derive/src/shared/zero_copy_new.rs (4)
223-235: Potential integer overflow in VecU8 initializationThe integer conversion issues flagged in previous reviews are still present:
- Line 228:
.into()conversion without bounds checking- Line 234: Direct
as usizeconversion without overflow protection
238-245: Unsafe integer conversion in VecCopy initializationThe unsafe
.into()conversion flagged in previous reviews is still present on line 241.
301-305: Missing config parameter in Copy field initializationThe
new_zero_copycall is still missing the required config parameter as flagged in previous reviews.
314-318: Potential arithmetic overflow in VecU8 byte length calculationThe arithmetic operation without overflow checking flagged in previous reviews is still present on line 316.
program-libs/zero-copy/src/borsh_mut.rs (1)
161-198: Add documentation for VecU8's u8 length prefix limitation.The VecU8 implementation uses a single u8 for length encoding (line 189), limiting it to 255 elements, while regular Vec uses U32. This constraint should be documented.
Add a doc comment to clearly state this limitation:
+/// A vector wrapper that uses a single byte (u8) for length encoding. +/// This limits the maximum number of elements to 255. +/// Use regular Vec<T> for larger collections. #[derive(Clone, Debug, Default, PartialEq)] pub struct VecU8<T>(Vec<T>);program-libs/zero-copy/tests/borsh_2.rs (1)
266-269: Fix inconsistent method usage.The code uses
ZeroCopySliceBorsh::from_bytes_athere while line 184 usesZeroCopySliceBorsh::zero_copy_at. This inconsistency should be resolved for maintainability.Standardize to use the same method throughout:
- 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/instruction_data.rs (9)
98-183: This commented-out code should be removed.This large block of commented-out manual
ZeroCopyNewimplementation has been replaced by derive macros and should be cleaned up to improve code maintainability.
217-237: Remove commented-out manual implementation.This commented-out
ZeroCopyNewimplementation is now handled by derive macros and should be removed.
258-269: Remove commented-out manual implementation.This commented-out
ZeroCopyNewimplementation is now handled by derive macros and should be removed.
351-362: Remove commented-out manual implementation.This commented-out
ZeroCopyNewimplementation is now handled by derive macros and should be removed.
408-432: Remove commented-out manual implementation.This commented-out
ZeroCopyNewimplementation is now handled by derive macros and should be removed.
453-470: Remove commented-out manual implementation.This commented-out
ZeroCopyNewimplementation is now handled by derive macros and should be removed.
541-552: Remove commented-out manual implementation.This commented-out
ZeroCopyNewimplementation is now handled by derive macros and should be removed.
573-604: Remove commented-out manual implementation.This commented-out
ZeroCopyNewimplementation is now handled by derive macros and should be removed.
730-760: Remove commented-out manual implementation.This commented-out
ZeroCopyNewimplementation is now handled by derive macros and should be removed.
🧹 Nitpick comments (4)
program-libs/zero-copy/src/borsh_mut.rs (1)
18-18: Consider renaming the TODO comment for clarity.The TODO comment suggests renaming to
ZeroCopybut this could be confusing since there's already azerocopycrate. Consider a more specific name likeZeroCopyOutputorMutableOutput.program-libs/zero-copy-derive/tests/instruction_data.rs (3)
1133-1138: Remove or uncomment test code.These commented-out lines appear to be part of a test for mutable zero-copy functionality. Either complete the test or remove the commented code to improve clarity.
- // { - // let (mut zero_copy_mut, _remaining) = MyStruct::zero_copy_at_mut(&mut bytes).unwrap(); - // zero_copy_mut.a = 42; - // } - // let borsh = MyStruct::try_from_slice(&bytes).unwrap(); - // assert_eq!(borsh.a, 42u8);
1156-1272: Consider simplifying manual PartialEq implementations.These manual
PartialEqimplementations are quite verbose and contain repetitive comparison patterns. Consider extracting helper functions or using macros to reduce code duplication and improve maintainability.For example, you could create a helper function for Option comparisons:
fn compare_options<T: PartialEq>(left: &Option<T>, right: &Option<T>) -> bool { match (left, right) { (Some(l), Some(r)) => l == r, (None, None) => true, _ => false, } }
1274-1343: Extract common comparison logic.This
PartialEqimplementation shares similar patterns with other manual implementations in the file. The address and data comparison logic is duplicated across multiple implementations.Consider creating shared comparison functions to reduce duplication:
fn compare_compressed_account_address<T, U>(self_addr: &Option<T>, other_addr: &Option<U>) -> bool where T: std::ops::Deref<Target = [u8; 32]>, U: std::ops::Deref<Target = [u8; 32]>, { match (self_addr, other_addr) { (Some(self_a), Some(other_a)) => **self_a == **other_a, (None, None) => true, _ => false, } }
📜 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 (40)
.github/workflows/rust.yml(1 hunks).gitignore(1 hunks)Cargo.toml(2 hunks)program-libs/compressed-account/Cargo.toml(1 hunks)program-libs/compressed-account/src/compressed_account.rs(5 hunks)program-libs/compressed-account/src/instruction_data/compressed_proof.rs(2 hunks)program-libs/compressed-account/src/instruction_data/cpi_context.rs(1 hunks)program-libs/compressed-account/src/instruction_data/data.rs(2 hunks)program-libs/compressed-account/src/instruction_data/invoke_cpi.rs(2 hunks)program-libs/compressed-account/src/instruction_data/with_account_info.rs(1 hunks)program-libs/compressed-account/src/instruction_data/with_readonly.rs(1 hunks)program-libs/compressed-account/src/pubkey.rs(2 hunks)program-libs/hasher/Cargo.toml(2 hunks)program-libs/hasher/src/to_byte_array.rs(1 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/lib.rs(1 hunks)program-libs/zero-copy-derive/src/shared/from_impl.rs(1 hunks)program-libs/zero-copy-derive/src/shared/meta_struct.rs(1 hunks)program-libs/zero-copy-derive/src/shared/mod.rs(1 hunks)program-libs/zero-copy-derive/src/shared/utils.rs(1 hunks)program-libs/zero-copy-derive/src/shared/z_struct.rs(1 hunks)program-libs/zero-copy-derive/src/shared/zero_copy_new.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_eq.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_mut.rs(1 hunks)program-libs/zero-copy-derive/tests/config_test.rs(1 hunks)program-libs/zero-copy-derive/tests/cross_crate_copy.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(4 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
✅ Files skipped from review due to trivial changes (8)
- program-libs/compressed-account/Cargo.toml
- program-libs/hasher/Cargo.toml
- program-libs/compressed-account/src/instruction_data/invoke_cpi.rs
- program-libs/compressed-account/src/instruction_data/cpi_context.rs
- program-libs/compressed-account/src/instruction_data/compressed_proof.rs
- program-libs/compressed-account/src/instruction_data/data.rs
- program-libs/compressed-account/src/compressed_account.rs
- program-libs/zero-copy-derive/src/shared/mod.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- .github/workflows/rust.yml
- .gitignore
- program-libs/compressed-account/src/instruction_data/with_account_info.rs
- program-libs/compressed-account/src/instruction_data/with_readonly.rs
- Cargo.toml
- program-libs/zero-copy/Cargo.toml
- program-libs/zero-copy-derive/src/zero_copy_mut.rs
- program-libs/zero-copy/src/slice_mut.rs
- program-libs/zero-copy-derive/README.md
- program-libs/zero-copy-derive/src/shared/meta_struct.rs
- program-libs/zero-copy-derive/tests/from_test.rs
- program-libs/zero-copy-derive/tests/config_test.rs
- program-libs/zero-copy-derive/src/lib.rs
- program-libs/zero-copy/src/lib.rs
- program-libs/zero-copy/src/init_mut.rs
- program-libs/zero-copy-derive/Cargo.toml
- program-libs/zero-copy-derive/tests/random.rs
- program-libs/zero-copy-derive/src/shared/utils.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
program-libs/zero-copy-derive/src/shared/zero_copy_new.rs (5)
program-libs/zero-copy-derive/src/shared/z_struct.rs (1)
analyze_struct_fields(155-168)program-libs/zero-copy/src/init_mut.rs (25)
byte_len(23-23)byte_len(42-51)byte_len(81-83)byte_len(98-100)byte_len(115-117)byte_len(132-134)byte_len(149-151)byte_len(172-174)byte_len(190-192)byte_len(206-208)byte_len(222-224)byte_len(239-245)config(241-244)new_zero_copy(28-31)new_zero_copy(53-73)new_zero_copy(85-91)new_zero_copy(102-108)new_zero_copy(119-125)new_zero_copy(136-142)new_zero_copy(153-159)new_zero_copy(176-182)new_zero_copy(194-199)new_zero_copy(210-215)new_zero_copy(226-231)new_zero_copy(247-267)program-libs/zero-copy/src/slice_mut.rs (2)
new_at(39-53)zero_copy_at_mut(286-290)program-libs/compressed-account/src/pubkey.rs (2)
byte_len(52-54)new_zero_copy(55-61)program-libs/zero-copy-derive/src/shared/utils.rs (2)
get_vec_inner_type(101-114)convert_to_zerocopy_type(176-217)
program-libs/zero-copy/tests/borsh.rs (3)
program-libs/zero-copy/src/borsh.rs (10)
zero_copy_at(20-20)zero_copy_at(27-30)zero_copy_at(36-49)zero_copy_at(56-62)zero_copy_at(69-75)zero_copy_at(113-116)zero_copy_at(122-140)zero_copy_at(168-178)zero_copy_at(383-386)zero_copy_at(441-445)program-libs/zero-copy/src/borsh_mut.rs (14)
zero_copy_at_mut(20-21)zero_copy_at_mut(29-34)zero_copy_at_mut(41-56)zero_copy_at_mut(63-67)zero_copy_at_mut(74-78)zero_copy_at_mut(86-91)zero_copy_at_mut(97-117)zero_copy_at_mut(186-197)zero_copy_at_mut(404-409)zero_copy_at_mut(467-478)zero_copy_at_mut(531-538)zero_copy_at_mut(569-574)zero_copy_at_mut(627-646)zero_copy_at_mut(716-723)js/stateless.js/src/state/types.ts (1)
CompressedAccountData(199-212)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v1
- GitHub Check: lint
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs-slow
- GitHub Check: cli-v1
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v2
🔇 Additional comments (37)
program-libs/hasher/src/to_byte_array.rs (1)
73-89: Well-designed macro implementation.The macro effectively reduces code duplication and follows the established pattern of delegating to primitive implementations. The approach of converting zero-copy types to primitives and delegating is sound.
program-libs/compressed-account/src/pubkey.rs (2)
3-3: LGTM: Clean import addition for zero-copy functionality.The import of
ZeroCopyNewaligns with the PR's objective to add zero-copy macro capabilities.
49-62: LGTM: Correct ZeroCopyNew implementation for fixed-size Pubkey.The implementation correctly:
- Returns a fixed byte length of 32 for Pubkey
- Uses
zerocopy::Ref::from_prefixfor efficient zero-copy reference creation- Properly handles the slice splitting with error propagation
This follows the expected pattern for implementing ZeroCopyNew on fixed-size types.
program-libs/zero-copy-derive/src/shared/from_impl.rs (3)
85-90: LGTM: Clean Option type handling with helper function.The use of
utils::get_option_inner_typeprovides a clean abstraction for extracting inner types from Option, addressing the complexity mentioned in previous reviews.
12-22: LGTM: Clean generic design for mutable/immutable variants.The const generic
MUTparameter elegantly handles both mutable and immutable zero-copy struct variants by adjusting the struct name with "Mut" suffix when needed.
174-242: LGTM: Comprehensive test coverage for both variants.The tests validate both mutable and immutable From implementations with appropriate field types, ensuring the generic design works correctly.
program-libs/zero-copy-derive/tests/cross_crate_copy.rs (3)
1-9: LGTM: Well-documented test module with clear purpose.The module documentation clearly explains the test's purpose: validating cross-crate Copy identification for both built-in and user-defined types.
58-86: LGTM: Comprehensive validation of field splitting logic.The test correctly validates that primitive Copy types are placed in meta_fields until a Vec is encountered, and then verifies both zero-copy access patterns and field accessor methods work as expected.
140-250: LGTM: Excellent coverage of all derive macros with Vec-first edge case.This test provides comprehensive validation of a challenging edge case (Vec as first field) while testing all derive macros (ZeroCopy, ZeroCopyEq, ZeroCopyMut, ZeroCopyNew) working together correctly.
program-libs/zero-copy-derive/src/shared/z_struct.rs (4)
7-44: LGTM: Comprehensive FieldType enum with proper categorization.The FieldType enum effectively covers all supported field types for zero-copy structs, including special cases like OptionU64/U32/U16 for optimized integer option handling. The
name()method provides clean field name access across all variants.
46-84: LGTM: Well-structured type classification functions.The classification functions properly handle the complexity of determining appropriate zero-copy types for Vec and Option fields, with logical fallbacks for different type categories.
170-318: LGTM: Robust zero-copy field type generation with proper mutability handling.The function correctly handles the complex mapping from original field types to their zero-copy equivalents, properly adjusting for mutable vs immutable variants and using appropriate traits and wrappers for each field type category.
514-629: LGTM: Excellent fuzz testing coverage with comprehensive validation.The fuzz test with 10,000 iterations provides robust validation of the struct generation logic, testing random field combinations and verifying the generated code contains expected patterns. The test properly validates both meta field and non-meta field scenarios.
program-libs/zero-copy-derive/src/shared/zero_copy_new.rs (3)
10-113: LGTM - Well-structured implementation.The
generate_init_mut_implfunction properly separates meta fields from struct fields and generates appropriate initialization code for theZeroCopyNewtrait implementation.
117-180: LGTM - Comprehensive configuration type handling.The configuration system functions properly handle all field types and determine appropriate config types for each variant.
182-218: LGTM - Well-designed config struct generation.The function properly handles both empty and non-empty configuration cases and correctly filters fields based on configuration requirements.
program-libs/zero-copy-derive/src/zero_copy.rs (3)
11-26: LGTM - Clean helper function design.The use of const generics to handle mutability variants is elegant and the function is well-structured.
28-115: LGTM - Comprehensive field type handling.The function properly handles all field types with appropriate deserialization logic and correctly manages mutability variants through const generics.
566-636: LGTM - Well-orchestrated code generation.The remaining implementation functions properly handle trait generation and the main entry point function effectively coordinates all the code generation steps.
program-libs/zero-copy/tests/borsh.rs (6)
1-36: LGTM - Comprehensive test setup and first test case.The feature flags are appropriate and the first test thoroughly validates both immutable and mutable zero-copy operations with proper assertions.
38-94: LGTM - Solid testing of Vec field handling.The tests properly validate zero-copy deserialization of structs with Vec fields and ensure equality with original structs.
96-161: LGTM - Excellent coverage of complex nested structures.The tests effectively validate nested structs and complex vector compositions, which are important edge cases for the zero-copy implementation.
163-235: LGTM - Thorough testing of vectors and options.The tests properly validate vectors of complex types and comprehensive Option handling with both Some and None cases.
237-275: LGTM - Important validation of deep nesting scenarios.The deeply nested struct tests ensure the derive macro implementation handles complex hierarchical structures correctly.
277-335: LGTM - Valuable real-world testing scenarios.The array tests and CompressedAccountData struct provide important validation of fixed-size arrays and realistic use case scenarios.
program-libs/zero-copy/src/borsh_mut.rs (2)
100-109: LGTM: Heap exhaustion protection implemented.Good implementation of DoS protection by validating that remaining bytes are sufficient before allocation. This addresses the security concern from past reviews.
336-965: Comprehensive test coverage demonstrates zero-copy patterns.The extensive manual test implementations effectively demonstrate the expected patterns for zero-copy mutable deserialization. These serve as excellent examples for users implementing the derive macros.
program-libs/zero-copy/src/borsh.rs (7)
8-8: Good addition of signed integer support.Adding I16, I32, I64 to the imports enables zero-copy deserialization for signed integers, providing feature parity with unsigned types.
65-76: Verify bool deserialization semantics.The bool implementation returns
u8as output rather thanbool. Ensure this aligns with your intended semantics - users will need to interpret the byte value as boolean themselves.
93-106: Enhanced macro supports both native and zerocopy types.The updated macro elegantly handles both native types (u16) and their zerocopy equivalents (U16), providing comprehensive coverage for all supported integer types.
108-117: Fixed-size array support enhances type coverage.The generic implementation for
[T; N]arrays is well-designed and leverages the zerocopy crate's safety guarantees appropriately.
125-132: Heap exhaustion protection successfully implemented.The DoS protection validates remaining bytes before allocation, preventing malicious inputs from causing excessive memory allocation.
181-219: ZeroCopyStructInner trait provides clean type associations.This trait elegantly maps native types to their zero-copy representations, enabling generic programming while maintaining type safety.
221-228: Utility function includes proper bounds checking.The
borsh_vec_u8_as_slicefunction correctly validates the length prefix against available bytes before splitting, preventing buffer overruns.program-libs/zero-copy/tests/borsh_2.rs (3)
1-1: Appropriate feature gating for derive macro tests.The feature gates correctly limit these tests to configurations where both standard library and derive macros are available.
14-29: Excellent documentation of zero-copy design patterns.The detailed comments explaining the rules for zero-copy struct design provide valuable guidance for users implementing these patterns manually or understanding the derive macro behavior.
33-559: Comprehensive test coverage validates zero-copy patterns.The test implementations cover a wide range of scenarios including nested structs, vectors, options, and mixed types. This provides excellent validation of the zero-copy deserialization framework.
| // ToByteArray implementations for zero-copy types | ||
| #[cfg(feature = "zero-copy")] | ||
| impl_to_byte_array_for_zero_copy_type!(zerocopy::little_endian::U16, u16); | ||
| #[cfg(feature = "zero-copy")] | ||
| impl_to_byte_array_for_zero_copy_type!(zerocopy::little_endian::U32, u32); | ||
| #[cfg(feature = "zero-copy")] | ||
| impl_to_byte_array_for_zero_copy_type!(zerocopy::little_endian::U64, u64); | ||
| #[cfg(feature = "zero-copy")] | ||
| impl_to_byte_array_for_zero_copy_type!(zerocopy::little_endian::I16, i16); | ||
| #[cfg(feature = "zero-copy")] | ||
| impl_to_byte_array_for_zero_copy_type!(zerocopy::little_endian::I32, i32); | ||
| #[cfg(feature = "zero-copy")] | ||
| impl_to_byte_array_for_zero_copy_type!(zerocopy::little_endian::I64, i64); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for zero-copy types and verify dependency configuration.
The implementations look correct, but there are no tests covering the new zero-copy types. Additionally, ensure the zerocopy crate dependency is properly configured when the zero-copy feature is enabled.
Add test coverage for the zero-copy types:
+ #[test]
+ #[cfg(feature = "zero-copy")]
+ fn test_to_byte_array_zero_copy_types() {
+ use zerocopy::little_endian::{U16, U32, U64, I16, I32, I64};
+
+ // Test U16
+ let val = U16::new(0x1234);
+ let result = val.to_byte_array().unwrap();
+ let expected = 0x1234u16.to_byte_array().unwrap();
+ assert_eq!(result, expected);
+
+ // Test I32
+ let val = I32::new(-42);
+ let result = val.to_byte_array().unwrap();
+ let expected = (-42i32).to_byte_array().unwrap();
+ assert_eq!(result, expected);
+
+ // Add similar tests for other types...
+ }Verify the zerocopy dependency configuration:
🏁 Script executed:
#!/bin/bash
# Description: Verify zerocopy dependency is properly configured for the zero-copy feature
# Check if zerocopy is listed as a dependency in Cargo.toml
rg -A 5 -B 5 "zerocopy.*=" program-libs/hasher/Cargo.toml
# Check if the zero-copy feature properly enables zerocopy
rg -A 10 -B 5 '\[features\]' program-libs/hasher/Cargo.tomlLength of output: 989
Add test coverage for zero-copy ToByteArray implementations
The zerocopy crate is already correctly declared as an optional workspace dependency and enabled by the zero-copy feature. However, there are no unit tests covering the new impl_to_byte_array_for_zero_copy_type! implementations. Please add tests to ensure they behave identically to the primitive equivalents.
• In program-libs/hasher/src/to_byte_array.rs (or in a dedicated tests/ module), add a #[cfg(feature = "zero-copy")] test such as:
+#[cfg(test)]
+mod tests {
+ use super::*;
+ #[test]
+ #[cfg(feature = "zero-copy")]
+ fn test_to_byte_array_zero_copy_types() {
+ use zerocopy::little_endian::{U16, U32, U64, I16, I32, I64};
+
+ // U16
+ let z = U16::new(0x1234);
+ assert_eq!(z.to_byte_array().unwrap(), 0x1234u16.to_byte_array().unwrap());
+
+ // U32
+ let z = U32::new(0xDEAD_BEEFu32);
+ assert_eq!(z.to_byte_array().unwrap(), 0xDEAD_BEEFu32.to_byte_array().unwrap());
+
+ // U64
+ let z = U64::new(0x0123_4567_89AB_CDEFu64);
+ assert_eq!(z.to_byte_array().unwrap(), 0x0123_4567_89AB_CDEFu64.to_byte_array().unwrap());
+
+ // I16
+ let z = I16::new(-0x1234);
+ assert_eq!(z.to_byte_array().unwrap(), (-0x1234i16).to_byte_array().unwrap());
+
+ // I32
+ let z = I32::new(-42);
+ assert_eq!(z.to_byte_array().unwrap(), (-42i32).to_byte_array().unwrap());
+
+ // I64
+ let z = I64::new(-0x0FED_CBA9_8765_4321i64);
+ assert_eq!(z.to_byte_array().unwrap(), (-0x0FED_CBA9_8765_4321i64).to_byte_array().unwrap());
+ }
+}This will ensure full coverage of all zero-copy types.
🤖 Prompt for AI Agents
In program-libs/hasher/src/to_byte_array.rs around lines 91 to 103, add unit
tests under a #[cfg(feature = "zero-copy")] module to cover the
impl_to_byte_array_for_zero_copy_type! macro implementations for zerocopy types.
The tests should verify that the zero-copy ToByteArray implementations produce
the same byte arrays as their corresponding primitive types, ensuring behavior
consistency and full test coverage.
| FieldType::VecCopy(field_name, inner_type) => { | ||
| quote! { | ||
| (4 + (config.#field_name as usize * core::mem::size_of::<#inner_type>())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential arithmetic overflow in VecCopy byte length calculation
The arithmetic operations could overflow if config values are large. Consider using checked arithmetic:
- (4 + (config.#field_name as usize * core::mem::size_of::<#inner_type>()))
+ 4usize.checked_add(
+ (config.#field_name as usize).checked_mul(core::mem::size_of::<#inner_type>())
+ .ok_or(light_zero_copy::errors::ZeroCopyError::InvalidCapacity)?
+ ).ok_or(light_zero_copy::errors::ZeroCopyError::InvalidCapacity)?📝 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.
| FieldType::VecCopy(field_name, inner_type) => { | |
| quote! { | |
| (4 + (config.#field_name as usize * core::mem::size_of::<#inner_type>())) | |
| } | |
| } | |
| FieldType::VecCopy(field_name, inner_type) => { | |
| quote! { | |
| 4usize.checked_add( | |
| (config.#field_name as usize) | |
| .checked_mul(core::mem::size_of::<#inner_type>()) | |
| .ok_or(light_zero_copy::errors::ZeroCopyError::InvalidCapacity)? | |
| ) | |
| .ok_or(light_zero_copy::errors::ZeroCopyError::InvalidCapacity)? | |
| } | |
| } |
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/src/shared/zero_copy_new.rs around lines 320 to
324, the calculation for VecCopy byte length uses unchecked arithmetic which can
overflow with large config values. Modify the code to use checked arithmetic
methods like checked_mul and checked_add to safely compute the byte length,
handling potential overflow cases appropriately.
| // FieldType::Bool(field_name) => { | ||
| // quote! { | ||
| // #(#attributes)* | ||
| // pub #field_name: <bool as #trait_name<'a>>::Output | ||
| // } | ||
| // } |
| } | ||
|
|
||
| // #[cfg(test)] | ||
| // mod tests { |
There was a problem hiding this comment.
tests commented - leftover?
ananas-block
force-pushed
the
jorrit/refactor-zero-copy-borsh
branch
from
95ebdc0 to
ce59fa2
Compare
| pub e: bool, | ||
| } | ||
|
|
||
| // use light_zero_copy::borsh_mut::DeserializeMut; // Not needed for non-mut derivations |
There was a problem hiding this comment.
leftover? (also the commented asserts below)
ananas-block
force-pushed
the
jorrit/refactor-zero-copy-borsh
branch
from
76884ef to
59f1438
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
program-libs/zero-copy-derive/src/lib.rs (1)
130-166: Commented-out ZeroCopyNew macro may cause test failures.The
ZeroCopyNewderive macro is commented out but based on the past review comment and the extensive documentation, tests may be expecting this macro to generate config structs.Either implement the macro, remove/disable the failing tests, or add a clear TODO comment explaining the plan for this macro.
program-libs/zero-copy-derive/src/shared/zero_copy_new.rs (5)
233-235: Potential integer overflow in split operationsConverting
config.#field_nametousizewithout bounds checking could cause issues if the config value exceedsusize::MAXon 32-bit systems.
241-244: Unsafe integer conversion in Vec initializationThe
.into()conversion from u32 to the slice's length type happens without bounds checking.
301-305: Issue: Incomplete field initialization for Copy typesThe
FieldType::Copycase is missing the required config parameter in thenew_zero_copycall.
316-317: Potential arithmetic overflow in byte length calculationThe arithmetic operations could overflow if config values are large.
320-324: Potential arithmetic overflow in VecCopy byte length calculationThe arithmetic operations could overflow if config values are large. Consider using checked arithmetic.
program-libs/zero-copy/src/borsh_mut.rs (1)
189-189: Document VecU8's 1-byte length prefixThe VecU8 zero-copy implementation always reads its element count from a single u8 (max 255), whereas the standard Vec uses a U32 prefix. If this 255-element cap is intentional, please add a doc comment.
program-libs/zero-copy/tests/borsh_2.rs (1)
266-269: Inconsistent method usage - address the previously flagged issue.Lines 266 and 269 use
ZeroCopySliceBorsh::from_bytes_atwhile line 184 usesZeroCopySliceBorsh::zero_copy_at. This inconsistency was previously identified and should be standardized.program-libs/zero-copy-derive/tests/random.rs (1)
41-174: Significant code duplication between populate functions.The
populate_invoke_cpi_zero_copyandpopulate_invoke_zero_copyfunctions contain nearly identical code with ~95% duplication, as previously identified. This violates the DRY principle and should be refactored.Also applies to: 176-300
program-libs/zero-copy-derive/tests/instruction_data.rs (1)
98-183: Remove commented-out ZeroCopyNew implementation.This large block of commented-out manual implementation was already flagged in previous reviews and should be removed to improve code maintainability.
🧹 Nitpick comments (9)
program-libs/zero-copy-derive/src/shared/from_impl.rs (1)
85-101: Simplify the Option field conversion logic.The current implementation for
FieldType::Optioncreates a complex clone chain that could be simplified and made more readable.- quote! { - #field_name: if value.#field_name.is_some() { - // 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 - Some((#field_type::from(*value.#field_name.as_ref().unwrap()).clone())) - } else { - None - }, - } + quote! { + #field_name: value.#field_name.as_ref().map(|v| #field_type::from(**v)), + }program-libs/zero-copy-derive/tests/cross_crate_copy.rs (1)
1-1: Consider the scope of feature gating.The entire test file is gated behind
#![cfg(feature = "mut")], but some tests only exerciseZeroCopyandZeroCopyEqwhich don't require themutfeature. Consider splitting the tests or using more granular feature gating.Move non-mutable tests outside the feature gate:
-#![cfg(feature = "mut")] //! Test cross-crate Copy identification functionality //! //! This test validates that the zero-copy derive macro correctly identifies //! which types implement Copy, both for built-in types and user-defined types. use borsh::{BorshDeserialize, BorshSerialize}; -use light_zero_copy_derive::{ZeroCopy, ZeroCopyEq, ZeroCopyMut}; +use light_zero_copy_derive::{ZeroCopy, ZeroCopyEq}; +#[cfg(feature = "mut")] +use light_zero_copy_derive::ZeroCopyMut;program-libs/zero-copy/src/init_mut.rs (1)
77-92: Consider consistency in return type patterns.The
u64implementation returnszerocopy::Ref<&'a mut [u8], zerocopy::little_endian::U64>whileu8(lines 128-143) returns<Self as crate::borsh_mut::DeserializeMut<'a>>::Output. Consider using a consistent pattern across all primitive types.Either use the explicit zerocopy type for all primitives or use the trait-based approach consistently.
program-libs/zero-copy-derive/src/lib.rs (1)
1-12: Documentation mentions ZeroCopyNew but it's not available.The crate documentation at the top mentions
ZeroCopyNewas one of the main macros, but it's commented out below.//! ## Main Macros //! //! - `ZeroCopy`: Generates zero-copy structs and deserialization traits //! - `ZeroCopyMut`: Adds mutable zero-copy support //! - `ZeroCopyEq`: Adds PartialEq implementation for comparing with original structs -//! - `ZeroCopyNew`: Generates configuration structs for initialization +//! - `ZeroCopyNew`: Generates configuration structs for initialization (planned)program-libs/zero-copy-derive/src/shared/z_struct.rs (2)
180-190: Conditional attribute handling could be simplified.The conditional logic for attributes creates an empty quote when
hasheris false, which is unnecessary complexity.- let attributes = if *hasher { - field - .attrs - .iter() - .map(|attr| { - quote! { #attr } - }) - .collect::<Vec<_>>() - } else { - vec![quote! {}] - }; + let attributes: Vec<_> = if *hasher { + field.attrs.iter().map(|attr| quote! { #attr }).collect() + } else { + vec![] + };
228-241: Complex conditional logic for VecCopy field generation.The nested conditional logic for handling primitive vs. complex Copy types could be extracted into a helper function for better readability.
Extract the type conversion logic into a helper function:
fn generate_vec_copy_field(field_name: &Ident, inner_type: &Type, slice_name: &syn::Type, struct_inner_trait_name: &syn::Type, attributes: &[TokenStream]) -> TokenStream { if utils::is_primitive_integer(inner_type) || utils::is_bool_type(inner_type) || utils::is_pubkey_type(inner_type) { let zerocopy_type = utils::convert_to_zerocopy_type(inner_type); quote! { #(#attributes)* pub #field_name: #slice_name<'a, #zerocopy_type> } } else { let inner_type = utils::convert_to_zerocopy_type(inner_type); quote! { #(#attributes)* pub #field_name: #slice_name<'a, <#inner_type as #struct_inner_trait_name>> } } }program-libs/zero-copy/src/borsh_mut.rs (1)
176-180: Inconsistent DerefMut implementation for VecU8
DerefMutis only implemented forVecU8<u8>butDerefis implemented for genericVecU8<T>. This inconsistency may be intentional but seems unusual - consider implementingDerefMutfor the generic case or documenting why it's restricted.program-libs/zero-copy/src/borsh.rs (2)
65-76: Consider clarifying the bool deserialization design.The
boolimplementation returnsu8as its output type rather thanbool. While this may be intentional for zero-copy efficiency, it could be confusing to users expecting a boolean value.Consider adding documentation to clarify why
booldeserialization returnsu8:+/// Deserializes a bool as a single byte (u8) for zero-copy efficiency. +/// Use pattern matching (0 = false, 1 = true) to interpret the value. impl<'a> Deserialize<'a> for bool { type Output = u8;
317-873: Consider refactoring repetitive test code patterns.The test module contains extensive manual implementations that demonstrate zero-copy patterns, but there's significant code duplication across the struct implementations (ZStruct1-8 follow very similar patterns).
Consider extracting common test patterns into helper macros or functions to reduce duplication while maintaining comprehensive test coverage. This would make the tests more maintainable and easier to extend.
📜 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 (31)
.github/workflows/rust.yml(1 hunks).gitignore(1 hunks)Cargo.toml(2 hunks)program-libs/compressed-account/src/instruction_data/with_account_info.rs(1 hunks)program-libs/compressed-account/src/instruction_data/with_readonly.rs(1 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/lib.rs(1 hunks)program-libs/zero-copy-derive/src/shared/from_impl.rs(1 hunks)program-libs/zero-copy-derive/src/shared/meta_struct.rs(1 hunks)program-libs/zero-copy-derive/src/shared/mod.rs(1 hunks)program-libs/zero-copy-derive/src/shared/utils.rs(1 hunks)program-libs/zero-copy-derive/src/shared/z_struct.rs(1 hunks)program-libs/zero-copy-derive/src/shared/zero_copy_new.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_eq.rs(1 hunks)program-libs/zero-copy-derive/src/zero_copy_mut.rs(1 hunks)program-libs/zero-copy-derive/tests/config_test.rs(1 hunks)program-libs/zero-copy-derive/tests/cross_crate_copy.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(4 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
✅ Files skipped from review due to trivial changes (1)
- program-libs/zero-copy-derive/src/shared/mod.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- .gitignore
- .github/workflows/rust.yml
- program-libs/compressed-account/src/instruction_data/with_account_info.rs
- program-libs/zero-copy/Cargo.toml
- program-libs/zero-copy-derive/src/zero_copy_mut.rs
- Cargo.toml
- program-libs/compressed-account/src/instruction_data/with_readonly.rs
- program-libs/zero-copy-derive/src/shared/meta_struct.rs
- program-libs/zero-copy/src/slice_mut.rs
- program-libs/zero-copy-derive/Cargo.toml
- program-libs/zero-copy-derive/README.md
- program-libs/zero-copy-derive/src/zero_copy_eq.rs
- program-libs/zero-copy-derive/tests/config_test.rs
- program-libs/zero-copy-derive/tests/from_test.rs
- program-libs/zero-copy/src/lib.rs
- program-libs/zero-copy-derive/src/shared/utils.rs
🧰 Additional context used
🧬 Code Graph Analysis (4)
program-libs/zero-copy/src/init_mut.rs (2)
program-libs/zero-copy/src/borsh_mut.rs (16)
size_of(44-44)zero_copy_at_mut(20-21)zero_copy_at_mut(29-34)zero_copy_at_mut(41-56)zero_copy_at_mut(63-67)zero_copy_at_mut(74-78)zero_copy_at_mut(86-91)zero_copy_at_mut(97-117)zero_copy_at_mut(186-197)zero_copy_at_mut(404-409)zero_copy_at_mut(467-478)zero_copy_at_mut(531-538)zero_copy_at_mut(569-574)zero_copy_at_mut(627-646)zero_copy_at_mut(716-723)new(164-166)program-libs/zero-copy-derive/tests/instruction_data.rs (3)
byte_len(72-74)new_zero_copy(76-81)zero_copy_at_mut(52-56)
program-libs/zero-copy-derive/src/shared/z_struct.rs (4)
program-libs/zero-copy-derive/src/shared/utils.rs (11)
is_specific_primitive_type(158-165)is_copy_type(276-315)is_primitive_integer(131-146)is_vec_type(83-90)get_vec_inner_type(101-114)is_option_type(92-99)get_option_inner_type(116-129)is_pubkey_type(167-174)is_bool_type(148-155)convert_to_zerocopy_type(176-217)process_fields(58-77)program-libs/zero-copy-derive/src/shared/meta_struct.rs (1)
field(24-30)program-libs/zero-copy-derive/src/zero_copy.rs (5)
generate_z_struct(442-442)random_ident(206-215)random_type(218-240)random_field(243-250)random_fields(253-255)program-libs/zero-copy-derive/src/zero_copy_mut.rs (1)
z_struct(32-32)
program-libs/zero-copy/tests/borsh.rs (5)
program-libs/zero-copy/tests/borsh_2.rs (9)
zero_copy_at(67-70)zero_copy_at(125-129)zero_copy_at(182-187)zero_copy_at(264-279)zero_copy_at(320-326)zero_copy_at(360-363)zero_copy_at(438-442)zero_copy_at(497-501)zero_copy_at(519-522)program-libs/zero-copy/src/borsh.rs (3)
zero_copy_at(20-20)zero_copy_at(27-30)zero_copy_at(36-49)program-libs/zero-copy/src/borsh_mut.rs (14)
zero_copy_at_mut(20-21)zero_copy_at_mut(29-34)zero_copy_at_mut(41-56)zero_copy_at_mut(63-67)zero_copy_at_mut(74-78)zero_copy_at_mut(86-91)zero_copy_at_mut(97-117)zero_copy_at_mut(186-197)zero_copy_at_mut(404-409)zero_copy_at_mut(467-478)zero_copy_at_mut(531-538)zero_copy_at_mut(569-574)zero_copy_at_mut(627-646)zero_copy_at_mut(716-723)program-libs/zero-copy-derive/src/zero_copy_mut.rs (2)
zero_copy(40-40)zero_copy(45-45)js/stateless.js/src/state/types.ts (1)
CompressedAccountData(199-212)
program-libs/zero-copy-derive/src/zero_copy.rs (7)
program-libs/zero-copy-derive/src/zero_copy_mut.rs (2)
meta_struct(27-27)z_struct(32-32)program-libs/zero-copy-derive/src/shared/z_struct.rs (4)
generate_z_struct(342-449)generate_z_struct(539-539)field(181-187)name(28-43)program-libs/zero-copy-derive/src/shared/utils.rs (3)
convert_to_zerocopy_type(176-217)process_fields(58-77)process_input(22-56)program-libs/zero-copy-derive/tests/instruction_data.rs (2)
zero_copy_at_mut(52-56)zero_copy_at(43-45)program-libs/zero-copy/src/borsh_mut.rs (15)
zero_copy_at_mut(20-21)zero_copy_at_mut(29-34)zero_copy_at_mut(41-56)zero_copy_at_mut(63-67)zero_copy_at_mut(74-78)zero_copy_at_mut(86-91)zero_copy_at_mut(97-117)zero_copy_at_mut(186-197)zero_copy_at_mut(404-409)zero_copy_at_mut(467-478)zero_copy_at_mut(531-538)zero_copy_at_mut(569-574)zero_copy_at_mut(627-646)zero_copy_at_mut(716-723)borsh_vec_u8_as_slice_mut(150-159)program-libs/zero-copy/src/borsh.rs (4)
zero_copy_at(20-20)zero_copy_at(27-30)zero_copy_at(36-49)borsh_vec_u8_as_slice(221-228)program-libs/zero-copy-derive/src/shared/meta_struct.rs (2)
field(24-30)generate_meta_struct(9-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test sdk-libs
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v2
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (31)
program-libs/zero-copy-derive/tests/cross_crate_copy.rs (1)
197-221: Mutable zero-copy test logic needs correction.The mutable zero-copy test calls
zero_copy_atinstead ofzero_copy_at_muton line 209, which doesn't test the mutable functionality properly.- let (deserialized_mut, _) = - PrimitiveCopyStruct2::zero_copy_at(&mut serialized_mut).unwrap(); + let (deserialized_mut, _) = + PrimitiveCopyStruct2::zero_copy_at_mut(&mut serialized_mut).unwrap();Likely an incorrect or invalid review comment.
program-libs/zero-copy/src/init_mut.rs (1)
57-59: Bounds check properly addresses the previous review concern.The implementation correctly adds a bounds check before writing to
bytes[0], which addresses the previous review comment about potential panics.program-libs/zero-copy-derive/src/shared/z_struct.rs (2)
509-623: Excellent fuzz testing implementation.The fuzz testing approach with 10,000 iterations and comprehensive validation is well-implemented and provides good confidence in the code generation logic.
428-436: Meta field boolean accessor methods use hardcoded mutability.The boolean accessor methods for meta fields always use
generate_bool_accessor_methods::<false>regardless of theMUTparameter, which might be intentional but should be verified.Verify that meta fields should always use immutable boolean accessors even in mutable zero-copy structs, or if this should respect the
MUTparameter.program-libs/zero-copy-derive/src/shared/zero_copy_new.rs (1)
182-218: Well-structured configuration generationThe configuration struct generation logic is well-designed, properly filtering fields that require configuration and handling empty config structs gracefully.
program-libs/zero-copy-derive/src/zero_copy.rs (4)
11-26: Clean const generic design for mutability handlingThe use of const generic
MUTparameter to handle both mutable and immutable deserialization cases is an elegant solution that avoids code duplication.
28-115: Comprehensive field type handlingThe field deserialization logic thoroughly covers all supported field types with appropriate zero-copy implementations for each case.
281-399: Excellent fuzz test implementationThe fuzz test with deterministic seeding and comprehensive validation provides robust testing of the procedural macro generation. The 10,000 iterations with detailed validation checks ensure correctness across a wide range of inputs.
424-471: Well-orchestrated procedural macro implementationThe main derive implementation properly coordinates all generation steps (meta struct, z-struct, inner trait, and deserialize impl) with consistent error handling throughout.
program-libs/zero-copy/tests/borsh.rs (4)
1-6: Proper feature gating for comprehensive testingThe feature gates ensure tests run only when all required features (
std,derive,mut) are available, and the imports cover all necessary zero-copy traits.
14-36: Comprehensive basic struct testingThe test covers both immutable and mutable zero-copy access, verifies field values, equality checks, and that mutations persist correctly through deserialization.
49-64: Thorough vector testing coverageThe tests comprehensively cover various vector scenarios: simple
Vec<u8>, nestedVec<Vec<u8>>, and vectors of structs, ensuring zero-copy handling works correctly for dynamic content.Also applies to: 147-161, 170-198
208-235: Complete Option type testingThe test properly covers both
SomeandNonecases for optional fields, ensuring correct zero-copy deserialization of optional data.program-libs/zero-copy/src/borsh_mut.rs (5)
14-22: Clean trait design for mutable deserializationThe
DeserializeMuttrait follows good Rust patterns with an associatedOutputtype and clear method signature for zero-copy mutable deserialization.
94-118: Good DoS protection in Vec deserializationThe implementation includes proper bounds checking to prevent heap exhaustion attacks by validating that remaining bytes are sufficient before allocation.
120-148: Efficient macro for primitive type implementationsThe macro cleanly implements
DeserializeMutfor all primitive types, covering both native types and their zerocopy equivalents while avoiding code duplication.
200-236: Well-designed type mapping traitThe
ZeroCopyStructInnerMuttrait and its implementations provide a clean mapping between regular types and their zero-copy mutable equivalents, supporting primitives, collections, and arrays.
238-335: Excellent test coverage for mutable deserializationThe unit tests comprehensively cover all implementations including edge cases, error conditions, and proper validation of zero-copy mutable behavior.
program-libs/zero-copy/src/borsh.rs (5)
8-8: LGTM: Good addition of signed integer support.Adding signed little-endian integer types (I16, I32, I64) enhances the zero-copy deserialization capabilities and maintains consistency with unsigned types.
78-106: LGTM: Well-designed macro enhancement.The enhanced macro now accepts pairs of native and zerocopy types, enabling comprehensive support for both unsigned and signed integers. The implementation correctly handles both native types and their zerocopy equivalents.
109-117: LGTM: Good addition of array support.The fixed-size array implementation follows the established pattern and correctly leverages zerocopy's
FromBytestrait for safe deserialization.
125-132: Excellent security improvement with heap exhaustion protection.Adding the length check before allocating the vector prevents potential DoS attacks through memory exhaustion. The validation ensures that the declared number of elements can be satisfied by the remaining bytes.
221-228: LGTM: Useful utility function with proper validation.The
borsh_vec_u8_as_slicefunction provides a convenient way to parse Borsh-encoded byte vectors with appropriate length validation.program-libs/zero-copy/tests/borsh_2.rs (2)
14-30: Excellent documentation of zero-copy design rules.The comprehensive comment block clearly explains the design patterns and rules for implementing zero-copy structs, which will be valuable for developers using the derive macros.
1-12: LGTM: Proper feature gating and imports.The feature gates ensure the tests only run with the required features enabled, and the imports are well-organized.
program-libs/zero-copy-derive/tests/random.rs (3)
504-593: Excellent randomized testing approach.The test uses proper randomization with seeded RNG for reproducibility, comprehensive iteration count, and good progress logging. The configuration-driven approach effectively validates the zero-copy derive macros.
595-651: LGTM: Comprehensive CPI instruction testing.The CPI test follows the same robust pattern as the invoke test, with appropriate iteration count and thorough validation of round-trip functionality.
302-384: Well-designed random data generators.The random data generation functions create realistic test scenarios with appropriate constraints and proper use of the RNG for deterministic testing.
program-libs/zero-copy-derive/tests/instruction_data.rs (3)
628-656: LGTM! Option comparison logic is now correct.The PartialEq implementation properly handles Option comparisons by checking both inequality when both are Some and presence mismatch cases.
762-1139: Excellent test coverage for zero-copy functionality.The test suite comprehensively validates zero-copy construction, field mutation, deserialization, and various configuration scenarios. The tests properly verify exact buffer usage and field access patterns.
1156-1401: Well-implemented cross-type PartialEq comparisons.The PartialEq implementations correctly handle complex comparisons between owned and zero-copy reference types, with proper type conversions and Option handling throughout.
| impl<'a> From<&#z_struct_name<'a>> for #name { | ||
| fn from(value: &#z_struct_name<'a>) -> Self { | ||
| Self { | ||
| #(#all_field_conversions)* | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Both From implementations use identical conversion logic.
The two From implementations (consuming and reference) use the same #all_field_conversions tokens, which means they both handle the value the same way. This could lead to incorrect behavior for the reference version.
Consider differentiating the conversion logic for reference vs. owned conversions, or verify that the generated field conversions correctly handle both cases.
🤖 Prompt for AI Agents
In program-libs/zero-copy-derive/src/shared/from_impl.rs around lines 163 to
169, the From implementations for owned and reference types use the same field
conversion logic (#all_field_conversions), which may cause incorrect behavior
for the reference version. Review and separate the conversion logic so that the
owned From implementation consumes the value appropriately, while the reference
From implementation correctly handles references without consuming. Adjust the
generated field conversions to reflect these differences.
| pub trait ZeroCopyStructInner { | ||
| type ZeroCopyInner; | ||
| } | ||
|
|
||
| impl ZeroCopyStructInner for u64 { | ||
| type ZeroCopyInner = U64; | ||
| } | ||
| impl ZeroCopyStructInner for u32 { | ||
| type ZeroCopyInner = U32; | ||
| } | ||
| impl ZeroCopyStructInner for u16 { | ||
| type ZeroCopyInner = U16; | ||
| } | ||
| impl ZeroCopyStructInner for u8 { | ||
| type ZeroCopyInner = u8; | ||
| } | ||
|
|
||
| impl ZeroCopyStructInner for U16 { | ||
| type ZeroCopyInner = U16; | ||
| } | ||
| impl ZeroCopyStructInner for U32 { | ||
| type ZeroCopyInner = U32; | ||
| } | ||
| impl ZeroCopyStructInner for U64 { | ||
| type ZeroCopyInner = U64; | ||
| } | ||
|
|
||
| impl<T: ZeroCopyStructInner + Copy> ZeroCopyStructInner for Vec<T> { | ||
| type ZeroCopyInner = Vec<T::ZeroCopyInner>; | ||
| } | ||
|
|
||
| impl<T: ZeroCopyStructInner + Copy> ZeroCopyStructInner for Option<T> { | ||
| type ZeroCopyInner = Option<T::ZeroCopyInner>; | ||
| } | ||
|
|
||
| // Add ZeroCopyStructInner for array types | ||
| impl<const N: usize> ZeroCopyStructInner for [u8; N] { | ||
| type ZeroCopyInner = Ref<&'static [u8], [u8; N]>; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify completeness of ZeroCopyStructInner implementations.
The ZeroCopyStructInner trait provides essential type mapping but appears to be missing implementations for signed integers (i16, i32, i64).
🏁 Script executed:
#!/bin/bash
# Check if signed integer implementations are defined elsewhere
rg -A 5 "impl ZeroCopyStructInner for i(16|32|64)"Length of output: 52
Implement ZeroCopyStructInner for signed integer types
The ZeroCopyStructInner trait currently covers unsigned types and their wrappers but is missing implementations for signed integers. Please add the following (or equivalent) to ensure completeness:
• impl ZeroCopyStructInner for i8 { type ZeroCopyInner = i8; }
• impl ZeroCopyStructInner for i16 { type ZeroCopyInner = I16; }
• impl ZeroCopyStructInner for i32 { type ZeroCopyInner = I32; }
• impl ZeroCopyStructInner for i64 { type ZeroCopyInner = I64; }
(Adjust wrapper names—e.g. I16, I32, I64—to match your existing signed‐integer types.)
🤖 Prompt for AI Agents
In program-libs/zero-copy/src/borsh.rs between lines 181 and 219, the
ZeroCopyStructInner trait implementations cover unsigned integer types but lack
implementations for signed integer types. Add impl blocks for i8, i16, i32, and
i64, setting their ZeroCopyInner associated types to the corresponding signed
wrapper types (e.g., I16, I32, I64) consistent with existing naming conventions.
This will complete the trait coverage for all standard integer types.
| // pub fn data_hash_struct_1(a: u8, b: u16) -> [u8; 32] { | ||
|
|
||
| // } |
Summary by CodeRabbit
Summary by CodeRabbit
New Features
light-zero-copy-deriveprocedural macro crate, enabling automatic zero-copy deserialization, mutable deserialization, and equality comparisons for Rust structs.Bug Fixes
Documentation
Tests
Chores