Conversation
|
Claude finished @iamquang95's task in 3m 16s —— View job Code Review:
|
| # | Severity | Area |
|---|---|---|
| 1 | Medium | Double verify in definition-file path |
| 2 | Medium | Node dir check skipped for definition-file |
| 3 | Medium | validate_threshold wrong when --nodes absent |
| 4 | Low | Misleading ValueExceedsU8 error variant |
| 5 | Medium | Box::leak memory leak in From<TestnetConfig> |
| 6 | Low | Duplicate agg_sign |
| 7 | Low | Redundant double check |
| 8 | Medium | HTTP/HTTPS check vs error message mismatch |
| 9 | Low | No rollback on partial keymanager import |
| 10 | Low | Module coupling |
| 11 | Low | Stale "charon" branding |
| 12 | Trivial | Unnecessary clone |
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
Overall OK, but there are a couple of problems:
- Missing tracing initialization
- The validation of arguments is done in a different order, potentially causing issues
- There is a missing address checksum
- A check in Charon that results in a warning is a hard error here.
The rests are nits or QoL changes that don't fundamentally affect the code.
crates/cli/src/error.rs
Outdated
|
|
||
| /// Errors that can occur in the Pluto CLI. | ||
| #[derive(Error, Debug)] | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Unnecessary attribute.
crates/cli/src/error.rs
Outdated
| process::{ExitCode, Termination}, | ||
| }; | ||
|
|
||
| use pluto_eth2util as eth2util; |
There was a problem hiding this comment.
nit: no need for these kind of aliases, makes it a bit harder to follow the code.
| impl From<TestnetConfig> for network::Network { | ||
| fn from(config: TestnetConfig) -> Self { | ||
| network::Network { | ||
| chain_id: config.chain_id.unwrap_or(0), | ||
| name: Box::leak( | ||
| config | ||
| .testnet_name | ||
| .as_ref() | ||
| .unwrap_or(&String::new()) | ||
| .clone() | ||
| .into_boxed_str(), | ||
| ), | ||
| genesis_fork_version_hex: Box::leak( | ||
| config | ||
| .fork_version | ||
| .as_ref() | ||
| .unwrap_or(&String::new()) | ||
| .clone() | ||
| .into_boxed_str(), | ||
| ), | ||
| genesis_timestamp: config.genesis_timestamp.unwrap_or(0), | ||
| capella_hard_fork: "", | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: the Box::leak is fine here since we only need it once, but I'd rather make changes in pluto_eth2util::network::Network to support non static strings.
| let files = load_files_unordered(&split_keys_dir).await?; | ||
| Ok(files.sequenced_keys()?) | ||
| } else { | ||
| let files = load_files_recursively(&split_keys_dir).await?; |
There was a problem hiding this comment.
nit: unnecessary refs here
| let files = load_files_unordered(&split_keys_dir).await?; | |
| Ok(files.sequenced_keys()?) | |
| } else { | |
| let files = load_files_recursively(&split_keys_dir).await?; | |
| let files = load_files_unordered(split_keys_dir).await?; | |
| Ok(files.sequenced_keys()?) | |
| } else { | |
| let files = load_files_recursively(split_keys_dir).await?; |
| /// Aggregates BLS signatures over `message` produced by every private share | ||
| /// across all validators, mirroring Go's `aggSign`. | ||
| /// | ||
| /// `share_sets` — outer dimension is per-validator, inner is per-node private | ||
| /// key share. | ||
| fn agg_sign(share_sets: &[Vec<PrivateKey>], message: &[u8]) -> Result<Vec<u8>> { |
There was a problem hiding this comment.
The comment and parameter names are a bit confusing, they deviate from Charon unnecessarily.
| let node_sig = | ||
| pluto_k1util::sign(op_key, &lock.lock_hash).map_err(CreateClusterError::K1UtilError)?; |
| tokio::fs::write(&lock_path, &bytes) | ||
| .await | ||
| .map_err(CreateClusterError::IoError)?; | ||
|
|
||
| let perms = std::fs::Permissions::from_mode(0o400); | ||
| tokio::fs::set_permissions(&lock_path, perms) | ||
| .await | ||
| .map_err(CreateClusterError::IoError)?; | ||
| } |
| .await | ||
| .map_err(CreateClusterError::IoError)?; | ||
|
|
||
| let perms = std::fs::Permissions::from_mode(0o400); |
There was a problem hiding this comment.
nit: we could use set_readonly here but it's fine either way. We should try be consistent throughout the codebase though.
| Ok(()) | ||
| } | ||
|
|
||
| fn write_split_keys_warning(w: &mut dyn Write) -> Result<()> { |
There was a problem hiding this comment.
Unnecessary map_err in multiple places.
|
I'd suggest also taking a look at Claude's findings, some of them are relevant. |
| fork_version, | ||
| pluto_cluster::definition::Creator::default(), | ||
| operators, | ||
| args.deposit_amounts.clone(), |
There was a problem hiding this comment.
eth2util::deposit::eths_to_gweis(&args.deposit_amounts)
args.deposit_amounts is ETH, not gwei
There was a problem hiding this comment.
I didn't get this one, we are converting eth to gwei, as it's done in the go implementation.
There was a problem hiding this comment.
args.deposit_amounts is input as ETH List of partial deposit amounts (integers) in ETH
But the deposit_amounts in Definition should be stored as gwei
3 participants
Summary
Implements the initial pluto create cluster command and wires it into the CLI.
This adds local cluster artifact generation, including cluster definitions/locks, validator key material, deposit data, and node-specific setup, along with supporting validation and error handling for network, threshold, keymanager, and withdrawal address configuration.
Testing
To keep PR scope smaller, tests will come in a separate PR.