feat: add devnet runner and client CLI binaries#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds local devnet infrastructure for spawning multi-node networks and a client CLI for testing chunk operations. It introduces two new binaries (saorsa-devnet and saorsa-client) and a devnet library module with network management, health monitoring, and manifest-based peer discovery.
Changes:
- Added devnet library module with
Devnet,DevnetConfig, andDevnetManifesttypes for spawning and managing local multi-node networks - Added
saorsa-devnetCLI binary for network orchestration with configurable presets (minimal/small/default) - Added
saorsa-clientCLI binary for chunk put/get operations with bootstrap or manifest-based discovery
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/lib.rs | Exports devnet module and its public types |
| src/devnet.rs | Implements devnet infrastructure with node lifecycle, health monitoring, and stabilization logic |
| Cargo.toml | Adds new binary targets, moves rand/serde_json to dependencies, switches saorsa-core to path dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Payment cache capacity for devnet nodes. | ||
| const DEVNET_PAYMENT_CACHE_CAPACITY: usize = 1000; | ||
|
|
||
| /// Devnet rewards address (20 bytes, all 0x01). |
There was a problem hiding this comment.
This constant creates a devnet rewards address with all bytes set to 0x01, but there's no documentation explaining why this specific value was chosen or if it has special meaning in the devnet context. Consider adding a comment explaining the rationale for this address pattern.
| /// Devnet rewards address (20 bytes, all 0x01). | |
| /// Devnet rewards address (20 bytes, all 0x01). | |
| /// | |
| /// This is a deterministic, non-zero placeholder address used only in local | |
| /// devnets. The `0x01` pattern is chosen for simplicity and recognizability; | |
| /// it has no special meaning on-chain and must not be used on production | |
| /// networks. Using a fixed value keeps tests and devnet runs reproducible | |
| /// without requiring real key material or funding. |
| } | ||
|
|
||
| /// A single devnet node instance. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
The #[allow(dead_code)] attribute on the entire struct is overly broad. Several fields (e.g., index, port, address) appear to be used in the implementation. Remove this blanket suppression and only apply #[allow(dead_code)] to specific unused fields if necessary.
| #[allow(dead_code)] |
| let node_count_u16 = u16::try_from(config.node_count).map_err(|_| { | ||
| DevnetError::Config(format!("Node count {} exceeds u16::MAX", config.node_count)) | ||
| })?; |
There was a problem hiding this comment.
This validation is redundant since config.node_count is already validated against MAX_DEVNET_NODE_COUNT (1000) at line 325, which is well below u16::MAX (65535). The try_from conversion will never fail after that check passes.
| let node_count_u16 = u16::try_from(config.node_count).map_err(|_| { | |
| DevnetError::Config(format!("Node count {} exceeds u16::MAX", config.node_count)) | |
| })?; | |
| let node_count_u16 = config.node_count as u16; |
| let index_u16 = u16::try_from(index) | ||
| .map_err(|_| DevnetError::Config(format!("Node index {index} exceeds u16::MAX")))?; |
There was a problem hiding this comment.
This conversion check is unnecessary because the index comes from a loop over 0..config.node_count, and node_count is already validated to be at most MAX_DEVNET_NODE_COUNT (1000), which always fits in a u16.
| let index_u16 = u16::try_from(index) | |
| .map_err(|_| DevnetError::Config(format!("Node index {index} exceeds u16::MAX")))?; | |
| let index_u16 = index as u16; |
Greptile OverviewGreptile SummaryThis PR adds two new binaries on top of the existing
No merge-blocking correctness issues were identified in the changeset based on static review of the new devnet/client flows and their integration points (P2P event routing + AntProtocol request/response). Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| src/bin/saorsa-client/cli.rs | Adds clap-based CLI for saorsa-client with bootstrap/manifest discovery and put/get subcommands. |
| src/bin/saorsa-client/main.rs | Implements saorsa-client entrypoint: resolves bootstrap peers, creates ephemeral P2P node, and performs chunk put/get via QuantumClient. |
| src/bin/saorsa-devnet/cli.rs | Adds clap-based CLI for saorsa-devnet to configure node counts, ports, data dir, presets, manifest output, and logging. |
| src/bin/saorsa-devnet/main.rs | Implements saorsa-devnet entrypoint: builds DevnetConfig from CLI, starts devnet, emits manifest (file or stdout), waits for Ctrl+C, then shuts down. |
| src/devnet.rs | Introduces Devnet/DevnetConfig/DevnetManifest and local in-process multi-node runner with stabilization checks and health monitoring; no definite correctness issues found. |
| src/lib.rs | Exports new devnet module and re-exports Devnet types at crate root. |
Sequence Diagram
sequenceDiagram
participant User
participant DevnetBin as saorsa-devnet
participant Devnet as Devnet (src/devnet.rs)
participant Node as P2PNode (saorsa-core)
participant ClientBin as saorsa-client
participant QClient as QuantumClient
participant Proto as AntProtocol
User->>DevnetBin: run --preset/--nodes ... --manifest
DevnetBin->>Devnet: Devnet::new(config)
DevnetBin->>Devnet: devnet.start()
loop for each bootstrap node
Devnet->>Node: P2PNode::new(core_config)
Devnet->>Node: node.start()
Devnet->>Proto: create_ant_protocol(storage+payment+quote)
Devnet->>Devnet: spawn protocol_task (subscribe_events)
end
loop for each regular node
Devnet->>Node: P2PNode::new(core_config w/ bootstrap peers)
Devnet->>Node: node.start()
Devnet->>Devnet: spawn protocol_task (subscribe_events)
end
Devnet->>Devnet: wait_for_stabilization(peer_count checks)
DevnetBin-->>User: write DevnetManifest (file/stdout)
User->>ClientBin: run --devnet-manifest ... put/get
ClientBin->>ClientBin: resolve_bootstrap(manifest or --bootstrap)
ClientBin->>Node: create_client_node(listen 0.0.0.0:0, bootstrap peers)
ClientBin->>QClient: QuantumClient::new(...).with_node(node)
alt put
ClientBin->>QClient: put_chunk(bytes)
QClient->>Node: send chunk protocol request
Node->>Devnet: deliver P2PEvent::Message
Devnet->>Proto: handle_message(request)
Proto-->>Devnet: response bytes
Devnet->>Node: send_message(response)
QClient-->>ClientBin: address
else get
ClientBin->>QClient: get_chunk(address)
QClient->>Node: send chunk protocol request
Node->>Devnet: deliver P2PEvent::Message
Devnet->>Proto: handle_message(request)
Proto-->>Devnet: response bytes
Devnet->>Node: send_message(response)
QClient-->>ClientBin: chunk content
end
DevnetBin->>Devnet: ctrl_c() then devnet.shutdown()
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Cargo.toml
Outdated
| [dependencies] | ||
| # Core (provides EVERYTHING: networking, DHT, security, trust, storage) | ||
| saorsa-core = "0.10.2" | ||
| saorsa-core = { path = "../saorsa-core" } |
There was a problem hiding this comment.
path dependency should not be committed
| saorsa-core = { path = "../saorsa-core" } | |
| saorsa-core = "0.10.2" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Cargo.toml
Line: 35:35
Comment:
path dependency should not be committed
```suggestion
saorsa-core = "0.10.2"
```
How can I resolve this? If you propose a fix, please make it concise.
mickvandijke
force-pushed
the
feat/devnet-and-client-cli
branch
from
81c431f to
77b82f0
Compare
|
@greptile-apps @claude please review again |
|
Claude finished @mickvandijke's task —— View job Code Review Complete ✓I've completed a thorough review of this PR. Overall, this is excellent work with very high code quality. The implementation properly follows the project's strict "NO PANICS" policy and uses the correct port isolation ranges. ✅ Strengths1. Excellent Error Handling
2. Correct Port Isolation
3. Clean Architecture
4. Good Documentation
🔴 Critical Issue: Missing Cargo.toml ChangesThe Cargo.toml file is missing the binary definitions for the new binaries! The commit message and file contents show two new binaries were added:
But Cargo.toml doesn't include the
Required Fix: Add these sections to Cargo.toml after line 23: [[bin]]
name = "saorsa-devnet"
path = "src/bin/saorsa-devnet/main.rs"
[[bin]]
name = "saorsa-client"
path = "src/bin/saorsa-client/main.rs"Also, 💡 Minor Suggestions1. Redundant Type Conversion (src/devnet.rs:340-342)let node_count_u16 = u16::try_from(config.node_count).map_err(|_| {
DevnetError::Config(format!("Node count {} exceeds u16::MAX", config.node_count))
})?;This check is redundant because #[allow(clippy::cast_possible_truncation)] // Validated <= 1000 above
let node_count_u16 = config.node_count as u16;Same issue at line 495-496 - the 2. Overly Broad
|
Add saorsa-devnet for spawning local multi-node networks and saorsa-client for chunk put/get operations. Includes devnet library module with configurable presets (minimal, small, default), health monitoring, and manifest output. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
mickvandijke
force-pushed
the
feat/devnet-and-client-cli
branch
from
77b82f0 to
b670dd9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rand = "0.8" | ||
| serde_json = "1" |
There was a problem hiding this comment.
The dependencies rand and serde_json are now used in production code (devnet module) but lack specific version constraints. Consider using more precise version specifications (e.g., rand = \"0.8.5\", serde_json = \"1.0\") to ensure reproducible builds and avoid unexpected breaking changes from patch updates.
| rand = "0.8" | |
| serde_json = "1" | |
| rand = "0.8.5" | |
| serde_json = "1.0" |
| fn default() -> Self { | ||
| let mut rng = rand::thread_rng(); | ||
|
|
||
| #[allow(clippy::cast_possible_truncation)] // DEFAULT_NODE_COUNT is 25, always fits u16 |
There was a problem hiding this comment.
The comment explains why truncation is safe for DEFAULT_NODE_COUNT but doesn't account for user-provided node counts. The validation ensuring node_count <= MAX_DEVNET_NODE_COUNT happens later in the new() method. Consider clarifying that this default initialization is safe but runtime values are validated separately.
| #[allow(clippy::cast_possible_truncation)] // DEFAULT_NODE_COUNT is 25, always fits u16 | |
| #[allow(clippy::cast_possible_truncation)] | |
| // Casting DEFAULT_NODE_COUNT (usize) to u16 is safe here because it is a small | |
| // fixed constant (currently 25) used only for computing the default base port | |
| // range, and 25 is well within both u16::MAX and MAX_DEVNET_NODE_COUNT. | |
| // User-provided node counts are not cast here; they are validated separately | |
| // in Devnet::new to ensure node_count <= MAX_DEVNET_NODE_COUNT at runtime. |
src/devnet.rs
Outdated
|
|
||
| if config.base_port == 0 { | ||
| let mut rng = rand::thread_rng(); | ||
| // node_count is validated <= MAX_DEVNET_NODE_COUNT (1000) above, fits u16 |
There was a problem hiding this comment.
The clippy suppression lacks an inline comment explaining why the cast is safe. Although the validation above ensures node_count <= MAX_DEVNET_NODE_COUNT (1000), adding a brief explanation here would improve code clarity.
| // node_count is validated <= MAX_DEVNET_NODE_COUNT (1000) above, fits u16 | |
| // Safe cast: node_count is validated <= MAX_DEVNET_NODE_COUNT (1000) above, which is well below u16::MAX, so no truncation can occur |
Remove the arbitrary 1000-node cap and replace the cast with try_from for the auto-port allocation path. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@claude please do a final review |
|
Claude finished @mickvandijke's task —— View job Final Review Complete ✓I've completed a comprehensive final review of this PR. This is excellent, production-ready code that demonstrates high quality engineering and careful attention to the project's standards. ✅ Strengths1. Perfect Adherence to NO PANICS Policy
2. Correct Port Isolation Implementation
3. Excellent Binary Configuration
4. High-Quality Architecture
5. Comprehensive Configuration
6. Good Documentation
📝 Previous Review Comments - StatusLooking at previous review comments from other reviewers: ✅ Addressed
💡 Minor Suggestions from Previous ReviewsSome reviewers suggested optimizations that are not blocking but worth considering:
These are minor code quality suggestions that don't affect functionality or safety. 🔍 Security & Safety Analysis✅ No Security Issues Found
✅ Concurrency Safety
✅ Resource Management
🎯 Functionality AssessmentCore Features
Edge Cases Handled
📊 Code Quality Metrics
✅ Test Plan VerificationBased on the PR description test plan, I can verify:
Note: I cannot execute cargo commands directly, but the code structure is sound for all test plan items. 🎉 SummaryThis PR adds valuable local development infrastructure with excellent code quality: What's Great
What's Optional (Non-Blocking)
RecommendationAPPROVE - Ready to Merge ✅ This PR is production-ready and demonstrates excellent engineering practices. The optional suggestions above are minor code quality improvements that don't affect functionality or safety. Confidence Level: Very High (5/5) The code is well-written, safe, properly configured, and thoroughly handles edge cases. No blocking issues found. Job Run | Branch: |
Summary
saorsa-devnetbinary for spawning local multi-node networks with configurable presets (minimal/small/default), health monitoring, and JSON manifest outputsaorsa-clientbinary for chunk put/get operations against a running network (supports--bootstrapor--devnet-manifestfor peer discovery)src/devnet.rslibrary module withDevnet,DevnetConfig, andDevnetManifesttypesUsage
Start a local devnet
The devnet runs until you press Ctrl+C. The
--manifestflag writes a JSON file with bootstrap addresses that the client can use for peer discovery.Upload and retrieve a chunk
You can also connect directly to bootstrap peers instead of using a manifest:
Test plan
cargo build --releasecompiles all three binariescargo testpassescargo clippy --all-targets --all-features -- -D warningscleansaorsa-devnet --preset minimal --manifest /tmp/devnet.jsonspawns a local networksaorsa-client --devnet-manifest /tmp/devnet.json put --file <testfile>stores a chunk and prints its addresssaorsa-client --devnet-manifest /tmp/devnet.json get <address> --out <outfile>retrieves it🤖 Generated with Claude Code