Conversation
|
Hi, this is WorkflowBot.
|
WalkthroughThis PR introduces two new TCK handler modules (consensus and key) for topic creation and key generation operations, updates SDK client initialization to support legacy node setups and DER-encoded keys, and enables Flask debug mode in the server startup. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22cb49ee-f332-49c3-a953-3835c7263769
📒 Files selected for processing (5)
tck/handlers/__init__.pytck/handlers/consensus.pytck/handlers/key.pytck/handlers/sdk.pytck/server.py
| try: | ||
| # Try to parse as a private key first | ||
| try: | ||
| return PrivateKey.from_string(key_string) | ||
| except Exception: | ||
| # If it fails, try as a public key | ||
| return PublicKey.from_string(key_string) | ||
| except Exception as e: | ||
| raise JsonRpcError(INTERNAL_ERROR, f"Failed to parse key: {str(e)}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using INVALID_PARAMS instead of INTERNAL_ERROR for key parsing failures.
When a user provides an invalid key string, this is an input validation error rather than an internal server error. Using INTERNAL_ERROR may mislead clients about the nature of the failure.
♻️ Proposed fix
except Exception as e:
- raise JsonRpcError(INTERNAL_ERROR, f"Failed to parse key: {str(e)}")
+ raise JsonRpcError(INVALID_PARAMS, f"Invalid key format: {str(e)}")| except Exception as e: | ||
| raise JsonRpcError(INTERNAL_ERROR, f"Failed to parse signer key: {str(e)}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same error code concern: Consider using INVALID_PARAMS for signer key parsing failures.
Consistent with the _parse_key function, invalid signer keys are user input errors.
♻️ Proposed fix
except Exception as e:
- raise JsonRpcError(INTERNAL_ERROR, f"Failed to parse signer key: {str(e)}")
+ raise JsonRpcError(INVALID_PARAMS, f"Invalid signer key format: {str(e)}")📝 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.
| except Exception as e: | |
| raise JsonRpcError(INTERNAL_ERROR, f"Failed to parse signer key: {str(e)}") | |
| except Exception as e: | |
| raise JsonRpcError(INVALID_PARAMS, f"Invalid signer key format: {str(e)}") |
| try: | ||
| period = int(params["autoRenewPeriod"]) | ||
| # Validate auto-renew period (approximate range: 6999999 to 8000001) | ||
| if period <= 0 or period < 6999999 or period > 8000001: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant condition in auto-renew period validation.
The condition period <= 0 or period < 6999999 is redundant since period < 6999999 already covers all non-positive values.
♻️ Proposed simplification
- if period <= 0 or period < 6999999 or period > 8000001:
+ if period < 6999999 or period > 8000001:📝 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.
| if period <= 0 or period < 6999999 or period > 8000001: | |
| if period < 6999999 or period > 8000001: |
| except ValueError: | ||
| raise JsonRpcError(INVALID_PARAMS, "Invalid params", "autoRenewPeriod must be a string representing an integer") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing exception chaining.
The raise statement discards the original exception context. Use raise ... from None if intentional, or preserve the chain for debugging.
♻️ Proposed fix to preserve exception chain
except ValueError:
- raise JsonRpcError(INVALID_PARAMS, "Invalid params", "autoRenewPeriod must be a string representing an integer")
+ raise JsonRpcError(INVALID_PARAMS, "Invalid params", "autoRenewPeriod must be a string representing an integer") from None| def _key_to_hex(key: Any) -> str: | ||
| """Convert a key to its DER-encoded hex representation. | ||
|
|
||
| Args: | ||
| key: PrivateKey or PublicKey instance | ||
|
|
||
| Returns: | ||
| Hex string representation of the key | ||
| """ | ||
| proto_key = key_to_proto(key) | ||
| if proto_key is None: | ||
| raise JsonRpcError(INTERNAL_ERROR, "Failed to convert key to protobuf") | ||
|
|
||
| # Serialize to bytes and convert to hex | ||
| key_bytes = proto_key.SerializeToString() | ||
| return key_bytes.hex().upper() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify key_to_proto extracts public key from private key
ast-grep --pattern $'def key_to_proto($$$):
$$$
'Repository: MonaaEid/hiero-sdk-python
Length of output: 51
🏁 Script executed:
cd /tmp && find . -name "key_utils.py" -type f 2>/dev/null | head -5Repository: MonaaEid/hiero-sdk-python
Length of output: 51
🏁 Script executed:
rg "def key_to_proto" --type py -A 10Repository: MonaaEid/hiero-sdk-python
Length of output: 1056
🏁 Script executed:
rg "key_to_proto" --type py -B 2 -A 5 | head -50Repository: MonaaEid/hiero-sdk-python
Length of output: 3238
🏁 Script executed:
sed -n '30,50p' src/hiero_sdk_python/utils/key_utils.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 712
🏁 Script executed:
cat src/hiero_sdk_python/utils/key_utils.py | head -80Repository: MonaaEid/hiero-sdk-python
Length of output: 1739
🏁 Script executed:
cat -n tck/handlers/key.py | sed -n '50,70p'Repository: MonaaEid/hiero-sdk-python
Length of output: 1049
🏁 Script executed:
cat -n tck/handlers/key.py | sed -n '110,165p'Repository: MonaaEid/hiero-sdk-python
Length of output: 3241
🏁 Script executed:
rg "def to_string_der" --type py -A 3Repository: MonaaEid/hiero-sdk-python
Length of output: 665
🏁 Script executed:
rg "class PrivateKey" --type py -A 20 | head -50Repository: MonaaEid/hiero-sdk-python
Length of output: 1629
🏁 Script executed:
rg "def to_string_der\|def to_bytes_raw\|def to_string_raw" src/hiero_sdk_python/crypto/private_key.py -A 5Repository: MonaaEid/hiero-sdk-python
Length of output: 51
🏁 Script executed:
cat -n src/hiero_sdk_python/crypto/private_key.py | grep -A 10 "def to_string_der"Repository: MonaaEid/hiero-sdk-python
Length of output: 469
🏁 Script executed:
cat -n src/hiero_sdk_python/crypto/private_key.py | sed -n '386,400p'Repository: MonaaEid/hiero-sdk-python
Length of output: 614
Critical: _key_to_hex returns public key hex for private keys, not private key hex.
The key_to_proto function in src/hiero_sdk_python/utils/key_utils.py (lines 38-46) extracts and converts the public key when a PrivateKey is passed: return key.public_key()._to_proto(). This means _key_to_hex(private_key) serializes the public key, not the private key.
This affects:
- Lines 57-58:
ed25519PrivateKeyendpoint - Lines 65-66:
ecdsaSecp256k1PrivateKeyendpoint - Lines 115, 119:
keyListprivate key serialization - Lines 158, 162:
thresholdKeyprivate key serialization
Use PrivateKey.to_string_der() for private key serialization:
🐛 Proposed fix
def _key_to_hex(key: Any) -> str:
- """Convert a key to its DER-encoded hex representation.
+ """Convert a key to its protobuf-serialized hex representation.
+
+ Note: For PrivateKey, this returns the PUBLIC key's protobuf serialization,
+ as key_to_proto extracts the public key. Use PrivateKey.to_string_der() for private keys.
Args:
key: PrivateKey or PublicKey instanceThen update private key handlers:
if key_type == "ed25519PrivateKey":
private_key = PrivateKey.generate_ed25519()
+ private_key_hex = private_key.to_string_der()
return {
- "key": _key_to_hex(private_key),
- "privateKeys": [_key_to_hex(private_key)]
+ "key": private_key_hex,
+ "privateKeys": [private_key_hex]
}Apply same fix to ecdsaSecp256k1PrivateKey, and update keyList/thresholdKey private key appends to use private_key.to_string_der() instead of _key_to_hex(private_key).
📝 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.
| def _key_to_hex(key: Any) -> str: | |
| """Convert a key to its DER-encoded hex representation. | |
| Args: | |
| key: PrivateKey or PublicKey instance | |
| Returns: | |
| Hex string representation of the key | |
| """ | |
| proto_key = key_to_proto(key) | |
| if proto_key is None: | |
| raise JsonRpcError(INTERNAL_ERROR, "Failed to convert key to protobuf") | |
| # Serialize to bytes and convert to hex | |
| key_bytes = proto_key.SerializeToString() | |
| return key_bytes.hex().upper() | |
| def _key_to_hex(key: Any) -> str: | |
| """Convert a key to its protobuf-serialized hex representation. | |
| Note: For PrivateKey, this returns the PUBLIC key's protobuf serialization, | |
| as key_to_proto extracts the public key. Use PrivateKey.to_string_der() for private keys. | |
| Args: | |
| key: PrivateKey or PublicKey instance | |
| Returns: | |
| Hex string representation of the key | |
| """ | |
| proto_key = key_to_proto(key) | |
| if proto_key is None: | |
| raise JsonRpcError(INTERNAL_ERROR, "Failed to convert key to protobuf") | |
| # Serialize to bytes and convert to hex | |
| key_bytes = proto_key.SerializeToString() | |
| return key_bytes.hex().upper() |
| elif key_type == "ed25519PublicKey": | ||
| # Get or generate Ed25519 public key | ||
| if "fromKey" in params and params["fromKey"]: | ||
| # Derive public key from private key | ||
| private_key = PrivateKey.from_string(params["fromKey"]) | ||
| public_key = private_key.public_key() | ||
| else: | ||
| # Generate new key pair and return public key | ||
| private_key = PrivateKey.generate_ed25519() | ||
| public_key = private_key.public_key() | ||
|
|
||
| return { | ||
| "key": _key_to_hex(public_key) | ||
| } | ||
|
|
||
| elif key_type == "ecdsaSecp256k1PublicKey": | ||
| # Get or generate ECDSA (secp256k1) public key | ||
| if "fromKey" in params and params["fromKey"]: | ||
| # Derive public key from private key | ||
| private_key = PrivateKey.from_string(params["fromKey"]) | ||
| public_key = private_key.public_key() | ||
| else: | ||
| # Generate new key pair and return public key | ||
| private_key = PrivateKey.generate_ecdsa_secp256k1() | ||
| public_key = private_key.public_key() | ||
|
|
||
| return { | ||
| "key": _key_to_hex(public_key) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if PrivateKey has methods to determine key type
ast-grep --pattern $'class PrivateKey:
$$$
def is_ed25519($$$):
$$$
$$$
}'Repository: MonaaEid/hiero-sdk-python
Length of output: 51
🏁 Script executed:
# First, let's explore the repository structure and find where PrivateKey is imported from
cat tck/handlers/key.py | head -30Repository: MonaaEid/hiero-sdk-python
Length of output: 1082
🏁 Script executed:
# Also search for the PrivateKey class definition
fd -t f -e py | xargs rg "class PrivateKey" -lRepository: MonaaEid/hiero-sdk-python
Length of output: 111
🏁 Script executed:
# Look for the from_string method definition
rg "def from_string" -A 5 -B 2Repository: MonaaEid/hiero-sdk-python
Length of output: 12219
🏁 Script executed:
sed -n '1,100p' src/hiero_sdk_python/crypto/private_key.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 3707
🏁 Script executed:
# Look for the full from_string method
sed -n '/def from_string/,/^[[:space:]]*def /p' src/hiero_sdk_python/crypto/private_key.py | head -50Repository: MonaaEid/hiero-sdk-python
Length of output: 1619
🏁 Script executed:
# Check if PrivateKey has any methods/properties to determine its type
rg "def.*type|self\._.*type|self\.key_type" src/hiero_sdk_python/crypto/private_key.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 231
🏁 Script executed:
# View the specific lines 69-97 in the handler file
sed -n '69,97p' tck/handlers/key.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 1342
🏁 Script executed:
# Find and examine the from_bytes method
sed -n '/def from_bytes/,/^[[:space:]]*def /p' src/hiero_sdk_python/crypto/private_key.py | head -80Repository: MonaaEid/hiero-sdk-python
Length of output: 2012
🏁 Script executed:
# Also check if PrivateKey has a method to get/determine its type
rg "def.*_key_type|def.*is_ed|def.*is_ecdsa|_private_key\.__class__" src/hiero_sdk_python/crypto/private_key.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 134
🏁 Script executed:
# Check the full context around lines 69-97 and see if there's any validation
sed -n '50,110p' tck/handlers/key.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 2650
🏁 Script executed:
# Also look for the is_ed25519 and is_ecdsa method implementations
sed -n '/def is_ed25519/,/^[[:space:]]*def /p' src/hiero_sdk_python/crypto/private_key.py | head -30Repository: MonaaEid/hiero-sdk-python
Length of output: 348
🏁 Script executed:
# Check is_ecdsa as well
sed -n '/def is_ecdsa/,/^[[:space:]]*def /p' src/hiero_sdk_python/crypto/private_key.py | head -30Repository: MonaaEid/hiero-sdk-python
Length of output: 342
Use key-type-specific loaders and validate key types when deriving from fromKey.
When fromKey is provided for ecdsaSecp256k1PublicKey, the code uses PrivateKey.from_string(), which defaults to Ed25519 for 32-byte keys (trying Ed25519 first, then ECDSA). This means a 32-byte ECDSA private key will be silently interpreted as Ed25519, producing an incorrect public key.
Fix: For ecdsaSecp256k1PublicKey, use PrivateKey.from_string_ecdsa() instead of PrivateKey.from_string(). Alternatively, validate the loaded key type using private_key.is_ed25519() or private_key.is_ecdsa() matches the requested type.
| for key_spec in keys_spec: | ||
| if not isinstance(key_spec, dict): | ||
| raise JsonRpcError(INVALID_PARAMS, "Each key spec must be a dict") | ||
|
|
||
| spec_type = key_spec.get("type") | ||
| if spec_type == "ed25519PrivateKey": | ||
| private_key = PrivateKey.generate_ed25519() | ||
| private_keys_hex.append(_key_to_hex(private_key)) | ||
| proto_key = key_to_proto(private_key) | ||
| elif spec_type == "ecdsaSecp256k1PrivateKey": | ||
| private_key = PrivateKey.generate_ecdsa_secp256k1() | ||
| private_keys_hex.append(_key_to_hex(private_key)) | ||
| proto_key = key_to_proto(private_key) | ||
| else: | ||
| raise JsonRpcError(INVALID_PARAMS, f"Unsupported key type in keyList: {spec_type}") | ||
|
|
||
| if proto_key: | ||
| key_list_proto.keys.append(proto_key) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Code duplication in key spec parsing.
The key spec parsing logic for keyList (lines 108-125) and thresholdKey (lines 151-168) is nearly identical. Consider extracting a helper function to reduce duplication.
♻️ Proposed refactoring
def _parse_key_spec(key_spec: Dict[str, Any]) -> tuple[PrivateKey, Any]:
"""Parse a key spec and return (private_key, proto_key)."""
if not isinstance(key_spec, dict):
raise JsonRpcError(INVALID_PARAMS, "Each key spec must be a dict")
spec_type = key_spec.get("type")
if spec_type == "ed25519PrivateKey":
private_key = PrivateKey.generate_ed25519()
elif spec_type == "ecdsaSecp256k1PrivateKey":
private_key = PrivateKey.generate_ecdsa_secp256k1()
else:
raise JsonRpcError(INVALID_PARAMS, f"Unsupported key type: {spec_type}")
return private_key, key_to_proto(private_key)Also applies to: 151-168
| config = ServerConfig() | ||
| logger.info(f"Starting TCK server on {config.host}:{config.port}") | ||
| app.run(host=config.host, port=config.port) | ||
| app.run(host=config.host, port=config.port, debug=True) |
There was a problem hiding this comment.
Security concern: Debug mode enabled in Flask.
Enabling debug=True exposes the Werkzeug interactive debugger, which can execute arbitrary Python code if an attacker can trigger an exception. This is a security risk if this server is accessible beyond localhost.
Consider making debug mode configurable via environment variable or disabling it for non-development environments:
🛡️ Proposed fix to make debug mode configurable
- app.run(host=config.host, port=config.port, debug=True)
+ debug_mode = os.getenv("TCK_DEBUG", "false").lower() == "true"
+ app.run(host=config.host, port=config.port, debug=debug_mode)
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
|
Hi there! I'm the LinkedIssueBot.
Thank you, |
1 participant
No description provided.