Add Network keypair to enclave init. #209
Conversation
|
Warning Rate limit exceeded@hmzakhalid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces changes to the ciphernode enclave's networking capabilities, focusing on network key management and initialization. The modifications span multiple files, adding support for generating, setting, and managing network keypairs using the Changes
Possibly related PRs
Suggested labels
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: 5
🧹 Nitpick comments (9)
packages/ciphernode/enclave/src/commands/net/set.rs (3)
11-21: Prefer using &str instead of &String for arguments.
Accepting &String may unnecessarily limit function usage. Typical Rust idiomatic practice is to accept &str for string references.-pub fn create_keypair(input: &String) -> Result<Keypair> { +pub fn create_keypair(input: &str) -> Result<Keypair> { match hex::check(input) { ... } }
27-40: Consider capturing repeated logic for user prompts.
The prompt and validation steps here mirror the logic used for password collection or similar interactive steps in your application. If repeated across commands, refactoring into a common prompt helper would DRY up your code.
42-49: Ensure encryption errors are logged at the source.
Although the code returns promptly on encryption errors via the question mark operator (?), consider adding a detailed log entry or trace for encryption failures before returning, to assist in future debugging.packages/ciphernode/enclave/src/commands/init.rs (1)
44-45: Clarity on new parameters
Ensure that public documentation or inline comments mention how net_keypair and generate_net_keypair are used during initialization, especially for developers unfamiliar with the new network capabilities.packages/ciphernode/enclave/src/commands/net/generate.rs (2)
21-27: Improve error handling structureThe error handling block could be more concise and idiomatic.
Consider this more idiomatic approach:
- let errors = bus.send(GetErrors).await?; - if errors.len() > 0 { - for error in errors.iter() { - println!("{error}"); - } - bail!("There were errors generating the network keypair") - } + let errors = bus.send(GetErrors).await?; + if !errors.is_empty() { + errors.iter().for_each(|error| println!("{error}")); + bail!("There were errors generating the network keypair"); + }
18-19: Document encryption format in the commentThe comment about encrypted string would be more helpful if it included details about the encryption format or referred to relevant documentation.
Suggested comment improvement:
- // NOTE: We are writing an encrypted string here + // NOTE: Writing the keypair encrypted using Cipher::encrypt_data + // Format: <encryption details or link to documentation>packages/ciphernode/enclave/src/commands/mod.rs (1)
59-66: Documentation could be more descriptive for network keypair options.The implementation looks good, but consider enhancing the documentation for better clarity:
- Specify the expected format for
net_keypair(e.g., hex-encoded ed25519 private key)- Clarify the precedence between
net_keypairandgenerate_net_keypairoptions- /// The network private key (ed25519) + /// The network private key in hex format (ed25519). If provided, this will be used instead of generating a new keypair. #[arg(long = "net-keypair")] net_keypair: Option<String>, - /// Generate a new network keypair + /// Generate a new ed25519 network keypair if no existing keypair is provided #[arg(long = "generate-net-keypair")] generate_net_keypair: bool,packages/ciphernode/enclave/src/main.rs (1)
61-61: Performance improvement: Config passed by reference.Changed
password::executeto take a reference to config instead of moving ownership, avoiding unnecessary cloning.Consider applying the same optimization to other commands that don't need ownership of config:
- Commands::Aggregator { command } => aggregator::execute(command, config).await?, + Commands::Aggregator { command } => aggregator::execute(command, &config).await?, - Commands::Wallet { command } => wallet::execute(command, config).await?, + Commands::Wallet { command } => wallet::execute(command, &config).await?,packages/ciphernode/net/src/network_manager.rs (1)
86-96: Consider adding keypair format validation.The code directly attempts to convert the decrypted bytes into an ed25519 keypair. Consider adding validation of the decrypted data format before the conversion to provide more specific error messages.
let keypair: libp2p::identity::Keypair = + // Validate keypair format + if bytes.len() != ed25519::KEYPAIR_LENGTH { + bail!("Invalid keypair length in repository"); + } ed25519::Keypair::try_from_bytes(&mut bytes)?.try_into()?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/ciphernode/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
packages/ciphernode/enclave/Cargo.toml(1 hunks)packages/ciphernode/enclave/src/commands/init.rs(4 hunks)packages/ciphernode/enclave/src/commands/mod.rs(1 hunks)packages/ciphernode/enclave/src/commands/net/generate.rs(1 hunks)packages/ciphernode/enclave/src/commands/net/mod.rs(2 hunks)packages/ciphernode/enclave/src/commands/net/purge.rs(1 hunks)packages/ciphernode/enclave/src/commands/net/set.rs(1 hunks)packages/ciphernode/enclave/src/commands/password/mod.rs(1 hunks)packages/ciphernode/enclave/src/commands/wallet/set.rs(2 hunks)packages/ciphernode/enclave/src/main.rs(1 hunks)packages/ciphernode/enclave_node/src/aggregator.rs(1 hunks)packages/ciphernode/enclave_node/src/ciphernode.rs(1 hunks)packages/ciphernode/net/src/network_manager.rs(2 hunks)packages/ciphernode/router/src/repositories.rs(1 hunks)tests/basic_integration/base.sh(1 hunks)tests/basic_integration/fns.sh(3 hunks)tests/basic_integration/persist.sh(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/ciphernode/router/src/repositories.rs (1)
Learnt from: ryardley
PR: gnosisguild/enclave#145
File: packages/ciphernode/router/src/repositories.rs:40-71
Timestamp: 2024-11-10T16:42:46.226Z
Learning: In `packages/ciphernode/router/src/repositories.rs`, prefer to keep method implementations as they are if they are straightforward and maintainable, even if refactoring could reduce duplication.
packages/ciphernode/enclave/src/commands/wallet/set.rs (1)
Learnt from: ryardley
PR: gnosisguild/enclave#197
File: packages/ciphernode/enclave/src/commands/wallet/set.rs:0-0
Timestamp: 2024-12-07T09:21:37.108Z
Learning: Manual private key range checks are unnecessary because Alloy will produce an error if the private key is invalid.
🪛 Shellcheck (0.10.0)
tests/basic_integration/fns.sh
[warning] 19-19: NETWORK_PRIVATE_KEY_AG appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 36-36: NETWORK_PRIVATE_KEY_1 appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 37-37: NETWORK_PRIVATE_KEY_2 appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 38-38: NETWORK_PRIVATE_KEY_3 appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 39-39: NETWORK_PRIVATE_KEY_4 appears unused. Verify use (or export if used externally).
(SC2034)
🪛 Gitleaks (8.21.2)
tests/basic_integration/fns.sh
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
37-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (19)
packages/ciphernode/enclave/src/commands/net/set.rs (1)
50-56: Robust error handling
The mechanism that aggregates all errors from EventBus and logs them is well-designed. This straightforward approach helps debug misconfigurations quickly.
packages/ciphernode/enclave/src/commands/init.rs (2)
32-34: Stronger Ethereum address validation
Leveraging Address::parse_checksummed for validation is more robust and reduces manual parsing errors. Great improvement!
133-143: Handle missing net_keypair gracefully
If generate_net_keypair is false but net_keypair is None, the SetKey command may fail. You may want to provide a fallback user flow or a clearer error message.
packages/ciphernode/enclave/src/commands/net/purge.rs (1)
10-10: Consistent naming aligns with keypair usage
The change from libp2pid() to libp2p_keypair() clarifies that the stored data is a keypair rather than just an identifier. This naming improvement supports a more self-documenting codebase.
packages/ciphernode/enclave/src/commands/net/mod.rs (4)
2-3: New modules introduced
Modules set and generate provide dedicated functionality for keypair management. This separation promotes maintainability and clarity.
13-15: Dedicated command for key generation
Adding a GenerateKey variant in NetCommands is a clear approach to separate generation from setting existing keys. This aligns well with single-responsibility principles.
16-20: Optional net_keypair
Providing a configurable Option for net_keypair is flexible. Consider ensuring that user feedback is clear when providing an invalid or empty string.
23-27: Passing config by reference
Switching from config ownership to a reference helps avoid unnecessary clones or moves. Good for performance and clarity.
packages/ciphernode/enclave/Cargo.toml (1)
22-22: LGTM: Required dependency for network keypair functionality
The addition of libp2p from the workspace is appropriate for implementing the network keypair functionality.
packages/ciphernode/enclave/src/commands/password/mod.rs (1)
33-33: LGTM: Improved efficiency by using reference
Good optimization to change config parameter to use a reference instead of taking ownership. This avoids unnecessary cloning of the AppConfig struct.
packages/ciphernode/enclave/src/commands/wallet/set.rs (1)
11-14: Great improvement to private key validation!
The changes effectively leverage the alloy library's built-in validation, aligning with previous feedback that manual range checks are unnecessary. The two-step validation (hex format + key validity) with clear error messages is a robust approach.
packages/ciphernode/enclave_node/src/ciphernode.rs (1)
75-75: Verify consistent renaming across the codebase.
The rename from libp2pid() to libp2p_keypair() improves clarity. Let's ensure this rename is applied consistently.
✅ Verification successful
Rename from libp2pid() to libp2p_keypair() has been applied consistently
The verification shows:
- No instances of the old method name
libp2pidremain in the codebase - The new method name
libp2p_keypair()is used consistently across all relevant components:- Definition in router/repositories.rs
- Usage in enclave commands (set.rs, purge.rs, generate.rs)
- Usage in node components (aggregator.rs, ciphernode.rs)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old method name
# and verify consistent usage of the new name
echo "Checking for old method name 'libp2pid'..."
rg "libp2pid"
echo -e "\nVerifying consistent usage of new method name 'libp2p_keypair'..."
rg "libp2p_keypair"
Length of output: 884
packages/ciphernode/router/src/repositories.rs (1)
77-78: LGTM! Method rename improves clarity and organization.
The change from libp2pid to libp2p_keypair better reflects the method's purpose, and the new storage path follows a more organized hierarchical structure.
Let's verify all usages of the old method name have been updated:
✅ Verification successful
Method rename was successfully implemented across the codebase
The verification shows that:
- No occurrences of the old method name
libp2pidexist in the codebase - The new method name
libp2p_keypairis consistently used across multiple files:- Defined in
packages/ciphernode/router/src/repositories.rs - Used in
packages/ciphernode/enclave_node/src/ciphernode.rs - Used in
packages/ciphernode/enclave_node/src/aggregator.rs - Used in
packages/ciphernode/enclave/src/commands/net/purge.rs - Used in
packages/ciphernode/enclave/src/commands/net/generate.rs - Used in
packages/ciphernode/enclave/src/commands/net/set.rs
- Defined in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old method name
# Expect: No occurrences of the old method name
rg "libp2pid\(\)" --type rust
Length of output: 31
Script:
#!/bin/bash
# Let's search for both old and new method names to understand the changes better
# and ensure we're not missing any references
# Search for any occurrence of libp2pid (without restricting to function calls)
echo "=== Searching for 'libp2pid' ==="
rg "libp2pid" --type rust -A 2
# Search for the new method name to verify its usage
echo -e "\n=== Searching for 'libp2p_keypair' ==="
rg "libp2p_keypair" --type rust -A 2
# Let's also check the git history for this specific change
echo -e "\n=== Checking git history for this rename ==="
git log -p --all --full-history -S "libp2pid"
Length of output: 1894
packages/ciphernode/enclave_node/src/aggregator.rs (1)
86-86: LGTM! Method call updated consistently.
The change correctly updates the method call to use libp2p_keypair() instead of libp2pid(), maintaining consistency with the repository changes.
packages/ciphernode/enclave/src/main.rs (1)
58-60: LGTM! Network keypair initialization parameters added.
The addition of net_keypair and generate_net_keypair parameters enhances the initialization process by providing explicit control over network key management.
tests/basic_integration/persist.sh (1)
27-32: Identical code block as in base.sh
See the review comment in base.sh about extracting this code to a shared function.
packages/ciphernode/net/src/network_manager.rs (3)
5-5: LGTM: Import addition aligns with new error handling.
The addition of bail to the anyhow imports supports the improved error handling in the setup_with_peer method.
97-101: Verify the impact on existing network connections.
The changes to keypair handling might affect how nodes identify and connect to each other in the network.
Let's check for potential breaking changes:
✅ Verification successful
Let me gather more information about the network connection handling and peer setup by checking the implementation details.
Let me try one more search pattern to understand the network setup and connection handling.
Based on the code analysis results, I can now provide a final response:
The network connection changes appear to be backward compatible.
The implementation shows that peer identification and network setup remain consistent with the existing pattern:
- NetworkPeer creation still uses the same keypair-based identification mechanism
- The peer ID derivation (
to_peer_id()) is used consistently across the codebase - Network setup maintains the same connection flow through the NetworkManager
The changes in the keypair handling don't introduce breaking changes to the network connectivity as the fundamental peer identification and connection mechanisms remain unchanged.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing uses of peer ID or network identity
rg -A 3 "to_peer_id|NetworkPeer::new"
# Check for network connection handling
ast-grep --pattern 'fn setup_with_peer($$$)'
Length of output: 2113
Script:
#!/bin/bash
# Check NetworkManager::setup implementation and related methods
ast-grep --pattern 'fn setup($$$) {
$$$
}'
# Check for peer connection handling
rg -A 5 "handle_connection|connect_to_peer"
# Look for any network event handling
rg -A 3 "NetworkEvent|PeerEvent"
Length of output: 127
Script:
#!/bin/bash
# Look for network setup and connection handling with broader context
rg -A 10 "impl NetworkManager|impl NetworkPeer"
# Check for any connection-related structs or types
rg -A 3 "struct.*Connection|type.*Connection|enum.*Connection"
# Search for network-related error handling
rg -A 3 "Error.*network|network.*Error"
Length of output: 2045
77-83: LGTM: Improved error handling with clear user guidance.
The new implementation provides better error handling with a clear message guiding users on how to resolve the missing keypair issue.
Let's verify the referenced command exists:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/ciphernode/enclave/src/commands/net/set.rs (4)
11-19: Validate user input string length for Ed25519 keys
Although you already check for valid hex, consider validating that the decoded byte length matches what's expected for Ed25519 (32 bytes), rather than relying solely on the internal error from Keypair::ed25519_from_bytes. This can produce clearer error messages for malformed input.
25-29: Unify key handling logic
Both branches (the provided net_keypair and the user-prompted input) perform identical transformations. Consider refactoring to a single function that returns the private key bytes to reduce duplication and make maintenance simpler.
40-41: Consider removing unnecessary clone
You're calling “&mut input.clone()”. Instead of cloning, you could pass the mutable slice of “input”. Reducing unnecessary copies can improve clarity, though the performance impact here is minor.
48-54: Clarify or group error handling
Multiple errors may occur. Consider grouping or clearly labeling them to indicate their cause (e.g., encryption vs. repository write) before output. This helps users understand which step failed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/ciphernode/enclave/src/commands/net/generate.rs(1 hunks)packages/ciphernode/enclave/src/commands/net/mod.rs(2 hunks)packages/ciphernode/enclave/src/commands/net/set.rs(1 hunks)packages/ciphernode/enclave/src/main.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ciphernode/enclave/src/commands/net/generate.rs
- packages/ciphernode/enclave/src/commands/net/mod.rs
- packages/ciphernode/enclave/src/main.rs
🔇 Additional comments (1)
packages/ciphernode/enclave/src/commands/net/set.rs (1)
30-35: Potential security concern: storing private key in memory
Sensitive data remains in-memory (in variable kp) even after validation. While this is common in Rust, consider zeroing out memory after use if feasible to reduce the risk of exposing private keys in memory dumps.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests