feat: fix node setup UX [skip-line-limit]#1338
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a new interactive Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Setup
participant Config as Config Manager
participant Password as Password Setup
participant Wallet as Wallet Setup
participant Net as Net Keypair Setup
User->>CLI: enclave ciphernode setup [options]
CLI->>CLI: Validate/prompt for RPC URL
CLI->>CLI: Resolve password (prompt if needed)
CLI->>CLI: Resolve private key (prompt if needed)
CLI->>Wallet: Derive address from key
CLI->>CLI: Prompt for config directory
CLI->>Config: execute(rpc_url, config_dir, address)
Config-->>CLI: AppConfig created
CLI->>Password: execute(config, password)
Password-->>CLI: Password stored & encrypted
CLI->>Wallet: execute(config, private_key)
Wallet-->>CLI: Wallet key stored & encrypted
alt Generate net keypair
CLI->>Net: generate_net_keypair()
else Use provided net keypair
CLI->>Net: set_net_keypair(keypair)
else Auto-generate if none provided
CLI->>Net: auto_generate()
end
Net-->>CLI: Net keypair configured
CLI-->>User: ✓ Enclave configuration successfully created!
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ryardley
changed the title
Contributor
Author
|
yeah I need to fix the tests to use real addresses not going to finish this tonight but will do so tomorrow |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
crates/entrypoint/src/config_set/mod.rs (1)
41-41: Prefer&Pathover&PathBufin function signatures.Idiomatic Rust prefers
&PathsincePathBufauto-derefs toPath, and accepting&Pathis more flexible for callers.♻️ Suggested change
+use std::path::Path; -pub fn execute(rpc_url: String, config_dir: &PathBuf, address: &Address) -> Result<AppConfig> { +pub fn execute(rpc_url: String, config_dir: &Path, address: &Address) -> Result<AppConfig> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/entrypoint/src/config_set/mod.rs` at line 41, Change the function signature of execute to accept a reference to Path instead of PathBuf: replace the parameter type config_dir: &PathBuf with config_dir: &Path in the execute function so callers can pass any Path-like type; update any internal uses (e.g., methods expecting &PathBuf) to work with &Path (use .to_path_buf() only when an owned PathBuf is required) and update all callers to pass a &Path (or let the compiler auto-deref other path types). Ensure necessary use statements (std::path::Path) are present and adjust any type annotations that assumed PathBuf.crates/cli/src/password_set.rs (1)
13-18: Inconsistent trimming between provided and prompted password paths.When a password is provided via argument (line 18), it's returned as-is (untrimmed). When entered interactively (line 40), it's trimmed. This means a CLI-passed password
" foo "would be stored differently than if the same string were typed at the prompt.Suggested fix
if let Some(pw_str) = input { if pw_str.trim().is_empty() { bail!("Password must not be blank") } - return Ok(pw_str); + let trimmed = Zeroizing::new(pw_str.trim().to_owned()); + return Ok(trimmed); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/password_set.rs` around lines 13 - 18, The ask_for_password function treats argument-provided passwords and prompted passwords inconsistently: trim the provided Zeroizing<String> the same way as the interactive path to ensure identical handling; specifically, in ask_for_password when handling the Some(pw_str) branch, apply the same trimming/check (e.g., call trim() on the string and bail if empty) and return the trimmed Zeroizing<String> so the return value matches what prompt_password returns.crates/cli/src/cli.rs (1)
121-131:enclave startwith no config runs setup but doesn't start the node or inform the user.When a user runs
enclave startwithout an existing config, setup runs and then the function returns (line 140). The node never actually starts, and unlike the explicit setup path (line 119), there's no message telling the user to runenclave startagain.Consider adding a hint after setup completes:
ciphernode::setup::execute( None, None, None, None, false, ) .await?; + println!("Setup complete. Please run `enclave start` again to start your node.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/cli.rs` around lines 121 - 131, The Commands::Start match arm currently runs ciphernode::setup::execute when no config exists but then returns, leaving the node stopped and the user uninformed; update that branch so that after awaiting ciphernode::setup::execute you either (a) invoke the same start logic used for the normal start path (reuse or call the function that actually launches the node) so the node begins immediately, or (b) print a clear hint like "Setup complete — run `enclave start` again to start the node" (or both). Locate the Commands::Start match arm and the ciphernode::setup::execute call and add the follow-up action (start invocation or user message) to ensure the user knows the next step or the node starts automatically.crates/cli/src/wallet.rs (1)
20-21: Consider updatingensure_hexto returnZeroizing<String>to avoid plaintext key exposure in intermediate String.The parser currently returns
Result<String>, which means the plaintext key briefly exists in a non-zeroizingStringbefore clap converts it toZeroizing<String>. For sensitive data like private keys, it's better to wrap the result immediately:Suggested improvement
-fn ensure_hex(s: &str) -> Result<String> { +fn ensure_hex(s: &str) -> Result<Zeroizing<String>> { if !s.starts_with("0x") { bail!("hex value must start with '0x'") } hex::decode(&s[2..])?; - Ok(s.to_string()) + Ok(Zeroizing::new(s.to_string())) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/wallet.rs` around lines 20 - 21, The parser ensure_hex currently returns Result<String>, briefly creating a non-zeroizing String for sensitive private-key input; change ensure_hex to return Result<Zeroizing<String>> so the sensitive bytes are immediately wrapped and never exist in a plain String, update its signature and any call sites (e.g., the clap arg value_parser used with private_key) to accept Result<Zeroizing<String>>, and ensure any internal string construction uses Zeroizing::new(...) or maps the existing String into Zeroizing before returning to avoid exposing plaintext in memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 416-426: The comparison between signer.address().to_string() and
addr is case-sensitive; instead parse the config addr string into the same
Address type and compare the Address values (or at minimum use a
case-insensitive comparison) before bailing. In the block guarded by
self.ignore_address_check where you call provider_cache.ensure_signer().await?
and use signer.address(), parse addr into an ethers::types::Address (handle
parse errors) and compare it to signer.address() for equality (or call
.eq_ignore_ascii_case if you prefer string comparison), then bail only if they
differ.
In `@crates/cli/src/wallet_set.rs`:
- Line 36: The success message printed in wallet_set.rs currently reads
"WalletKey key has been successfully stored and encrypted." — update the println
in the wallet_set module (the println! call) to correct the typo to a clearer
message such as "Wallet key has been successfully stored and encrypted." or
"Private key has been successfully stored and encrypted." so it reads naturally.
In `@docs/pages/ciphernode-operators/running.mdx`:
- Line 62: The "Initialize Configuration" step currently shows the wrong command
string "enclave ciphernode start"; update that text to "enclave ciphernode
setup" so it matches the intended initialization command (leave the "Start Your
Node" section using "enclave start -v" as-is). Search for the literal "enclave
ciphernode start" in the document and replace it with "enclave ciphernode setup"
to correct the documentation.
---
Nitpick comments:
In `@crates/cli/src/cli.rs`:
- Around line 121-131: The Commands::Start match arm currently runs
ciphernode::setup::execute when no config exists but then returns, leaving the
node stopped and the user uninformed; update that branch so that after awaiting
ciphernode::setup::execute you either (a) invoke the same start logic used for
the normal start path (reuse or call the function that actually launches the
node) so the node begins immediately, or (b) print a clear hint like "Setup
complete — run `enclave start` again to start the node" (or both). Locate the
Commands::Start match arm and the ciphernode::setup::execute call and add the
follow-up action (start invocation or user message) to ensure the user knows the
next step or the node starts automatically.
In `@crates/cli/src/password_set.rs`:
- Around line 13-18: The ask_for_password function treats argument-provided
passwords and prompted passwords inconsistently: trim the provided
Zeroizing<String> the same way as the interactive path to ensure identical
handling; specifically, in ask_for_password when handling the Some(pw_str)
branch, apply the same trimming/check (e.g., call trim() on the string and bail
if empty) and return the trimmed Zeroizing<String> so the return value matches
what prompt_password returns.
In `@crates/cli/src/wallet.rs`:
- Around line 20-21: The parser ensure_hex currently returns Result<String>,
briefly creating a non-zeroizing String for sensitive private-key input; change
ensure_hex to return Result<Zeroizing<String>> so the sensitive bytes are
immediately wrapped and never exist in a plain String, update its signature and
any call sites (e.g., the clap arg value_parser used with private_key) to accept
Result<Zeroizing<String>>, and ensure any internal string construction uses
Zeroizing::new(...) or maps the existing String into Zeroizing before returning
to avoid exposing plaintext in memory.
In `@crates/entrypoint/src/config_set/mod.rs`:
- Line 41: Change the function signature of execute to accept a reference to
Path instead of PathBuf: replace the parameter type config_dir: &PathBuf with
config_dir: &Path in the execute function so callers can pass any Path-like
type; update any internal uses (e.g., methods expecting &PathBuf) to work with
&Path (use .to_path_buf() only when an owned PathBuf is required) and update all
callers to pass a &Path (or let the compiler auto-deref other path types).
Ensure necessary use statements (std::path::Path) are present and adjust any
type annotations that assumed PathBuf.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/cli/src/helpers/mod.rs (1)
24-33: Error message in generic helper is domain-specific.
ensure_hexis a general-purpose hex validator, but Line 30's error message says "private key must only contain hex characters." If this function is reused for non-private-key hex values, the message will be confusing.Suggested fix
if !s[2..].chars().all(|c| c.is_ascii_hexdigit()) { - bail!("private key must only contain hex characters [0-9a-fA-F]"); + bail!("hex value must only contain hex characters [0-9a-fA-F]"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/helpers/mod.rs` around lines 24 - 33, The helper function ensure_hex currently emits a domain-specific error ("private key must only contain hex characters"), so update its error text to be generic: change the second bail! message to something like "hex value must only contain hex characters [0-9a-fA-F]" (and optionally make the first bail! message consistent, e.g., "hex value must start with '0x'") in the ensure_hex function to avoid referencing "private key" for non-key usage.crates/cli/src/ciphernode/mod.rs (2)
44-50: Sensitive CLI args will appear in/proc/<pid>/cmdlineand shell history.Both
--passwordand--private-keyaccept secrets as direct CLI arguments. WhileZeroizingprotects in-process memory, the values remain visible in the process list and shell history. Since the PR already implements interactive prompts as the primary UX (prompting when not supplied), consider documenting that users should prefer the interactive prompt over passing these flags directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/ciphernode/mod.rs` around lines 44 - 50, The CLI currently accepts sensitive secrets via the password and private_key args (fields password and private_key using parse_zeroizing and ensure_hex_zeroizing) which can leak to /proc and shell history; update the arg help text and user docs to clearly warn users to prefer the interactive prompt (i.e., when these Options are None) rather than passing secrets on the command line, and add an explicit warning in the CLI --help and project README near the ciphernode usage; optionally mark these args as hidden or document mitigation (e.g., use env/interactive) in the same place so users see the security guidance.
52-54: Consider wrappingnet_keypairinZeroizing<String>for consistency with other sensitive fields.The
passwordandprivate_keyfields useZeroizing<String>to scrub sensitive data from memory, butnet_keypair(an ed25519 private key) is a plainOption<String>. For consistency and defense-in-depth, applying the same zeroization would be prudent.Note: The net keypair is encrypted immediately after being passed to the downstream handler, so the window for exposure is small. If applied, ensure the downstream
net::keypair::set::executecan acceptZeroizing<String>.Suggested approach
/// The network private key (ed25519) - #[arg(long = "net-keypair", short = 'n')] - net_keypair: Option<String>, + #[arg(long = "net-keypair", short = 'n', value_parser = parse_zeroizing)] + net_keypair: Option<Zeroizing<String>>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/ciphernode/mod.rs` around lines 52 - 54, Wrap the net_keypair field's type in Zeroizing<String> (i.e., change net_keypair: Option<String> to net_keypair: Option<Zeroizing<String>>) so the ed25519 private key is zeroed from memory like password and private_key; update any constructors/deserializers that build the struct and convert incoming strings into Zeroizing<String> before storing, and adjust the downstream call site net::keypair::set::execute to accept or be given a &str/owned String by unwrapping or cloning the Zeroizing value as needed (or update that function signature to accept Zeroizing<String>) to ensure compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 416-426: The address comparison in the block guarded by
self.ignore_address_check is already correct: it normalizes both the config
address (addr) and the signer's address (signer.address()) to lowercase before
comparing; no code changes are required—retain the ensure_signer() call and the
current lowercase normalization of signer.address().to_string() and addr to
preserve EIP-55 casing tolerance.
---
Nitpick comments:
In `@crates/cli/src/ciphernode/mod.rs`:
- Around line 44-50: The CLI currently accepts sensitive secrets via the
password and private_key args (fields password and private_key using
parse_zeroizing and ensure_hex_zeroizing) which can leak to /proc and shell
history; update the arg help text and user docs to clearly warn users to prefer
the interactive prompt (i.e., when these Options are None) rather than passing
secrets on the command line, and add an explicit warning in the CLI --help and
project README near the ciphernode usage; optionally mark these args as hidden
or document mitigation (e.g., use env/interactive) in the same place so users
see the security guidance.
- Around line 52-54: Wrap the net_keypair field's type in Zeroizing<String>
(i.e., change net_keypair: Option<String> to net_keypair:
Option<Zeroizing<String>>) so the ed25519 private key is zeroed from memory like
password and private_key; update any constructors/deserializers that build the
struct and convert incoming strings into Zeroizing<String> before storing, and
adjust the downstream call site net::keypair::set::execute to accept or be given
a &str/owned String by unwrapping or cloning the Zeroizing value as needed (or
update that function signature to accept Zeroizing<String>) to ensure
compatibility.
In `@crates/cli/src/helpers/mod.rs`:
- Around line 24-33: The helper function ensure_hex currently emits a
domain-specific error ("private key must only contain hex characters"), so
update its error text to be generic: change the second bail! message to
something like "hex value must only contain hex characters [0-9a-fA-F]" (and
optionally make the first bail! message consistent, e.g., "hex value must start
with '0x'") in the ensure_hex function to avoid referencing "private key" for
non-key usage.
added 18 commits
February 19, 2026 22:54
- Add to retrieve wallet address - Return address and peer_id from wallet set for display - Add Flush message to ensure data is persisted to disk - Show address and peer_id after setup with colorized output
hmzakhalid
previously approved these changes
Feb 19, 2026
Contributor
Author
|
as soon as I can build and run this locally will approve or make changess. |
hmzakhalid
approved these changes
Feb 20, 2026
6 tasks
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #1337
This fixes up some UX issues I came across setting up the ciphernode on a box.
This PR:
ciphernode setupcommand: Consolidated oldconfig-setintoenclave ciphernode setuprefactored to collect all inputs first before executing.(current working directory)Zeroizingwrappers to securely zero memory--eth-addressflag)skip_ethbypass: Eliminated ability to skip Ethereum address configurationconfig-setnow point tociphernode setupExample output:
Summary by CodeRabbit
Release Notes
New Features
enclave ciphernode setupcommand as the unified initialization flow, replacing the legacyenclave config-setcommand.Changes