feat(dpp): add a convenience method to get the public key data for a private key depending on the key type #2214
Conversation
…te key depending on the key type
WalkthroughThe changes introduce a new public method Changes
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: 4
🧹 Outside diff range and nitpick comments (1)
packages/rs-dpp/src/identity/identity_public_key/key_type.rs (1)
254-258: Clarify the unsupported operation in the error messageThe error message returned for
KeyType::BIP13_SCRIPT_HASHcould be more descriptive. Providing additional context can help users understand why the operation isn't supported.Consider updating the error message:
return Err(ProtocolError::NotSupported( - "Converting a private key to a script hash is not supported".to_string(), + "Conversion from private key to script hash for BIP13_SCRIPT_HASH key type is not supported".to_string(), ));This change offers clearer information about the limitation.
| KeyType::ECDSA_SECP256K1 => { | ||
| let secp = Secp256k1::new(); | ||
| let secret_key = | ||
| dashcore::secp256k1::SecretKey::from_slice(private_key_bytes.as_slice()) | ||
| .map_err(|e| ProtocolError::Generic(e.to_string()))?; | ||
| let private_key = dashcore::PrivateKey::new(secret_key, network); | ||
|
|
||
| Ok(private_key.public_key(&secp).to_bytes()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to avoid code duplication in ECDSA key types
The match arms for KeyType::ECDSA_SECP256K1 and KeyType::ECDSA_HASH160 share similar code for handling ECDSA keys. Consider extracting the common logic into a helper function to enhance maintainability and reduce duplication.
Create a helper function:
fn get_ecdsa_private_key(
private_key_bytes: &[u8],
network: Network,
) -> Result<dashcore::PrivateKey, ProtocolError> {
let secp = Secp256k1::new();
let secret_key = dashcore::secp256k1::SecretKey::from_slice(private_key_bytes)
.map_err(|e| ProtocolError::Generic(e.to_string()))?;
Ok(dashcore::PrivateKey::new(secret_key, network))
}Update the match arms:
For KeyType::ECDSA_SECP256K1:
let private_key = get_ecdsa_private_key(private_key_bytes, network)?;
let secp = Secp256k1::new();
Ok(private_key.public_key(&secp).to_bytes())For KeyType::ECDSA_HASH160:
let private_key = get_ecdsa_private_key(private_key_bytes, network)?;
let secp = Secp256k1::new();
Ok(ripemd160_sha256(private_key.public_key(&secp).to_bytes().as_slice()).to_vec())This refactor eliminates redundancy and streamlines the code.
| pub fn public_key_data_from_private_key_data( | ||
| &self, | ||
| private_key_bytes: Vec<u8>, | ||
| network: Network, | ||
| ) -> Result<Vec<u8>, ProtocolError> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a slice for the private_key_bytes parameter
Currently, the method takes ownership of private_key_bytes as a Vec<u8>, but it doesn't modify it internally. Using a slice &[u8] would avoid unnecessary cloning and improve efficiency.
Apply this diff to change the parameter type:
-pub fn public_key_data_from_private_key_data(
- &self,
- private_key_bytes: Vec<u8>,
- network: Network,
-) -> Result<Vec<u8>, ProtocolError> {
+pub fn public_key_data_from_private_key_data(
+ &self,
+ private_key_bytes: &[u8],
+ network: Network,
+) -> Result<Vec<u8>, ProtocolError> {Adjust subsequent code by removing .as_slice() calls since private_key_bytes is already a slice.
📝 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 public_key_data_from_private_key_data( | |
| &self, | |
| private_key_bytes: Vec<u8>, | |
| network: Network, | |
| ) -> Result<Vec<u8>, ProtocolError> { | |
| pub fn public_key_data_from_private_key_data( | |
| &self, | |
| private_key_bytes: &[u8], | |
| network: Network, | |
| ) -> Result<Vec<u8>, ProtocolError> { |
| .g1_element() | ||
| .expect("expected to get a public key from a bls private key") | ||
| .to_bytes() | ||
| .to_vec(); |
There was a problem hiding this comment.
Handle potential errors without using expect
Using .expect() can lead to panics if g1_element() returns an error. Instead, propagate the error using ? to maintain robustness.
Apply this diff to handle the error properly:
-let public_key_bytes = private_key
- .g1_element()
- .expect("expected to get a public key from a bls private key")
- .to_bytes()
- .to_vec();
+let public_key_bytes = private_key
+ .g1_element()?
+ .to_bytes()
+ .to_vec();Ensure that g1_element() returns Result and any errors are appropriately mapped to ProtocolError.
📝 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.
| .g1_element() | |
| .expect("expected to get a public key from a bls private key") | |
| .to_bytes() | |
| .to_vec(); | |
| .g1_element()? | |
| .to_bytes() | |
| .to_vec(); |
| let key_pair = ed25519_dalek::SigningKey::from_bytes( | ||
| &private_key_bytes.as_slice().try_into().map_err(|_| { | ||
| ProtocolError::InvalidVectorSizeError(InvalidVectorSizeError::new( | ||
| 32, | ||
| private_key_bytes.len(), | ||
| )) | ||
| })?, | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Missing proper error handling for SigningKey::from_bytes
- In
packages/rs-dpp/src/identity/identity_public_key/key_type.rslines 244-251, theSigningKey::from_bytescall does not handle potential errors using the?operator or.map_err.
🔗 Analysis chain
Ensure correct error handling for invalid key sizes
In the KeyType::EDDSA_25519_HASH160 arm, the conversion of private_key_bytes to a fixed-size array could fail if the length is incorrect. Verify that the error handling accurately reflects the issue.
Run the following script to check if the from_bytes method handles varying key sizes correctly:
This script simulates keys of different sizes to ensure that the error handling works as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the ED25519 key handling correctly detects invalid key sizes.
# Test: Attempt to create a SigningKey with invalid key sizes.
# Expect: The method should return an error for sizes other than 32 bytes.
for size in 31 32 33; do
echo "Testing with key size: $size bytes"
python3 -c "
import os
key_bytes = os.urandom($size)
try:
from ed25519_dalek import SigningKey
SigningKey.from_bytes(key_bytes)
print('Key accepted')
except Exception as e:
print(f'Error: {e}')
"
done
Length of output: 1181
Script:
#!/bin/bash
# Description: Verify that all usages of SigningKey::from_bytes have proper error handling.
# Search for all instances of SigningKey::from_bytes in the codebase
ast-grep --lang rust --pattern $'SigningKey::from_bytes($_)' > signing_key_usages.txt
# Check each usage for error handling
echo "Verifying error handling for each SigningKey::from_bytes usage:"
while IFS= read -r line; do
# Extract file and line number
file=$(echo "$line" | awk '{print $1}')
code=$(echo "$line" | cut -d' ' -f2-)
# Check if error handling is present using regex
if echo "$code" | grep -qE '\?|\.map_err'; then
echo "[$file]: Proper error handling found."
else
echo "[$file]: Missing error handling for SigningKey::from_bytes."
fi
done < signing_key_usages.txt
Length of output: 220
Script:
#!/bin/bash
# Description: Verify that all usages of SigningKey::from_bytes in key_type.rs have proper error handling.
# Define the file to inspect
FILE_PATH="packages/rs-dpp/src/identity/identity_public_key/key_type.rs"
# Check if the file exists
if [ ! -f "$FILE_PATH" ]; then
echo "Error: File $FILE_PATH does not exist."
exit 1
fi
# Search for all instances of SigningKey::from_bytes in the specified file
echo "Searching for SigningKey::from_bytes usages in $FILE_PATH..."
matches=$(rg 'SigningKey::from_bytes' "$FILE_PATH")
# Check if any matches were found
if [ -z "$matches" ]; then
echo "No instances of SigningKey::from_bytes found in $FILE_PATH."
exit 0
fi
# Iterate over each match and verify error handling
echo "Verifying error handling for each SigningKey::from_bytes usage:"
echo "$matches" | while IFS= read -r line; do
# Extract the line number and code
line_number=$(echo "$line" | cut -d':' -f2)
code=$(echo "$line" | cut -d':' -f3-)
# Extract surrounding lines for context (2 lines before and after)
context=$(rg -C 2 "SigningKey::from_bytes" "$FILE_PATH" | grep -A2 "SigningKey::from_bytes" | grep -v "SigningKey::from_bytes")
# Check for error handling using '?' or '.map_err'
if echo "$code" | grep -qE '\?'; then
echo "Line $line_number: Proper error handling found using '?'."
elif echo "$code" | grep -qE '\.map_err'; then
echo "Line $line_number: Proper error handling found using '.map_err'."
else
echo "Line $line_number: **Missing** proper error handling for SigningKey::from_bytes."
fi
done
Length of output: 1731
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/rs-dpp/src/identity/identity_public_key/key_type.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
packages/rs-dpp/src/identity/identity_public_key/key_type.rs (7)
17-17: Import change looks goodThe addition of
InvalidVectorSizeErrorto the import statement is appropriate, as it's used in the newpublic_key_data_from_private_key_datamethod. This change enhances code clarity by explicitly importing the error type.
205-209: Method signature and structure look goodThe method signature is well-defined, taking a slice of private key bytes and a network parameter. The return type
Result<Vec<u8>, ProtocolError>is appropriate for this operation, allowing for error propagation.
211-218: ECDSA_SECP256K1 implementation is correctThe implementation for ECDSA_SECP256K1 keys is well-structured and includes proper error handling. It correctly creates a
SecretKeyfrom the input bytes, constructs aPrivateKeywith the given network, and returns the corresponding public key bytes.
237-244: ECDSA_HASH160 implementation is correctThe implementation for ECDSA_HASH160 keys is well-structured and includes proper error handling. It correctly creates a
SecretKeyfrom the input bytes, constructs aPrivateKeywith the given network, and applies theripemd160_sha256hash function to the public key bytes.
245-262: EDDSA_25519_HASH160 implementation is correct and well-handledThe implementation for EDDSA_25519_HASH160 keys is well-structured and includes proper error handling:
- The use of the
ed25519-dalekfeature flag is appropriate for conditional compilation.- The error handling for invalid input size using
InvalidVectorSizeErroris well-implemented.- The application of
ripemd160_sha256to the verifying key bytes is correct.- The error handling for the
not(feature = "ed25519-dalek")case is good, returning aNotSupportederror.
263-267: BIP13_SCRIPT_HASH handling is appropriateThe implementation for BIP13_SCRIPT_HASH keys correctly returns a
NotSupportederror. This is an appropriate approach for a key type that doesn't support conversion from private to public key data.
205-269: Overall implementation ofpublic_key_data_from_private_key_datais well-structured and comprehensiveThe new method
public_key_data_from_private_key_datais a valuable addition to theKeyTypeenum. It provides a unified way to derive public key data from private key data for various key types. Key points:
- The method signature is well-defined, taking appropriate parameters and returning a
Resulttype.- Implementation for each key type is handled separately, allowing for type-specific logic.
- Error handling is generally well-implemented, with appropriate use of
ProtocolErrorvariants.- Feature flags are used effectively to handle conditional compilation for BLS and EdDSA implementations.
- Unsupported key types (BIP13_SCRIPT_HASH) are handled explicitly.
The only suggested improvement is in the error handling for the BLS12_381 case, where
expect()could be replaced withmap_err()for more consistent error propagation.Overall, this implementation enhances the functionality of the
KeyTypeenum and provides a robust method for deriving public key data.
| KeyType::BLS12_381 => { | ||
| #[cfg(feature = "bls-signatures")] | ||
| { | ||
| let private_key = | ||
| bls_signatures::PrivateKey::from_bytes(private_key_bytes, false) | ||
| .map_err(|e| ProtocolError::Generic(e.to_string()))?; | ||
| let public_key_bytes = private_key | ||
| .g1_element() | ||
| .expect("expected to get a public key from a bls private key") | ||
| .to_bytes() | ||
| .to_vec(); | ||
| Ok(public_key_bytes) | ||
| } | ||
| #[cfg(not(feature = "bls-signatures"))] | ||
| return Err(ProtocolError::NotSupported( | ||
| "Converting a private key to a bls public key is not supported without the bls-signatures feature".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Improve error handling in BLS12_381 implementation
The BLS12_381 implementation is generally correct, but there's room for improvement in error handling:
-
The use of
expect()when callingg1_element()could lead to a panic. Consider usingmap_err()to convert any error to aProtocolErrorinstead. -
The error handling for the
not(feature = "bls-signatures")case is good, returning aNotSupportederror.
Consider replacing the expect() call with proper error handling:
- let public_key_bytes = private_key
- .g1_element()
- .expect("expected to get a public key from a bls private key")
- .to_bytes()
- .to_vec();
+ let public_key_bytes = private_key
+ .g1_element()
+ .map_err(|e| ProtocolError::Generic(e.to_string()))?
+ .to_bytes()
+ .to_vec();This change will propagate any errors from g1_element() as a ProtocolError instead of panicking.
📝 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.
| KeyType::BLS12_381 => { | |
| #[cfg(feature = "bls-signatures")] | |
| { | |
| let private_key = | |
| bls_signatures::PrivateKey::from_bytes(private_key_bytes, false) | |
| .map_err(|e| ProtocolError::Generic(e.to_string()))?; | |
| let public_key_bytes = private_key | |
| .g1_element() | |
| .expect("expected to get a public key from a bls private key") | |
| .to_bytes() | |
| .to_vec(); | |
| Ok(public_key_bytes) | |
| } | |
| #[cfg(not(feature = "bls-signatures"))] | |
| return Err(ProtocolError::NotSupported( | |
| "Converting a private key to a bls public key is not supported without the bls-signatures feature".to_string(), | |
| )); | |
| } | |
| KeyType::BLS12_381 => { | |
| #[cfg(feature = "bls-signatures")] | |
| { | |
| let private_key = | |
| bls_signatures::PrivateKey::from_bytes(private_key_bytes, false) | |
| .map_err(|e| ProtocolError::Generic(e.to_string()))?; | |
| let public_key_bytes = private_key | |
| .g1_element() | |
| .map_err(|e| ProtocolError::Generic(e.to_string()))? | |
| .to_bytes() | |
| .to_vec(); | |
| Ok(public_key_bytes) | |
| } | |
| #[cfg(not(feature = "bls-signatures"))] | |
| return Err(ProtocolError::NotSupported( | |
| "Converting a private key to a bls public key is not supported without the bls-signatures feature".to_string(), | |
| )); | |
| } |
QuantumExplorer
changed the title
Issue being fixed or feature implemented
It wasn't convenient to get public key data when we knew the private key data and the key type.
What was done?
Added a convenience method to get the public key data for a private key depending on the key type
How Has This Been Tested?
Breaking Changes
Not breaking
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit