feat(cli): improve init flow with better UX#942
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates deps (inquire → 0.9.4; adds vergen-gitcl build-dep), adds a build script emitting Git SHA, prints an init banner using VERGEN_GIT_SHA, refactors CLI init to unify API selection and remote-peer presets, and removes multiple example/config files. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bin/dolos/init.rs (1)
350-381:⚠️ Potential issue | 🟠 MajorDon't enable every listener by default.
These defaults turn "accept the defaults" into "bind several APIs on all interfaces" because the corresponding
apply_serve_*helpers use wildcard listen addresses. That's a meaningful security posture change for an init flow. Default them to off, or at least to loopback, and let the multi-select opt users in explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/dolos/init.rs` around lines 350 - 381, The init code currently enables multiple listeners by default via apply_serve_grpc(Some(true)), apply_serve_minibf(Some(true)), apply_serve_minikupo(Some(true)), apply_serve_trp(Some(true)) and (on unix) apply_serve_ouroboros(Some(true)), which causes wildcard binds; change these defaults to not enable listeners by default—either remove those Some(true) calls or pass Some(false) for each of apply_serve_grpc, apply_serve_minibf, apply_serve_minikupo, apply_serve_trp and apply_serve_ouroboros (or adjust the helpers to default to loopback instead of wildcard) so the init flow does not bind public interfaces unless the user explicitly opt-ins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.rs`:
- Around line 8-9: The current code passes PathBuf::from(".git") directly to
emit_git_rerun_hints which breaks for worktrees where .git is a file; update the
logic that builds git_dir (and the call sites around emit_git_rerun_hints) to
detect if .git is a file, read its contents (e.g. "gitdir: /actual/path"),
resolve that target to the real git directory, and then call
emit_git_rerun_hints with the resolved path so the function emits rerun hints
against the actual HEAD/ref files (adjust the same pattern used in the block
around lines 31-46 where HEAD/ref paths are emitted).
In `@src/bin/dolos/init.rs`:
- Around line 243-280: The API picker and defaults expose Minibf/Minikupo/TRP
even when those features are not compiled; update AvailableApi, its Display
impl, and the VARIANTS const as well as ConfigEditor::default() to use the same
feature gates as the actual service modules (e.g. add #[cfg(feature = "minibf")]
/ #[cfg(feature = "minikupo")] / #[cfg(feature = "trp")] around the enum
variants, their Display arms, and entries in VARIANTS, and wrap any
default-enabled selections in ConfigEditor::default() with the same
#[cfg(feature = "...")] so the binary only advertises and persists services that
are actually compiled in.
- Around line 573-617: In prompt_remote_peer, preserve the existing custom relay
by pre-filling the Text prompt when matching RemotePeerChoice::Other: use the
local variable current as the default for the Text prompt (the same way the
unknown-network branch does) so editing an existing custom peer doesn't force
retyping; update the RemotePeerChoice::Other branch inside the match in function
prompt_remote_peer to provide .with_default(current) (or equivalent) to the
Text::new prompt before calling .prompt().
---
Outside diff comments:
In `@src/bin/dolos/init.rs`:
- Around line 350-381: The init code currently enables multiple listeners by
default via apply_serve_grpc(Some(true)), apply_serve_minibf(Some(true)),
apply_serve_minikupo(Some(true)), apply_serve_trp(Some(true)) and (on unix)
apply_serve_ouroboros(Some(true)), which causes wildcard binds; change these
defaults to not enable listeners by default—either remove those Some(true) calls
or pass Some(false) for each of apply_serve_grpc, apply_serve_minibf,
apply_serve_minikupo, apply_serve_trp and apply_serve_ouroboros (or adjust the
helpers to default to loopback instead of wildcard) so the init flow does not
bind public interfaces unless the user explicitly opt-ins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5d2a809-e32d-49e2-a2bf-37983075ae8d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlbuild.rscrates/fjall/src/index/mod.rsexamples/v1/alonzo.jsonexamples/v1/byron.jsonexamples/v1/conway.jsonexamples/v1/dolos.tomlexamples/v1/shelley.jsonsrc/bin/dolos/banner.rssrc/bin/dolos/init.rssrc/bin/dolos/main.rs
💤 Files with no reviewable changes (5)
- examples/v1/alonzo.json
- examples/v1/dolos.toml
- examples/v1/shelley.json
- examples/v1/conway.json
- examples/v1/byron.json
Comment on lines
+243
to
+280
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| enum AvailableApi { | ||
| UtxoRpc, | ||
| Minibf, | ||
| Minikupo, | ||
| Trp, | ||
| #[cfg(unix)] | ||
| Ouroboros, | ||
| } | ||
|
|
||
| impl Display for AvailableApi { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Self::UtxoRpc => f.write_str("UTxO RPC (gRPC): Performant API for UTxO blockchains"), | ||
| Self::Minibf => f.write_str("Mini-Blockfrost (HTTP): Blockfrost-compatible API"), | ||
| Self::Minikupo => f.write_str("Mini-Kupo (HTTP): Kupo-compatible API"), | ||
| Self::Trp => f.write_str("TRP (JSON-RPC): Tx3 transaction resolver protocol"), | ||
| #[cfg(unix)] | ||
| Self::Ouroboros => { | ||
| f.write_str("Ouroboros (unix socket): node-to-client compatible API") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl AvailableApi { | ||
| #[cfg(unix)] | ||
| const VARIANTS: &'static [Self] = &[ | ||
| Self::UtxoRpc, | ||
| Self::Minibf, | ||
| Self::Minikupo, | ||
| Self::Trp, | ||
| Self::Ouroboros, | ||
| ]; | ||
|
|
||
| #[cfg(windows)] | ||
| const VARIANTS: &'static [Self] = &[Self::UtxoRpc, Self::Minibf, Self::Minikupo, Self::Trp]; | ||
| } |
There was a problem hiding this comment.
Mirror Cargo feature gates in the API picker and defaults.
AvailableApi always includes Minibf/Minikupo/TRP, and ConfigEditor::default() pre-enables them, even though those services are optional Cargo features. A --no-default-features --features utils build will still advertise and persist configs for services the binary doesn't contain. Gate these variants and their default selections behind the same #[cfg(feature = "...")] conditions used by the actual service modules.
Also applies to: 350-381, 639-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bin/dolos/init.rs` around lines 243 - 280, The API picker and defaults
expose Minibf/Minikupo/TRP even when those features are not compiled; update
AvailableApi, its Display impl, and the VARIANTS const as well as
ConfigEditor::default() to use the same feature gates as the actual service
modules (e.g. add #[cfg(feature = "minibf")] / #[cfg(feature = "minikupo")] /
#[cfg(feature = "trp")] around the enum variants, their Display arms, and
entries in VARIANTS, and wrap any default-enabled selections in
ConfigEditor::default() with the same #[cfg(feature = "...")] so the binary only
advertises and persists services that are actually compiled in.
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
1 participant
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.
Summary by CodeRabbit
New Features
Dependencies
Chores