feature(keys): crypto algorithm as enum #188
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant CryptoAlgorithm
participant SigningKey
participant VerifyingKey
Client->>CryptoAlgorithm: Select Algorithm
Client->>SigningKey: from_algorithm_and_bytes(algorithm, bytes)
SigningKey-->>Client: Create SigningKey
Client->>VerifyingKey: from_algorithm_and_bytes(algorithm, bytes)
VerifyingKey-->>Client: Create VerifyingKey
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Nitpick comments (5)
crates/keys/src/algorithm.rs (2)
3-9: Add documentation for the CryptoAlgorithm enum and its variants.Consider adding documentation to explain:
- The purpose of this enum
- When to use each variant
- Any specific characteristics or use cases for each algorithm
Example:
#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] +/// Represents supported cryptographic algorithms for digital signatures. pub enum CryptoAlgorithm { + /// Edwards-curve Digital Signature Algorithm (EdDSA) using curve25519 Ed25519, + /// Elliptic Curve Digital Signature Algorithm (ECDSA) using secp256k1 curve Secp256k1, + /// ECDSA using NIST P-256 (secp256r1) curve Secp256r1, }
3-9: Consider adding TryFrom<&str> implementation and unit tests.Adding string conversion and tests would improve robustness:
- Implement
TryFrom<&str>for parsing algorithm strings- Add unit tests for serialization/deserialization
- Add tests for string conversion
Example implementation:
impl TryFrom<&str> for CryptoAlgorithm { type Error = anyhow::Error; fn try_from(s: &str) -> Result<Self, Self::Error> { match s.to_lowercase().as_str() { "ed25519" => Ok(Self::Ed25519), "secp256k1" => Ok(Self::Secp256k1), "secp256r1" => Ok(Self::Secp256r1), _ => Err(anyhow::anyhow!("Unknown algorithm: {}", s)), } } } #[cfg(test)] mod tests { use super::*; #[test] fn test_serde() { let alg = CryptoAlgorithm::Ed25519; let serialized = serde_json::to_string(&alg).unwrap(); assert_eq!(serialized, "\"ed25519\""); let deserialized: CryptoAlgorithm = serde_json::from_str(&serialized).unwrap(); assert_eq!(deserialized, alg); } }crates/keys/src/signatures.rs (2)
31-42: Improve error handling in from_algorithm_and_bytes.The error handling could be more specific to help with debugging:
- Consider wrapping errors with context
- Add specific error types for different failure cases
Example improvement:
pub fn from_algorithm_and_bytes(algorithm: CryptoAlgorithm, bytes: &[u8]) -> Result<Self> { match algorithm { CryptoAlgorithm::Ed25519 => { - Ed25519Signature::try_from(bytes).map(Signature::Ed25519).map_err(|e| e.into()) + Ed25519Signature::try_from(bytes) + .map(Signature::Ed25519) + .map_err(|e| anyhow::anyhow!("Invalid Ed25519 signature: {}", e)) } CryptoAlgorithm::Secp256k1 => { - Secp256k1Signature::from_der(bytes).map(Signature::Secp256k1).map_err(|e| e.into()) + Secp256k1Signature::from_der(bytes) + .map(Signature::Secp256k1) + .map_err(|e| anyhow::anyhow!("Invalid Secp256k1 signature: {}", e)) } // Similar changes for Secp256r1... } }
58-60: Add tests for CryptoPayload conversion.The implementations look correct, but tests should be added to verify the conversion behavior, especially around the Placeholder variant.
Example test cases:
#[cfg(test)] mod tests { use super::*; #[test] fn test_crypto_payload_conversion() { let signature = Signature::Ed25519(/* ... */); let payload: CryptoPayload = signature.clone().into(); let converted: Signature = payload.try_into().unwrap(); assert_eq!(signature, converted); } #[test] fn test_placeholder_conversion() { let signature = Signature::Placeholder; let payload: CryptoPayload = signature.into(); assert_eq!(payload.algorithm, CryptoAlgorithm::Ed25519); assert!(payload.bytes.is_empty()); } }Also applies to: 65-69
crates/keys/src/verifying_keys.rs (1)
Line range hint
191-214: Consider enhancing FromBase64 with explicit algorithm specificationThe current implementation determines the algorithm based on key length. Consider adding an overload that accepts a
CryptoAlgorithmparameter for more explicit and type-safe key parsing:impl FromBase64 for VerifyingKey { // ... existing implementation ... fn from_base64_with_algorithm<T: AsRef<[u8]>>( base64: T, algorithm: CryptoAlgorithm, ) -> Result<Self, Self::Error> { let bytes = Vec::<u8>::from_base64(base64)?; Self::from_algorithm_and_bytes(algorithm, &bytes) } }This would allow callers to explicitly specify their expected algorithm while maintaining backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/keys/src/algorithm.rs(1 hunks)crates/keys/src/lib.rs(1 hunks)crates/keys/src/payload.rs(1 hunks)crates/keys/src/signatures.rs(3 hunks)crates/keys/src/signing_keys.rs(4 hunks)crates/keys/src/verifying_keys.rs(3 hunks)crates/serde/src/lib.rs(0 hunks)
💤 Files with no reviewable changes (1)
- crates/serde/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- crates/keys/src/lib.rs
🔇 Additional comments (8)
crates/keys/src/signatures.rs (1)
45-52: Reconsider default algorithm for Placeholder variant.
The algorithm() method returns Ed25519 for the Placeholder variant, which seems arbitrary and could be misleading. Consider:
- Making this behavior explicit in documentation
- Returning a Result instead to force handling of the Placeholder case
Let's check if this default is used elsewhere:
✅ Verification successful
Let me check how the Placeholder variant is used in the transaction signing flow to better understand the implications of the default algorithm.
Let me try a different approach to check the transaction signing implementation.
Based on the findings, I can now generate the final response:
The Ed25519 default for Placeholder is intentional and safe
The Placeholder variant is used as a temporary state in transaction signing flow, where:
- It's used to initialize unsigned transactions
- It's immediately replaced with the actual signature during signing
- The algorithm type doesn't impact the security since the placeholder signature is never used for verification
The current implementation is correct as:
- The signing flow properly handles the Placeholder by checking its presence before signing
- The Ed25519 default is only used temporarily and gets replaced with the actual signing algorithm
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Placeholder variant usage
rg "Signature::Placeholder" -A 2
Length of output: 993
Script:
#!/bin/bash
# Check transaction signing implementation
ast-grep --pattern 'fn sign($$$) {
$$$
}' crates/common/src/transaction.rs
Length of output: 78
Script:
#!/bin/bash
# Search for sign method implementation with context
rg "fn sign" -A 10 crates/common/src/transaction.rs
Length of output: 393
crates/keys/src/signing_keys.rs (4)
11-11: LGTM: Import changes align with enum-based approach
The updated imports properly introduce the new CryptoAlgorithm enum and CryptoPayload type.
45-56: LGTM: Enhanced type safety with CryptoAlgorithm enum
The conversion from string-based to enum-based algorithm selection improves type safety and eliminates potential runtime errors from invalid algorithm strings.
59-64: LGTM: Consistent algorithm type representation
The method now returns the strongly-typed CryptoAlgorithm enum, maintaining consistency with the new type system.
101-110: LGTM: Clean payload conversion implementations
The payload conversion implementations correctly utilize the new CryptoAlgorithm type while maintaining clean and straightforward conversion logic.
crates/keys/src/verifying_keys.rs (3)
19-20: LGTM: Clean import organization
The imports are well-organized and properly introduce the new types.
63-74: LGTM: Consistent implementation with SigningKey
The changes mirror those in SigningKey, maintaining consistency across the codebase while preserving proper error handling.
77-82: LGTM: Symmetric algorithm implementation
The implementation symmetrically matches SigningKey's approach, maintaining codebase consistency.
| #[derive(Serialize, Deserialize)] | ||
| pub struct CryptoPayload { | ||
| pub algorithm: CryptoAlgorithm, | ||
| #[serde(with = "raw_or_b64")] | ||
| pub bytes: Vec<u8>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and consider encapsulation.
The struct implementation could be improved:
- Add documentation explaining the purpose and usage
- Consider encapsulating fields with accessor methods
- Add validation for the bytes field
Example improvements:
#[derive(Serialize, Deserialize)]
+/// Represents a cryptographic payload containing an algorithm identifier and its associated bytes.
pub struct CryptoPayload {
- pub algorithm: CryptoAlgorithm,
- #[serde(with = "raw_or_b64")]
- pub bytes: Vec<u8>,
+ algorithm: CryptoAlgorithm,
+ #[serde(with = "raw_or_b64")]
+ bytes: Vec<u8>,
}
+impl CryptoPayload {
+ /// Creates a new CryptoPayload with validation
+ pub fn new(algorithm: CryptoAlgorithm, bytes: Vec<u8>) -> Result<Self> {
+ // Add validation based on algorithm's expected byte length
+ Ok(Self { algorithm, bytes })
+ }
+
+ /// Returns the cryptographic algorithm
+ pub fn algorithm(&self) -> CryptoAlgorithm {
+ self.algorithm
+ }
+
+ /// Returns a reference to the raw bytes
+ pub fn bytes(&self) -> &[u8] {
+ &self.bytes
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "lowercase")] | ||
| pub enum CryptoAlgorithm { |
There was a problem hiding this comment.
For these specific things, it should be called "Curve" instead of "CryptoAlgorithm"
Originally posted by @distractedm1nd in #187 (comment)
There was a problem hiding this comment.
I still disagree on this. What if we wanted to support keys/sigs without a curve?
Are there signing keys that are not based on elliptic curves?
RSA: Based on the factoring problem of large composite numbers. Still widely used.
Hash-based signatures: SPHINCS+, XMSS, WOTS+. Very quantum-resistant but larger signatures.
Lattice-based: Dilithium, Falcon. Also quantum-resistant, based on hard lattice problems.
Multivariate: Rainbow (though broken now). Based on solving systems of multivariate polynomial equations.
Summary by CodeRabbit
New Features
CryptoAlgorithmwith variants for different cryptographic algorithms.CryptoPayloadto encapsulate cryptographic data.algorithmfor better organization of cryptographic functionalities.Bug Fixes
CryptoAlgorithmenum.Chores
CryptoPayloadstruct from the serialization library.