fix(rpc): detect 401 auth errors and show actionable credential message#783
Conversation
When Dash Core RPC rejects with HTTP 401 (wrong password), the app previously showed generic "Disconnected" or "Could not communicate with Dash Core." Now it detects the 401 via downcast on the transport error and shows: "Dash Core rejected your credentials. Check your RPC username and password in settings." - Add `is_rpc_auth_error()` helper that downcasts through `jsonrpc::error::Error::Transport` to `simple_http::Error::HttpErrorCode(401)` - Add `TaskError::CoreRpcAuthFailed` variant with user-friendly message - Check auth before wallet-not-specified in `From<dashcore_rpc::Error>` - Refactor `get_best_chain_lock()` to return typed error instead of String, enabling auth detection in the `GetBestChainLocks` handler - Add 5 unit tests covering detection, display, and edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request improves RPC error handling by explicitly detecting HTTP 401 authentication failures and propagating them distinctly, while converting other configuration and connection errors to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 Generate unit tests (beta)
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 |
- Drop "username" from error message — only password is user-configurable - Add `with_char_limit()` builder to `PasswordInput` component - Set 40-char limit and 280px width on network settings RPC password field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the UX around Dash Core RPC authentication failures by detecting HTTP 401 responses and surfacing an actionable, credential-focused error instead of generic connectivity messaging.
Changes:
- Added a dedicated
TaskError::CoreRpcAuthFailedand detection helperis_rpc_auth_error()to classify RPC HTTP 401 errors. - Refactored Core chain-lock fetching to preserve typed
dashcore_rpc::Errorresults so auth failures can be surfaced on the active network. - Extended the
PasswordInputcomponent with optional character limits and applied it to the Core RPC password field in the network chooser UI.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ui/network_chooser_screen.rs | Adds width + character limit configuration to the Core RPC password input field. |
| src/ui/components/password_input.rs | Introduces char_limit support and applies it to the underlying egui TextEdit. |
| src/backend_task/error.rs | Adds CoreRpcAuthFailed, implements is_rpc_auth_error(), and tests 401 classification/display. |
| src/backend_task/core/mod.rs | Refactors chain-lock retrieval to keep typed errors and surfaces auth failures for the active network. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/ui/network_chooser_screen.rs (1)
124-127: Extract input policy literals into named constants.Line 126 and Line 127 use hard-coded UI policy values; constants make intent clearer and easier to update.
Suggested refactor
impl NetworkChooserScreen { + const LOCAL_RPC_PASSWORD_MAX_LEN: usize = 40; + const LOCAL_RPC_PASSWORD_INPUT_WIDTH: f32 = 280.0; + pub fn new( mainnet_app_context: &Arc<AppContext>, testnet_app_context: Option<&Arc<AppContext>>, devnet_app_context: Option<&Arc<AppContext>>, local_app_context: Option<&Arc<AppContext>>, @@ let mut dashmate_password_input = PasswordInput::new() .with_hint_text("Core RPC password") - .with_char_limit(40) - .with_desired_width(280.0); + .with_char_limit(Self::LOCAL_RPC_PASSWORD_MAX_LEN) + .with_desired_width(Self::LOCAL_RPC_PASSWORD_INPUT_WIDTH);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/network_chooser_screen.rs` around lines 124 - 127, The PasswordInput initialization in dashmate_password_input uses hard-coded UI policy values; extract those literals into named constants (e.g., DASHMATE_PASSWORD_CHAR_LIMIT and DASHMATE_PASSWORD_DESIRED_WIDTH) and replace the numeric literals in the PasswordInput builder chain (.with_char_limit(40) and .with_desired_width(280.0)) with those constants; declare the constants near the top of the module (or next to related UI constants) so intent and maintainability are clearer while keeping the variable name dashmate_password_input and the PasswordInput::new() builder usage unchanged.src/ui/components/password_input.rs (1)
84-87: Consider guardingwith_char_limit(0)to avoid unusable input.Line 84 currently accepts
0, which can effectively block input. A small guard keeps behavior safer.Suggested tweak
pub fn with_char_limit(mut self, limit: usize) -> Self { - self.char_limit = Some(limit); + self.char_limit = if limit == 0 { None } else { Some(limit) }; self }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/password_input.rs` around lines 84 - 87, The with_char_limit method currently allows calling with 0 which makes the input unusable; update the function (pub fn with_char_limit) to guard against limit == 0 by not setting self.char_limit (leave it as None) or otherwise ignoring the call when limit is zero, and only set self.char_limit = Some(limit) for limit >= 1 so PasswordInput retains usable behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend_task/error.rs`:
- Around line 59-61: The CoreRpcAuthFailed TaskError variant currently drops the
underlying dashcore_rpc::Error; update the enum variant to include a typed
source field (e.g., CoreRpcAuthFailed { #[source] source: dashcore_rpc::Error })
and update any conversion/From implementations that map dashcore_rpc::Error ->
TaskError (or the caller that constructs CoreRpcAuthFailed) to pass the original
error through into that source field so the error chain is preserved when
Debug/Display are printed; apply the same change for the other TaskError
variants noted (lines ~733-735) that wrap dashcore_rpc::Error.
---
Nitpick comments:
In `@src/ui/components/password_input.rs`:
- Around line 84-87: The with_char_limit method currently allows calling with 0
which makes the input unusable; update the function (pub fn with_char_limit) to
guard against limit == 0 by not setting self.char_limit (leave it as None) or
otherwise ignoring the call when limit is zero, and only set self.char_limit =
Some(limit) for limit >= 1 so PasswordInput retains usable behavior.
In `@src/ui/network_chooser_screen.rs`:
- Around line 124-127: The PasswordInput initialization in
dashmate_password_input uses hard-coded UI policy values; extract those literals
into named constants (e.g., DASHMATE_PASSWORD_CHAR_LIMIT and
DASHMATE_PASSWORD_DESIRED_WIDTH) and replace the numeric literals in the
PasswordInput builder chain (.with_char_limit(40) and
.with_desired_width(280.0)) with those constants; declare the constants near the
top of the module (or next to related UI constants) so intent and
maintainability are clearer while keeping the variable name
dashmate_password_input and the PasswordInput::new() builder usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc7dadfa-c96e-4c36-a139-624753925242
📒 Files selected for processing (4)
src/backend_task/core/mod.rssrc/backend_task/error.rssrc/ui/components/password_input.rssrc/ui/network_chooser_screen.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR correctly detects 401 auth errors and surfaces them as actionable user-facing messages. However, early-returning an error from GetBestChainLocks bypasses the existing success path that drives the connection status indicator offline, causing a stale 'connected' indicator after credentials fail. The error message text and password field limit also need minor adjustments.
Reviewed commit: 9e5cf7a
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/core/mod.rs`:
- [BLOCKING] lines 207-211: Auth error early-return leaves connection status indicator stale
When the active network returns HTTP 401, `GetBestChainLocks` now early-returns `Err(TaskError::CoreRpcAuthFailed)`. This becomes `TaskResult::Error`, but `ConnectionStatus::handle_task_result()` (line 409–436) only processes `TaskResult::Success` — it never calls `set_rpc_online(false)` for errors. Before this PR, auth failures were flattened to `None` via `.ok().flatten()`, producing `ChainLocks(None, ...)` which correctly drove `update_from_chainlocks()` → `set_rpc_online(false)`.
Result: after credentials break, the user sees the error banner but the connection status indicator stays 'connected'.
One fix: instead of early-returning, stash the auth error, continue to produce the `ChainLocks(...)` success result (with `None` for the failed network so `rpc_online` updates correctly), and surface the auth error through a secondary mechanism (e.g., a new result variant that carries both the chain locks and the auth warning). Alternatively, teach `handle_task_result()` to set `rpc_online = false` when it sees `TaskResult::Error` originating from a chain-lock task.
In `src/backend_task/error.rs`:
- [SUGGESTION] line 60: Error message mentions only password but a 401 can also mean wrong username
The `CoreRpcAuthFailed` display says "Check your RPC password in settings" but the fallback auth path uses `Auth::UserPass(core_rpc_user, core_rpc_password)`, so a wrong username produces the same HTTP 401. The message should mention both.
In `src/ui/network_chooser_screen.rs`:
- [SUGGESTION] line 126: 40-character password limit may truncate valid RPC passwords
`.with_char_limit(40)` silently prevents entry of longer passwords. Dash Core's `rpcpassword` accepts arbitrary-length strings, and dashmate or password generators can produce 64+ character values. The field already has `with_desired_width(280.0)` for visual sizing, so the character limit can be raised safely.
| if let Err(e) = active_result | ||
| && is_rpc_auth_error(e) | ||
| { | ||
| return Err(TaskError::CoreRpcAuthFailed); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Auth error early-return leaves connection status indicator stale
When the active network returns HTTP 401, GetBestChainLocks now early-returns Err(TaskError::CoreRpcAuthFailed). This becomes TaskResult::Error, but ConnectionStatus::handle_task_result() (line 409–436) only processes TaskResult::Success — it never calls set_rpc_online(false) for errors. Before this PR, auth failures were flattened to None via .ok().flatten(), producing ChainLocks(None, ...) which correctly drove update_from_chainlocks() → set_rpc_online(false).
Result: after credentials break, the user sees the error banner but the connection status indicator stays 'connected'.
One fix: instead of early-returning, stash the auth error, continue to produce the ChainLocks(...) success result (with None for the failed network so rpc_online updates correctly), and surface the auth error through a secondary mechanism (e.g., a new result variant that carries both the chain locks and the auth warning). Alternatively, teach handle_task_result() to set rpc_online = false when it sees TaskResult::Error originating from a chain-lock task.
source: ['codex-general']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/core/mod.rs`:
- [BLOCKING] lines 207-211: Auth error early-return leaves connection status indicator stale
When the active network returns HTTP 401, `GetBestChainLocks` now early-returns `Err(TaskError::CoreRpcAuthFailed)`. This becomes `TaskResult::Error`, but `ConnectionStatus::handle_task_result()` (line 409–436) only processes `TaskResult::Success` — it never calls `set_rpc_online(false)` for errors. Before this PR, auth failures were flattened to `None` via `.ok().flatten()`, producing `ChainLocks(None, ...)` which correctly drove `update_from_chainlocks()` → `set_rpc_online(false)`.
Result: after credentials break, the user sees the error banner but the connection status indicator stays 'connected'.
One fix: instead of early-returning, stash the auth error, continue to produce the `ChainLocks(...)` success result (with `None` for the failed network so `rpc_online` updates correctly), and surface the auth error through a secondary mechanism (e.g., a new result variant that carries both the chain locks and the auth warning). Alternatively, teach `handle_task_result()` to set `rpc_online = false` when it sees `TaskResult::Error` originating from a chain-lock task.
| CoreWalletNotConfigured, | ||
|
|
||
| /// Dash Core RPC rejected the request due to invalid credentials (HTTP 401). | ||
| #[error("Dash Core rejected your credentials. Check your RPC password in settings.")] |
There was a problem hiding this comment.
🟡 Suggestion: Error message mentions only password but a 401 can also mean wrong username
The CoreRpcAuthFailed display says "Check your RPC password in settings" but the fallback auth path uses Auth::UserPass(core_rpc_user, core_rpc_password), so a wrong username produces the same HTTP 401. The message should mention both.
💡 Suggested change
| #[error("Dash Core rejected your credentials. Check your RPC password in settings.")] | |
| #[error("Dash Core rejected your credentials. Check your RPC username and password in settings.")] |
source: ['codex-general', 'claude-general']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/error.rs`:
- [SUGGESTION] line 60: Error message mentions only password but a 401 can also mean wrong username
The `CoreRpcAuthFailed` display says "Check your RPC password in settings" but the fallback auth path uses `Auth::UserPass(core_rpc_user, core_rpc_password)`, so a wrong username produces the same HTTP 401. The message should mention both.
| let mut dashmate_password_input = PasswordInput::new().with_hint_text("Core RPC password"); | ||
| let mut dashmate_password_input = PasswordInput::new() | ||
| .with_hint_text("Core RPC password") | ||
| .with_char_limit(40) |
There was a problem hiding this comment.
🟡 Suggestion: 40-character password limit may truncate valid RPC passwords
.with_char_limit(40) silently prevents entry of longer passwords. Dash Core's rpcpassword accepts arbitrary-length strings, and dashmate or password generators can produce 64+ character values. The field already has with_desired_width(280.0) for visual sizing, so the character limit can be raised safely.
💡 Suggested change
| .with_char_limit(40) | |
| .with_char_limit(128) |
source: ['claude-rust-quality', 'claude-general']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/network_chooser_screen.rs`:
- [SUGGESTION] line 126: 40-character password limit may truncate valid RPC passwords
`.with_char_limit(40)` silently prevents entry of longer passwords. Dash Core's `rpcpassword` accepts arbitrary-length strings, and dashmate or password generators can produce 64+ character values. The field already has `with_desired_width(280.0)` for visual sizing, so the character limit can be raised safely.
| Network::Testnet => &testnet_result, | ||
| Network::Devnet => &devnet_result, | ||
| Network::Regtest => &local_result, | ||
| _ => &mainnet_result, |
There was a problem hiding this comment.
💬 Nitpick: Wildcard arm silently maps unknown Network variants to mainnet
Network is #[non_exhaustive], so the _ => &mainnet_result fallback is required. But if a new variant is added, this silently treats its auth check as mainnet's. A tracing::warn! in the wildcard arm would surface the gap.
source: ['claude-rust-quality']
Summary
is_rpc_auth_error()helper that downcasts through the boxed transport error chain to identifyHttpErrorCode(401)TaskError::CoreRpcAuthFailedvariant with user-friendly messageget_best_chain_lock()to preserve typed errors (Result<Option<ChainLock>, dashcore_rpc::Error>) instead of stringifying them, so auth errors on the active network surface through the task error systemUser story
Imagine you are configuring Dash Evo Tool to connect to your local Dash Core node. You accidentally enter the wrong RPC password. Previously, the app would just say "Disconnected — check that Dash Core is running" — leaving you chasing a non-problem. Now the app tells you directly that your credentials were rejected and to check your username and password in settings.
Test plan
rpc_http_401_converts_to_core_rpc_auth_failed— verifiesFromconversionrpc_http_401_display_mentions_credentials— verifies user-facing textis_rpc_auth_error_detects_401— unit test for helperis_rpc_auth_error_ignores_other_http_codes— 403 does not false-positiveis_rpc_auth_error_ignores_rpc_errors— JSON-RPC errors do not false-positive🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
New Features