feat: add create cluster tests

[varex83]

Summary

Ports the Go TestCreateCluster test suite to Rust, covering all major create_cluster flows end-to-end, and fixes several correctness issues uncovered in the process.

New tests (crates/cli/src/commands/create_cluster.rs)

A shared run_test_create_cluster helper drives all test cases, mirroring the Go test loop. Each case exercises run() against a temp directory and asserts:

• the command succeeds (or returns a specific error)
• the generated cluster-lock.json passes verify_hashes() and verify_signatures()
• all distributed validators have unique public keys
• deposit-data amounts match expectations
• CLI output and produced file tree match golden snapshots

Test cases covered:

Test	Notes
simnet	4-node simnet cluster
goerli	Goerli network, 3-node
custom_testnet_flags	Custom chain ID / fork version / genesis timestamp
custom_target_gas_limit	Non-default target_gas_limit
solo_flow_definition_from_disk	Definition loaded from a local JSON file
solo_flow_definition_from_network	Definition served over HTTP (wiremock)
splitkeys	--split-existing-keys, keys generated in a temp dir
splitkeys_with_cluster_definition	Split keys combined with a pre-existing definition
four_partial_deposits / two_partial_deposits	Multiple deposit amounts per validator
with_fee_recipient_and_withdrawal_addresses	Custom fee-recipient + withdrawal addresses
validate_def	Invalid definition triggers expected error

Golden files live in crates/cli/src/commands/testdata/ and can be regenerated with UPDATE_GOLDEN=1 cargo test.

Bug fixes

• create_cluster::run — call def.set_definition_hashes() before proceeding with key generation; the hashes were previously unset, causing verify_hashes() to fail.
• definition.rs deserialization — validate that num_validators == validator_addresses.len() for schema versions v1.5–v1.10; add #[serde(default)] to fee_recipient_address, withdrawal_address, and compounding so older JSON examples without those fields still deserialize correctly.
• lock.rs no_registration check — remove the zero-fee_recipient guard. The Go reference uses len == 0 (missing field), not the zero Ethereum address; a legitimately zero fee-recipient is valid. Only an all-zero BLS signature or pubkey indicates a missing registration.
• cluster-definition-005.json — fix malformed timestamp (stray spaces around : separators).

Test plan

• cargo test -p pluto-cli passes with all new test cases green
• UPDATE_GOLDEN=1 cargo test regenerates golden files without diff
• Existing tests unaffected

[mskrzypkows]

One comment left, LGTM in general. Will you ask claude to review it?

[emlautarom1]

The golden test matching logic is no particularly good, specially since it does not show where the mismatch is located in case there is one. We can revisit this latter for improvements but I don't intend to block this.

[emlautarom1]

@claude

[claude]

Claude finished @emlautarom1's task in 5m 12s —— View job

---

Code Review

• Read main changed files
• Review create_cluster.rs test code
• Review definition.rs changes
• Review lock.rs changes
• Check golden test files
• Post comprehensive review

---

Summary

This is a solid port of the Go TestCreateCluster suite. The structure is clear, the helper decomposition works well, and the bug fixes in definition.rs and lock.rs are correct. A few issues need attention before merge.

---

Bug: cluster-definition-005.json timestamp regression

The PR introduced a malformed timestamp, not fixed one. The git diff for commit 7d13197 shows:

- "timestamp": "2023-05-18T15:12:46+02:00",   ← correct
+ "timestamp": "2023-05-18T15: 12: 46+02: 00", ← spaces inserted by this PR

The PR description says "fix malformed timestamp" but the direction is reversed — the timestamp was correct before this PR. This should be reverted to "2023-05-18T15:12:46+02:00".

Fix this →

---

Tests require a live Ethereum node (http://127.0.0.1:8545)

Multiple tests fail in CI unless an eth1 node is running locally:

• test_eth1_client() at line 1524 connects to http://127.0.0.1:8545
• validate_cluster_lock() calls test_eth1_client().await (line 1616)
• build_create_cluster_args() sets execution_engine_addr = Some("http://127.0.0.1:8545") whenever a definition_file is present (line 1581–1583)
• run() at line 393–397 creates a real eth1 client from execution_engine_addr

Since verify_signatures only validates BLS signatures and the eth1 client is passed but unused for those validations (based on lock.rs), passing an empty string (triggering a Noop client) or restructuring to decouple eth1 from signature verification would make these tests hermetic. [This was also flagged by @emlautarom1.]

---

Non-deterministic genesis_timestamp in test config

build_create_cluster_args at lines 1575–1577:

genesis_timestamp: config.testnet_chain_id.map(|_| {
    u64::try_from(chrono::Utc::now().timestamp()).expect("timestamp fits u64")
}),

This sets genesis timestamp to "now" whenever a testnet_chain_id is present. This is non-deterministic (differs per run) and semantically wrong — genesis timestamp should be a fixed historical time, not the current clock. This can also make the golden file comparison for custom_testnet_flags brittle if the output includes the timestamp. [Flagged by @emlautarom1.]

---

Fragile substring-based error matching

Throughout the tests, errors are matched via err_str.contains(expected). This pattern:

• Passes if a future error message grows to include the expected substring for a different reason
• Gives misleading failures if messages are refactored
• Cannot distinguish between two errors that both contain the same substring

Using structured matching on CliError variants as suggested by @emlautarom1 is the right fix. Where the error is CliError::CreateClusterError(CreateClusterError::SomeVariant), pattern matching gives compile-time safety.

---

load_ref_def manual JSON patching is redundant after #[serde(default)] fix

load_ref_def() (line 1535–1542) and validate_definition test (line 2175–2181) both manually patch compounding: false into the JSON value before deserializing:

if value.get("compounding").is_none() {
    value["compounding"] = serde_json::json!(false);
}

The PR adds #[serde(default)] to DefinitionV1x10::compounding (line 1517), which makes this manual patching unnecessary. Both call sites should be cleaned up.

---

require_golden_bytes: byte comparison preferred over lossy UTF-8

At lines 1470–1475:

assert_eq!(
    String::from_utf8_lossy(actual),
    String::from_utf8_lossy(&expected),
    ...
);

from_utf8_lossy silently replaces invalid UTF-8 with \u{FFFD}, which can mask encoding bugs. As suggested by @emlautarom1, comparing bytes directly (assert_eq!(actual, &expected)) is more precise.

---

Test ordering differs from Charon

As acknowledged by @varex83 in reply to @mskrzypkows, the test case order doesn't match Charon's (where the "valid lock" / basic passing case comes last). The last refactor commit (4cf6184) split the helper but didn't reorder the cases. This is a readability/parity issue but doesn't affect correctness.

---

Minor: map_err at line 499 is correct and necessary

The reviewer comment "No need for explicit map_err" at line 499 appears to be a mistake. Since run() returns CliResult<()> (i.e. Result<(), CliError>), and Rust's ? only performs one From conversion, the chain DefinitionError → CliError requires an explicit step via CreateClusterError::DefinitionError. The current .map_err(CreateClusterError::DefinitionError)? is correct.

---

simnet test name uses goerli network

The test labeled "simnet" (line 1840) uses network: Some("goerli"). This is a naming mismatch — if the intent is to mirror Go's simnet (a local simulated network), it should either use a simnet-specific network value or the test should be renamed to reflect that it's a goerli cluster test.

---

Overall Assessment

The core logic — the run_test_create_cluster helper, validate_cluster_lock, check_golden_files, the bug fixes in definition.rs and lock.rs — is well-structured and a good faithful port of the Go test suite. The main blockers are the timestamp regression in cluster-definition-005.json and the non-hermetic eth1 client dependency. The error matching and non-deterministic timestamp should also be addressed before merge.