refactor(common): use underlying VK types instead of Vec<u8> #150
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing type safety and memory management. 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: 2
🧹 Outside diff range and nitpick comments (1)
crates/bin/src/main.rs (1)
43-46: Consider adding debug logging for rejected key types.
When a non-Ed25519 key is provided, it's silently converted to None. Consider adding debug logging to help troubleshoot configuration issues.
.and_then(|vk: VerifyingKey| match vk {
VerifyingKey::Ed25519(key) => Some(key),
- _ => None,
+ other => {
+ debug!("Ignoring non-Ed25519 verifying key: {:?}", other);
+ None
+ },
});📜 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 (5)
- Cargo.toml (1 hunks)
- crates/bin/src/main.rs (1 hunks)
- crates/common/src/keys.rs (6 hunks)
- crates/common/src/test_utils.rs (2 hunks)
- crates/common/src/tree.rs (4 hunks)
🔇 Additional comments (7)
crates/bin/src/main.rs (1)
43-46: LGTM! Improved type safety with explicit variant handling.
The change from try_into() to pattern matching enhances type safety by explicitly handling the Ed25519 variant and rejecting others, which aligns well with the PR's objective of using proper underlying types.
Cargo.toml (2)
84-88: LGTM! Feature addition aligns with refactoring goals.
Adding the "serde" feature to secp256k1 is appropriate for transitioning from Vec<u8> to proper verification key types.
84-88: Verify patch compatibility with serde feature.
The secp256k1 dependency is using a patched version. Let's verify that the patch supports the serde feature.
✅ Verification successful
Let me gather more information about the secp256k1 dependency to verify if it's actually using a patch and if the serde feature is supported.
Let me check the patched repository to verify if the serde feature is supported in the specific branch.
Serde feature is supported in the patched version
The patched repository (sp1-patches/rust-secp256k1) at branch patch-secp256k1-v0.29.0 contains serde-related files (src/serde_util.rs and tests/serde.rs), indicating that serde serialization is implemented and tested. The presence of these files confirms that the serde feature is supported in the patched version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the patched secp256k1 repository supports the serde feature
# Test: Check if the patch branch exists and contains serde feature
gh repo view sp1-patches/rust-secp256k1 --json defaultBranchRef -q .defaultBranchRef.name
gh repo view sp1-patches/rust-secp256k1 --json description -q .description
# Test: Look for serde-related changes in Cargo.toml
rg -A 5 'serde.*features' Cargo.toml
Length of output: 446
Script:
#!/bin/bash
# Check if there's a patch specification for secp256k1 in Cargo.toml
rg -A 3 'patch.crates-io' Cargo.toml
# Look for the actual secp256k1 dependency declaration
rg -A 5 '^secp256k1.*=' Cargo.toml
# Check if there are any workspace patches
fd Cargo.toml --exec rg -A 3 'patch.crates-io' {}
Length of output: 1245
Script:
#!/bin/bash
# Fetch the Cargo.toml from the patched repository's specific branch
gh api /repos/sp1-patches/rust-secp256k1/contents/Cargo.toml?ref=patch-secp256k1-v0.29.0 --jq '.content' | base64 -d | rg -A 5 'serde.*feature|^\[features\]' || echo "Failed to fetch Cargo.toml"
# Check for any serde-related files or implementations
gh api /repos/sp1-patches/rust-secp256k1/git/trees/patch-secp256k1-v0.29.0?recursive=1 --jq '.tree[].path' | rg 'serde|impl.*Serialize' || echo "Failed to fetch tree"
Length of output: 469
crates/common/src/test_utils.rs (2)
108-108: LGTM: Consistent with insert_account changes.
The dereferencing of the update proof follows the same safe pattern as insert_account, maintaining consistency in proof handling.
94-94: LGTM: Proof dereferencing aligns with boxed variant changes.
The dereferencing of the insert proof is correct and consistent with the PR's changes to box proof variants.
Let's verify the Proof enum's boxing:
✅ Verification successful
Dereferencing of boxed proof is correct and consistent with implementation
The verification confirms that in crates/common/src/tree.rs, the Proof enum variants are indeed boxed:
pub enum Proof {
Update(Box<UpdateProof>),
Insert(Box<InsertProof>),
}Therefore, dereferencing the boxed proof with *insert_proof in the test utilities is the correct approach to extract the inner value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Proof variants are now boxed
ast-grep --pattern 'enum Proof {
Insert(Box<$_>),
Update(Box<$_>)
}'
Length of output: 74
Script:
#!/bin/bash
# Let's try a more flexible pattern to find the Proof enum definition
ast-grep --pattern 'enum Proof {
$$$
}'
# Also search for any Proof enum usage
rg "enum Proof" -A 5
Length of output: 1038
crates/common/src/tree.rs (2)
188-189: LGTM! Boxing large proof variants improves memory efficiency.
The change to box the UpdateProof and InsertProof variants is a good optimization. This prevents the Proof enum from becoming too large on the stack, as proofs can contain significant amounts of data.
402-402: LGTM! Consistent boxing of proof variants.
The implementation consistently boxes the proofs when constructing the Proof enum variants across all code paths: process_operation, CreateAccount, and RegisterService operations.
Also applies to: 473-475, 494-496
distractedm1nd
force-pushed
the
vk-types
branch
from
e691d43 to
f5dfcf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/common/src/keys.rs (1)
152-173: Consider adding more error case tests
While the current tests cover the happy path well, consider adding test cases for:
- Invalid base64 encoding
- Corrupted key bytes
- Wrong key type (e.g., Ed25519 bytes for Secp256k1)
📜 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 (5)
- Cargo.toml (1 hunks)
- crates/bin/src/main.rs (1 hunks)
- crates/common/src/keys.rs (6 hunks)
- crates/common/src/test_utils.rs (2 hunks)
- crates/common/src/tree.rs (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- crates/bin/src/main.rs
- crates/common/src/test_utils.rs
- crates/common/src/tree.rs
🧰 Additional context used
📓 Learnings (1)
crates/common/src/keys.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#150
File: crates/common/src/keys.rs:27-31
Timestamp: 2024-10-29T15:02:10.475Z
Learning: In the Rust codebase of `prism`, within the `VerifyingKey::as_bytes` method in `crates/common/src/keys.rs`, returning a `Vec<u8>` is necessary because the underlying key types (`Secp256k1VerifyingKey` and `Ed25519VerifyingKey`) provide their byte representations by value. Returning a `&[u8]` would involve borrowing from a temporary value, which is not feasible.
🔇 Additional comments (5)
crates/common/src/keys.rs (5)
20-22: LGTM! Good improvement in type safety
The change from Vec<u8> to proper verification key types (Secp256k1VerifyingKey and Ed25519VerifyingKey) is a solid improvement. It provides better type safety and aligns the implementation with how SigningKey is structured.
27-31: LGTM! Necessary allocation confirmed
Based on the previous discussion, returning Vec<u8> is necessary here as the underlying key types provide their byte representations by value.
Line range hint 57-75: LGTM! Clean and efficient implementations
The From trait implementations are now more direct and efficient, properly utilizing the type system without unnecessary conversions.
103-112: LGTM! Well-documented and robust implementation
The try_from implementation properly handles key creation with:
- Accurate key length validation
- Specific error messages
- Direct creation of proper key types
37-46: Consider supporting different Secp256k1 signature formats
The signature length check of 64 bytes is still restrictive for Secp256k1 signatures, which can have different formats (e.g., DER-encoded signatures of 70-72 bytes). While the current implementation works with compact signatures, it might be worth documenting this limitation or supporting additional formats.
Consider:
- Adding a doc comment specifying that only 64-byte compact signatures are supported
- Or supporting DER format using
Secp256k1Signature::from_der
Let's verify the signature format usage in the codebase:
Noticed in the stream last night that SigningKey uses the correct underlying types from the curve libraries, while VerifyingKey uses Vec everywhere.
By enabling serde on the secp256k1 crate, we can derive serde. The only change this had elsewhere was that we now have to box the proof variant types in the proof enum.
Summary by CodeRabbit
New Features
secp256k1dependency to support serialization with the addition of the"serde"feature.Secp256k1andEd25519.Bug Fixes
mainfunction to ensure correct handling of verifying keys, enhancing type safety and error handling.Refactor
VerifyingKeyenum andProofenum, improving memory management and clarity.Tests