feature: Binary transcoding via trait blanket impl #165
Conversation
WalkthroughThe changes in this pull request involve a comprehensive update to the serialization and deserialization methods across various modules, primarily transitioning from the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
crates/da/src/lib.rs (1)
40-42: Consider error handling improvementWhile the error handling is functional, consider using a custom error type for better error categorization.
- .map_err(|e| anyhow::anyhow!("Failed to serialize epoch: {}", e))?; + .map_err(|e| SerializationError::EncodeError(e.to_string()))?;This would require defining a custom error enum:
#[derive(Debug, thiserror::Error)] pub enum SerializationError { #[error("Failed to encode epoch: {0}")] EncodeError(String), // ... other error variants }crates/storage/src/redis.rs (1)
136-137: LGTM! Consider batch encoding optimization.The serialization changes are correct. For future optimization, consider implementing batch encoding if the trait supports it.
crates/da/src/celestia.rs (1)
24-28: Verify error handling for decode_from_bytesThe error handling has been simplified from using
contextto usingmap_err. While this works, consider preserving the original error context for better debugging.- FinalizedEpoch::decode_from_bytes(&value.data).map_err(|_| { + FinalizedEpoch::decode_from_bytes(&value.data).context("Failed to decode blob").map_err(|e| { anyhow!(format!( - "Failed to decode blob into FinalizedEpoch: {value:?}" + "Failed to decode blob into FinalizedEpoch: {value:?} - {e}" )) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/common/src/hashchain.rs(3 hunks)crates/common/src/transaction.rs(2 hunks)crates/common/src/tree/key_directory_tree.rs(1 hunks)crates/common/src/tree/proofs.rs(4 hunks)crates/common/src/tree/snarkable_tree.rs(4 hunks)crates/da/Cargo.toml(1 hunks)crates/da/src/celestia.rs(4 hunks)crates/da/src/lib.rs(3 hunks)crates/serde/Cargo.toml(1 hunks)crates/serde/src/lib.rs(1 hunks)crates/storage/Cargo.toml(1 hunks)crates/storage/src/redis.rs(4 hunks)
🧰 Additional context used
📓 Learnings (1)
crates/common/src/tree/proofs.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-12T11:47:59.930Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
🪛 GitHub Check: clippy
crates/serde/src/lib.rs
[failure] 3-3:
file not found for module binary
🪛 GitHub Check: unit-test
crates/serde/src/lib.rs
[failure] 3-3:
file not found for module binary
🪛 GitHub Check: integration-test
crates/serde/src/lib.rs
[failure] 3-3:
file not found for module binary
🔇 Additional comments (19)
crates/serde/Cargo.toml (1)
13-18: LGTM: Clear dependency organization
Good organization of dependencies into logical sections (serde, binary) improves maintainability.
crates/common/src/transaction.rs (2)
2-2: LGTM: Clean transition to BinaryTranscodable
The change aligns well with the PR objective of transitioning from bincode to the new trait-based approach.
Also applies to: 17-17
2-2: Verify error handling coverage
While the transition to decode_from_bytes simplifies the code, please ensure that all previous error cases handled by bincode::deserialize are properly covered by the new implementation.
Also applies to: 17-17
✅ Verification successful
Let me gather more information about the error handling implementation.
Let me check one more thing to understand the BinaryTranscodable implementation details.
Based on the gathered information, I can now provide the final response:
Error handling appears properly maintained in the transition
The change from bincode::deserialize to BinaryTranscodable::decode_from_bytes maintains proper error handling:
- The
TryFrom<&Blob>implementation forTransactioncontinues to useanyhow::Error - The error propagation is preserved through the
?operator in the implementation - Other usages of
decode_from_bytesacross the codebase show consistent error handling patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the BinaryTranscodable implementation for Transaction
ast-grep --pattern 'impl BinaryTranscodable for Transaction {
$$$
}'
# Search for error handling in decode_from_bytes implementations
rg -A 5 "decode_from_bytes"
Length of output: 2834
Script:
#!/bin/bash
# Search for BinaryTranscodable trait definition to understand expected error handling
ast-grep --pattern 'trait BinaryTranscodable {
$$$
}'
# Look for Transaction implementation details
rg -A 10 "impl.*Transaction"
# Search for anyhow error handling in Transaction
rg -B 2 -A 5 "anyhow|Error" crates/common/src/transaction.rs
Length of output: 3386
Script:
#!/bin/bash
# Search for BinaryTranscodable implementation for Transaction
ast-grep --pattern 'impl BinaryTranscodable for Transaction'
# Look for any custom error handling in prism_serde
rg -A 5 "Error" crates/prism_serde/
Length of output: 158
crates/storage/Cargo.toml (1)
23-23: Verify dependency requirements
The crate now depends on both bincode and prism-serde. As part of the transition to BinaryTranscodable, please verify if the direct bincode dependency is still required.
crates/da/Cargo.toml (1)
26-26: LGTM: Workspace dependency addition is correct
The addition of prism-serde with workspace = true aligns with the PR's objective of transitioning to BinaryTranscodable for serialization.
Let's verify the workspace setup:
✅ Verification successful
Workspace dependency setup is correctly configured
The prism-serde dependency is properly defined in the root Cargo.toml with a path reference to crates/serde, and it's being used consistently across multiple crates in the workspace including da, keys, storage, common, and others. The workspace = true usage in crates/da/Cargo.toml is correct and follows the workspace dependency pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if prism-serde is properly defined in the workspace
rg -l "prism-serde.*=.*{.*version.*=.*" ./Cargo.toml
Length of output: 185
Script:
#!/bin/bash
# Check workspace dependencies in root Cargo.toml
rg "prism-serde" ./Cargo.toml
# Check all Cargo.toml files that depend on prism-serde
fd Cargo.toml --exec rg -l "prism-serde"
Length of output: 252
crates/common/src/tree/key_directory_tree.rs (1)
9-9: Verify usage of removed serialization methods
The removal of serialize_value and deserialize_value methods requires verification that all callers have been updated to use BinaryTranscodable trait methods instead.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Verified: Serialization methods correctly replaced with BinaryTranscodable trait
The removal of serialize_value and deserialize_value methods from KeyDirectoryTree is safe as:
- The removed bincode-based methods were only marked as
pub(crate) - The
SnarkableTreeimplementation now usesBinaryTranscodabletrait methods (encode_to_bytes/decode_from_bytes) for serialization - All references to serialization in
snarkable_tree.rshave been updated to use the new trait methods - No other direct usage of the removed methods was found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the removed methods
rg -A 3 "serialize_value|deserialize_value"
# Search for new encode/decode usage
ast-grep --pattern 'encode_to_bytes()|decode_from_bytes()'
Length of output: 106
Script:
#!/bin/bash
# Search for any remaining references to the removed methods with case insensitive flag
rg -i "serialize.*value|deserialize.*value" -A 3
# Search for specific method calls on KeyDirectoryTree
ast-grep --pattern 'KeyDirectoryTree::$_($$$)'
# Search for any encode/decode method calls
rg -i "encode.*bytes|decode.*bytes" -A 3
# Look at the git diff to see the exact changes
git diff origin/main -- crates/common/src/tree/key_directory_tree.rs
Length of output: 74731
Script:
#!/bin/bash
# Look at the git diff to see the removed methods and their context
git diff -U10 origin/main -- crates/common/src/tree/key_directory_tree.rs
# Search for any remaining references to the removed methods in the entire codebase
rg -i "serialize.*value|deserialize.*value" -A 3 crates/common/src/tree/key_directory_tree.rs
# Search for BinaryTranscodable trait usage
rg -i "BinaryTranscodable" -A 3 crates/common/src/tree/key_directory_tree.rs
Length of output: 1956
Script:
#!/bin/bash
# Search for references to the removed methods in snarkable_tree.rs since it's shown in earlier results
rg -i "serialize.*value|deserialize.*value" -A 3 crates/common/src/tree/snarkable_tree.rs
# Search for BinaryTranscodable trait usage in snarkable_tree.rs
rg -i "BinaryTranscodable|encode_to_bytes|decode_from_bytes" -A 3 crates/common/src/tree/snarkable_tree.rs
# Look for any other files that might be using KeyDirectoryTree
rg "KeyDirectoryTree" -A 3
Length of output: 5365
crates/da/src/lib.rs (2)
5-5: LGTM: Import addition is correct
The addition of BinaryTranscodable import aligns with the serialization changes.
26-28: Verify signature compatibility with existing data
The change from bincode::serialize to encode_to_bytes for signature generation needs verification to ensure compatibility with existing signed data.
✅ Verification successful
Let me gather more information about the signature verification and encoding implementation.
Let me search for more specific information about the encoding and signature verification implementation.
Let me try a different approach to find the encoding and signature verification implementations.
Let me try one more search to find the encoding and signature verification implementations.
Based on the code context found, I can now provide a final response:
The change from bincode::serialize to encode_to_bytes is safe and maintains compatibility
The codebase consistently uses encode_to_bytes for signature generation across multiple components:
- The signature is generated from the encoded bytes in
da/src/lib.rs - The same encoding method is used in signature verification in
verify_signature - This encoding approach is used consistently for hashchains, proofs, and other cryptographic operations throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for signature verification tests
rg -l "test.*verify_signature"
# Search for existing signature data formats
rg -A 3 "insert_signature.*SigningKey"
Length of output: 385
Script:
#!/bin/bash
# Search for signature verification implementation
ast-grep --pattern 'fn verify_signature'
# Search for encode_to_bytes implementation and its usage
ast-grep --pattern 'fn encode_to_bytes'
# Look for any tests involving signatures
rg -A 5 "signature.*test"
Length of output: 131015
Script:
#!/bin/bash
# Look for encode_to_bytes implementation
ast-grep --pattern 'fn encode_to_bytes'
# Look for verify_signature implementation
ast-grep --pattern 'fn verify_signature'
# Search for tests involving signatures and encoding
rg -A 5 "test.*signature.*encode"
Length of output: 120
Script:
#!/bin/bash
# Search for any signature verification related code
rg -A 5 "verify.*signature"
# Search for any encoding related code
rg -A 5 "encode.*bytes"
# Look for any test files that might contain signature tests
fd -e rs "test" --exec rg -l "signature"
Length of output: 130996
Script:
#!/bin/bash
# Search for any files containing both encode and signature related code
rg -l "encode" | xargs rg -l "signature"
# Look for test files containing encode or signature
fd -e rs "test" --exec rg -l "encode|signature"
# Search for any files with encode_to_bytes or verify_signature in their content
rg -A 3 "encode_to_bytes|verify_signature"
Length of output: 7120
crates/common/src/tree/proofs.rs (3)
91-91: LGTM! Consistent with previous learnings.
The serialization changes align with the trait-based approach. Note: As learned from PR #146, caching these serialized values wouldn't be beneficial since they represent different hashchains.
Also applies to: 99-99
120-120: LGTM! Clean and consistent implementation.
The serialization change maintains the simplicity of the verification logic.
58-58: LGTM! Verify error handling compatibility.
The transition to encode_to_bytes aligns with the trait-based serialization approach.
✅ Verification successful
LGTM! Error handling is consistent across the codebase
The verification shows that encode_to_bytes() with the ? operator is consistently used throughout the codebase for error propagation, particularly in the tree proofs, storage, and hashchain implementations. The error handling approach in the reviewed code matches the established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify error handling consistency across the codebase
rg -A 2 "encode_to_bytes\(\).*\?"
Length of output: 2480
crates/common/src/tree/snarkable_tree.rs (3)
127-127: LGTM! Consistent error propagation.
The serialization change maintains proper error handling while adopting the trait-based approach.
185-185: LGTM! Clean deserialization implementation.
The deserialization change maintains consistency with the trait-based approach.
154-154: LGTM! Verify bidirectional compatibility.
The serialization and deserialization changes are consistent. Consider verifying backward compatibility with existing stored data.
Also applies to: 159-159
✅ Verification successful
Let me search for more context about the serialization changes.
Let me search for more context about the Hashchain implementation and serialization.
Based on the evidence gathered, I can now generate the final response:
Serialization changes are consistent and safe.
The code shows that encode_to_bytes and decode_from_bytes are used consistently throughout the codebase for Hashchain serialization. The implementation includes proper error handling and verification in both the tree operations and proofs. The serialization is used in a bidirectionally compatible way, with verification steps ensuring data integrity during updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any custom deserialization logic that might need updating
rg -A 3 "decode_from_bytes.*hashchain"
Length of output: 395
Script:
#!/bin/bash
# Check for Hashchain serialization/deserialization implementations
ast-grep --pattern 'impl $_ for Hashchain {
$$$
encode_to_bytes($$$) {
$$$
}
$$$
}'
# Look for any tests related to Hashchain serialization
rg -l "test.*hashchain.*serial|serial.*hashchain"
# Check if there are any migration utilities or version checks
rg "version.*hashchain|hashchain.*version"
Length of output: 131070
Script:
#!/bin/bash
# Look for Hashchain struct definition and serialization traits
ast-grep --pattern 'struct Hashchain {
$$$
}'
# Check for any tests related to Hashchain serialization
rg -A 3 "test.*hashchain.*serial|serial.*hashchain"
# Look for any migration or compatibility code
rg "decode_from_bytes|encode_to_bytes"
Length of output: 5551
crates/storage/src/redis.rs (1)
93-93: LGTM! Proper error handling implementation.
The deserialization changes maintain robust error handling while adopting the trait-based approach.
Also applies to: 96-96
crates/da/src/celestia.rs (2)
Line range hint 153-157: Verify error handling for encode_to_bytes
The error handling for serialization looks good, maintaining the original error context through the custom error types.
212-214: Verify error handling for transaction encoding
The error handling looks good, using context to preserve the error chain.
crates/common/src/hashchain.rs (2)
345-345: Verify hash validation consistency
The hash validation logic has been updated to use the new serialization method. This is a critical change that affects the integrity of the hashchain.
✅ Verification successful
Let's search for hash validation and encoding related code to understand the impact and test coverage of these changes.
Based on the search results, I can now generate a final response:
Hash validation change is properly integrated and consistent
The encode_to_bytes method is consistently used across the codebase for hash validation:
- In
HashchainEntry::validate_hash()for computing the pristine entry hash - In proof verification flows in the tree module
- In signature verification processes
- In data availability layer serialization
The change maintains consistency with all dependent systems and is well-integrated into the existing validation logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hash validation tests
rg -A 10 "validate_hash.*test" --type rust
Length of output: 44
Script:
#!/bin/bash
# Search for hash validation related code and tests
rg -A 10 "validate.*hash|hash.*valid" --type rust
# Search for test modules related to hashchain
rg -A 10 "mod\s+tests.*hashchain" --type rust
# Search for encode_to_bytes usage in tests
rg -A 5 "encode_to_bytes" --type rust
Length of output: 9200
257-257: Verify serialization format consistency
The change from bincode to encode_to_bytes affects hash computation. Ensure that the new serialization format maintains backward compatibility with existing hashes.
✅ Verification successful
Let me gather more information about the serialization changes to verify the consistency.
Let me search for more specific information about the serialization changes and their impact on hash computation.
Based on the gathered information, I can now generate the final response:
The serialization change is safe and maintains backward compatibility
The change from bincode to encode_to_bytes is part of a consistent serialization framework across the codebase:
- The
encode_to_bytesis used uniformly for all hash computations in the hashchain - The serialization is handled through a dedicated binary transcoding trait implementation
- The hash computation remains consistent as all components (Operation, Hashchain, etc.) use the same serialization method
- Test coverage exists for serialization consistency through
test_serialize_and_deserialize_proof
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing hash computations to verify consistency
rg -A 5 "Digest::hash_items" --type rust
Length of output: 2831
Script:
#!/bin/bash
# Search for the old bincode serialization usage
rg "bincode::serialize" --type rust -B 2 -A 2
# Search for encode_to_bytes implementation and usage
ast-grep --pattern 'impl $_ for $_ {
$$$
fn encode_to_bytes($$$) -> $_ {
$$$
}
$$$
}'
# Search for any test cases that verify hash computation
rg "test.*hash" --type rust -A 5
Length of output: 2406
Script:
#!/bin/bash
# Search for Operation struct and its serialization implementation
ast-grep --pattern 'struct Operation {
$$$
}'
# Search for any migration or compatibility code
rg "encode_to_bytes|serialize" --type rust -B 2 -A 2
# Look for any test cases specifically comparing old and new serialization
rg "test.*serializ" --type rust -A 5
Length of output: 33721
jns-ps
force-pushed
the
binary-transcoding-as-trait
branch
from
525372e to
e51d24b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/serde/src/binary.rs (1)
4-7: Add documentation and consider security boundsThe trait design is clean and flexible, but could benefit from the following improvements:
- Add documentation comments explaining the trait's purpose, methods, and usage examples
- Consider adding size limits for serialization to prevent potential DoS attacks
Add documentation and size validation:
+/// Trait for types that can be encoded to and decoded from binary format. +/// +/// # Examples +/// ``` +/// use prism_serde::binary::BinaryTranscodable; +/// +/// #[derive(Serialize, Deserialize)] +/// struct MyType { /* ... */ } +/// +/// let data = MyType { /* ... */ }; +/// let bytes = data.encode_to_bytes()?; +/// let decoded = MyType::decode_from_bytes(&bytes)?; +/// ``` pub trait BinaryTranscodable<'de>: Sized { + /// Maximum allowed size in bytes for serialized data + const MAX_SIZE: usize = 1024 * 1024; // 1MB default + + /// Encodes the value into a byte vector. + /// + /// # Errors + /// Returns an error if serialization fails or if the result exceeds `MAX_SIZE`. fn encode_to_bytes(&self) -> Result<Vec<u8>>; + + /// Decodes a value from bytes. + /// + /// # Errors + /// Returns an error if deserialization fails or if input exceeds `MAX_SIZE`. fn decode_from_bytes<B: AsRef<[u8]>>(bytes: &'de B) -> Result<Self>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/common/src/hashchain.rs(3 hunks)crates/common/src/transaction.rs(2 hunks)crates/common/src/tree/key_directory_tree.rs(1 hunks)crates/common/src/tree/proofs.rs(4 hunks)crates/common/src/tree/snarkable_tree.rs(4 hunks)crates/da/Cargo.toml(1 hunks)crates/da/src/celestia.rs(4 hunks)crates/da/src/lib.rs(3 hunks)crates/serde/Cargo.toml(1 hunks)crates/serde/src/binary.rs(1 hunks)crates/serde/src/lib.rs(1 hunks)crates/storage/Cargo.toml(1 hunks)crates/storage/src/redis.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/da/Cargo.toml
- crates/storage/Cargo.toml
- crates/serde/Cargo.toml
- crates/serde/src/lib.rs
- crates/common/src/transaction.rs
- crates/common/src/tree/snarkable_tree.rs
- crates/common/src/hashchain.rs
- crates/da/src/lib.rs
- crates/da/src/celestia.rs
- crates/common/src/tree/key_directory_tree.rs
- crates/common/src/tree/proofs.rs
- crates/storage/src/redis.rs
🔇 Additional comments (1)
crates/serde/src/binary.rs (1)
1-20: Verify migration impact across codebase
Since this is a new trait that replaces direct bincode usage, we should verify the migration impact.
✅ Verification successful
Migration to BinaryTranscodable is already in use and working as intended
The verification shows that:
- The only direct
bincodeusage is within theBinaryTranscodabletrait implementation itself - The trait is already being used extensively across the codebase in multiple components:
- Storage layer (Redis implementation)
- DA layer (Celestia implementation)
- Common module (Transaction, Hashchain, Tree implementations)
The pattern is consistently used for binary serialization/deserialization throughout the codebase, showing this is a well-established abstraction rather than a new migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for remaining direct bincode usage that should be migrated
echo "Checking for direct bincode usage that should be migrated..."
rg --type rust 'bincode::(serialize|deserialize)' -l
# Check for potential breaking changes in existing implementations
echo "Checking for existing implementations that might conflict..."
ast-grep --pattern 'impl$$$BinaryTranscodable$$$'
# Check for size-sensitive types that might need custom limits
echo "Checking for types with potential large serialized sizes..."
rg --type rust 'struct.*\{[\s\S]*\}' -A 10
Length of output: 569
Script:
#!/bin/bash
# Let's check for any existing usage of bincode in the codebase with more context
echo "Checking bincode usage with context..."
rg --type rust 'bincode::' -C 3
# Let's also check if there are any existing serialization patterns
echo "Checking for existing serialization patterns..."
rg --type rust '(serialize|deserialize|encode|decode).*bytes' -C 2
# Check for any existing trait implementations that might be affected
echo "Checking for trait implementations related to serialization..."
ast-grep --pattern 'impl$_for$_where$$$'
Length of output: 15400
Ensures independence from a certain library (e.g. bincode).
jns-ps
force-pushed
the
binary-transcoding-as-trait
branch
from
e51d24b to
07ae97a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/storage/src/redis.rs (2)
Line range hint
148-148: Handle potential errors from pipe.execute.The pipe.execute result is currently ignored, which could hide potential Redis errors.
- pipe.execute(&mut con); + pipe.execute(&mut con).map_err(|e| anyhow!("Redis pipeline execution failed: {}", e))?;
Line range hint
1-214: Consider adding documentation for serialization format.As part of the transition to
BinaryTranscodable, consider:
- Documenting the binary format specification
- Adding version information to the serialized data
- Including migration strategy for existing data
This will help with future maintenance and potential format changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/common/src/hashchain.rs(3 hunks)crates/common/src/transaction.rs(2 hunks)crates/common/src/tree/key_directory_tree.rs(1 hunks)crates/common/src/tree/proofs.rs(4 hunks)crates/common/src/tree/snarkable_tree.rs(4 hunks)crates/da/Cargo.toml(1 hunks)crates/da/src/celestia.rs(4 hunks)crates/da/src/lib.rs(3 hunks)crates/serde/Cargo.toml(1 hunks)crates/serde/src/binary.rs(1 hunks)crates/serde/src/lib.rs(1 hunks)crates/storage/Cargo.toml(1 hunks)crates/storage/src/redis.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/da/Cargo.toml
- crates/storage/Cargo.toml
- crates/serde/Cargo.toml
- crates/serde/src/lib.rs
- crates/common/src/tree/proofs.rs
- crates/serde/src/binary.rs
- crates/common/src/hashchain.rs
- crates/da/src/lib.rs
- crates/common/src/tree/snarkable_tree.rs
- crates/common/src/tree/key_directory_tree.rs
- crates/common/src/transaction.rs
- crates/da/src/celestia.rs
🔇 Additional comments (3)
crates/storage/src/redis.rs (3)
7-7: LGTM: Import added for new serialization trait.
The addition of BinaryTranscodable import aligns with the PR's objective of transitioning to trait-based binary transcoding.
136-137: LGTM: Proper serialization with error handling.
The serialization changes look good, using proper error handling with the ? operator.
81-83:
Replace unwrap() with proper error handling.
The unwrap() usage could lead to runtime panics. Propagate the error using the ? operator instead.
- Ok(node_data.map(|data| Node::decode_from_bytes(&data).unwrap()))
+ Ok(node_data.map(|data| Node::decode_from_bytes(&data)?))
jns-ps
changed the title
Summary by CodeRabbit
New Features
HashchainandTransactionentities, enhancing data processing.binarymodule to improve serialization and deserialization capabilities.Bug Fixes
Chores
prism-serde,anyhow, andbincode, ensuring better error handling and serialization support.