start maker on creation#87
Conversation
📝 WalkthroughWalkthroughAuto-starts maker coinswap servers on startup recovery and immediately after creation (non-blocking on failures), adds a localhost standard-ports fallback RPC probe for bitcoind status, and updates tests to assume makers auto-start on creation. ChangesMaker Auto-Start Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/maker_manager/mod.rs (1)
110-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the outdated comment.
The comment states the function "does NOT start servers", but lines 148-159 now auto-start all successfully restored makers during dashboard startup.
📝 Proposed fix
- /// Creates a new MakerManager with persistence at the given config directory. - /// Loads any previously saved maker configs and re-initializes them (but does NOT start servers). + /// Creates a new MakerManager with persistence at the given config directory. + /// Loads any previously saved maker configs, re-initializes them, and attempts to auto-start them. pub fn new(config_dir: PathBuf) -> Result<Self> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/maker_manager/mod.rs` at line 110, The doc comment that starts "Loads any previously saved maker configs and re-initializes them (but does NOT start servers)" is now inaccurate; update that comment in the maker_manager module to reflect that restored makers are auto-started during dashboard startup (remove the "does NOT start servers" clause and describe that successfully restored makers will be started automatically). Locate the doc comment above the loader/restore function in mod.rs (the comment beginning with "Loads any previously saved maker configs and re-initializes them") and replace it with a concise, accurate description that mentions auto-start behavior for restored makers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/maker_manager/mod.rs`:
- Line 110: The doc comment that starts "Loads any previously saved maker
configs and re-initializes them (but does NOT start servers)" is now inaccurate;
update that comment in the maker_manager module to reflect that restored makers
are auto-started during dashboard startup (remove the "does NOT start servers"
clause and describe that successfully restored makers will be started
automatically). Locate the doc comment above the loader/restore function in
mod.rs (the comment beginning with "Loads any previously saved maker configs and
re-initializes them") and replace it with a concise, accurate description that
mentions auto-start behavior for restored makers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c0b2521-29b8-47d2-b7a5-ba09e74d3485
📒 Files selected for processing (2)
src/api/makers.rssrc/maker_manager/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/bitcoind.rs`:
- Line 56: The loop is cloning the entire auth_methods collection on each
iteration causing unnecessary allocations; change the for auth in
auth_methods.clone() loop to iterate by reference (for auth in &auth_methods)
and only clone the individual auth when needed (e.g., pass auth.clone() into
Client::new), ensuring other uses use &auth to avoid extra copies; update any
uses inside the loop that expect owned String accordingly to call .clone() at
the point of Client::new.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17f207fd-8840-4654-99cd-6e52bf904f71
📒 Files selected for processing (2)
src/api/bitcoind.rstests/integration_test.rs
| continue; | ||
| } | ||
| let url = format!("http://{}", address); | ||
| for auth in auth_methods.clone() { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Avoid unnecessary array cloning in the loop.
The current code clones the entire auth_methods array (including owned String values from line 44) on every iteration of the outer loop. Iterate by reference instead and clone only when passing to Client::new.
♻️ Proposed fix
- for auth in auth_methods.clone() {
- if let Ok(client) = Client::new(&url, auth) {
+ for auth in &auth_methods {
+ if let Ok(client) = Client::new(&url, auth.clone()) {
if let Ok(info) = client.get_blockchain_info() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for auth in auth_methods.clone() { | |
| for auth in &auth_methods { | |
| if let Ok(client) = Client::new(&url, auth.clone()) { | |
| if let Ok(info) = client.get_blockchain_info() { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/bitcoind.rs` at line 56, The loop is cloning the entire auth_methods
collection on each iteration causing unnecessary allocations; change the for
auth in auth_methods.clone() loop to iterate by reference (for auth in
&auth_methods) and only clone the individual auth when needed (e.g., pass
auth.clone() into Client::new), ensuring other uses use &auth to avoid extra
copies; update any uses inside the loop that expect owned String accordingly to
call .clone() at the point of Client::new.
2 participants
Summary by CodeRabbit
New Features
Tests