Skip to content

fix(ui): improve auth key error message and suppress on startup auto-select #761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thepastaclaw wants to merge 2 commits into
base: v1.0-dev
Choose a base branch
from
Open

fix(ui): improve auth key error message and suppress on startup auto-select #761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2b2708
fix(ui): improve auth key error message and suppress on startup auto-…
thepastaclaw Mar 15, 2026
a6bd8f3
fix: remove quotes from identity label and use empty string for unknown
thepastaclaw Mar 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions src/ui/dashpay/contact_requests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ use crate::ui::components::wallet_unlock_popup::{
WalletUnlockPopup, WalletUnlockResult, try_open_wallet_no_password, wallet_needs_unlock,
};
use crate::ui::components::{MessageBanner, ResultBannerExt};
use crate::ui::identities::get_selected_wallet;
use crate::ui::identities::keys::add_key_screen::AddKeyScreen;
use crate::ui::identities::{get_selected_wallet, is_missing_document_signing_key_error};
use crate::ui::theme::DashColors;
use crate::ui::{MessageType, Screen, ScreenLike, ScreenType};
use dash_sdk::dpp::document::DocumentV0Getters;
Expand Down Expand Up @@ -103,11 +103,19 @@ impl ContactRequests {
.id()
.to_string(dash_sdk::dpp::platform_value::string_encoding::Encoding::Base58);

// Get wallet for the selected identity
// Get wallet for the selected identity. Suppress the expected
// "missing document-signing key" error on auto-select (some
// identities, e.g. evonodes, legitimately lack one); surface
// anything else so we don't silently hide real failures.
new_self.selected_wallet =
get_selected_wallet(&identities[0], Some(&app_context), None)
.or_show_error(app_context.egui_ctx())
.unwrap_or(None);
match get_selected_wallet(&identities[0], Some(&app_context), None) {
Ok(wallet) => wallet,
Err(e) if is_missing_document_signing_key_error(&e) => None,
Err(e) => {
MessageBanner::set_global(app_context.egui_ctx(), &e, MessageType::Error);
None
}
};

// Load requests from database for this identity
new_self.load_requests_from_database();
Expand Down
18 changes: 13 additions & 5 deletions src/ui/dashpay/qr_code_generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::ui::components::wallet_unlock_popup::{
use crate::ui::components::{MessageBanner, ResultBannerExt};
use crate::ui::dashpay::dashpay_screen::DashPaySubscreen;
use crate::ui::identities::funding_common::generate_qr_code_image;
use crate::ui::identities::get_selected_wallet;
use crate::ui::identities::{get_selected_wallet, is_missing_document_signing_key_error};
use crate::ui::theme::DashColors;
use crate::ui::{MessageType, RootScreenType, ScreenLike};
use eframe::epaint::TextureHandle;
Expand Down Expand Up @@ -77,11 +77,19 @@ impl QRCodeGeneratorScreen {
new_self.selected_identity_string =
identities[0].identity.id().to_string(Encoding::Base58);

// Get wallet for the selected identity
// Get wallet for the selected identity. Suppress the expected
// "missing document-signing key" error on auto-select (some
// identities, e.g. evonodes, legitimately lack one); surface
// anything else so we don't silently hide real failures.
new_self.selected_wallet =
get_selected_wallet(&identities[0], Some(&app_context), None)
.or_show_error(app_context.egui_ctx())
.unwrap_or(None);
match get_selected_wallet(&identities[0], Some(&app_context), None) {
Ok(wallet) => wallet,
Err(e) if is_missing_document_signing_key_error(&e) => None,
Err(e) => {
MessageBanner::set_global(app_context.egui_ctx(), &e, MessageType::Error);
None
}
};
}

new_self
Expand Down
34 changes: 32 additions & 2 deletions src/ui/identities/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ pub mod top_up_identity_screen;
pub mod transfer_screen;
pub mod withdraw_screen;

/// Substring used to identify the "missing document-signing key" error returned
/// by [`get_selected_wallet`] when an identity has no key suitable for signing
/// document state transitions. Callsites that auto-select an identity (e.g.
/// DashPay startup) use this to suppress the expected case while still
/// surfacing unrelated errors.
const MISSING_DOCUMENT_SIGNING_KEY_MARKER: &str =
"doesn't have an authentication key for signing document transitions";
Comment on lines +36 to +37

@thepastaclaw thepastaclaw May 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Error message uses jargon and lacks an actionable next step

Per the Dash Evo Tool review skill user-facing message rules: user-facing messages must avoid jargon and must include a concrete self-service action ("what happened + what to do"). The marker text — "doesn't have an authentication key for signing document transitions" — is the original pre-PR wording and fails on both counts: it uses platform terminology aimed at developers ("authentication key", "document transitions") and tells the user nothing about what to do (e.g., "Add a signing key in this identity's Keys screen"). Since this PR already reformats the message, it's a reasonable touchpoint to bring the wording into compliance — though doing so requires updating the marker constant, which underlines the brittleness of the substring-matching design.

source: ['claude']

🤖 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/identities/mod.rs`:
- [SUGGESTION] lines 36-37: Error message uses jargon and lacks an actionable next step
  Per the Dash Evo Tool review skill user-facing message rules: user-facing messages must avoid jargon and must include a concrete self-service action ("what happened + what to do"). The marker text — `"doesn't have an authentication key for signing document transitions"` — is the original pre-PR wording and fails on both counts: it uses platform terminology aimed at developers ("authentication key", "document transitions") and tells the user nothing about what to do (e.g., "Add a signing key in this identity's Keys screen"). Since this PR already reformats the message, it's a reasonable touchpoint to bring the wording into compliance — though doing so requires updating the marker constant, which underlines the brittleness of the substring-matching design.


/// Returns `true` if the given error string is the expected
/// "identity has no document-signing key" error from [`get_selected_wallet`].
pub fn is_missing_document_signing_key_error(error: &str) -> bool {
error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER)
}
Comment on lines +36 to +43

@coderabbitai coderabbitai Bot May 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Replace string-marker detection with a typed error variant.

is_missing_document_signing_key_error detects the error by checking error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER). This directly violates the coding guideline: "Never parse error strings to extract information — always use the typed error chain via downcast, match on variants, or access structured fields; if no typed variant exists, define a new TaskError variant or extend the existing error type."

The fragility is real: any future rephrasing of the marker (typo fix, wording change) silently breaks detection — is_missing_document_signing_key_error returns false, the auto-select suppression stops working, and the startup error banner reappears with no compiler warning.

The fix is to introduce a typed error enum and change get_selected_wallet's return type:

♻️ Suggested approach 
+#[derive(Debug)]
+pub enum GetWalletError {
+    MissingDocumentSigningKey { label: String, id: String },
+    NoKeyProvided,
+    DpnsError(String),
+}
+
+impl fmt::Display for GetWalletError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::MissingDocumentSigningKey { label, id } => write!(
+                f,
+                "Identity {} ({}) doesn't have an authentication key for signing document transitions",
+                label, id
+            ),
+            Self::NoKeyProvided => write!(f, "No key provided when getting selected wallet"),
+            Self::DpnsError(e) => write!(f, "DPNS preorder document type not found: {}", e),
+        }
+    }
+}
+
+pub fn is_missing_document_signing_key_error(err: &GetWalletError) -> bool {
+    matches!(err, GetWalletError::MissingDocumentSigningKey { .. })
+}

-const MISSING_DOCUMENT_SIGNING_KEY_MARKER: &str =
-    "doesn't have an authentication key for signing document transitions";
-
-pub fn is_missing_document_signing_key_error(error: &str) -> bool {
-    error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER)
-}

Then update get_selected_wallet's signature to -> Result<Option<Arc<RwLock<Wallet>>>, GetWalletError> and update all callsites to match on the variant rather than calling is_missing_document_signing_key_error(&e) on a String.

As per coding guidelines: "Never parse error strings to extract information — always use the typed error chain via downcast, match on variants, or access structured fields."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/ui/identities/mod.rs` around lines 36 - 43, Replace the fragile
string-marker approach by adding a typed error variant (e.g., define enum
GetWalletError { MissingDocumentSigningKey, /* other cases */ } or extend
TaskError with MissingDocumentSigningKey), remove
MISSING_DOCUMENT_SIGNING_KEY_MARKER and is_missing_document_signing_key_error,
change get_selected_wallet to return Result<Option<Arc<RwLock<Wallet>>>,
GetWalletError>, and update all callers to match on the returned error variant
(handle GetWalletError::MissingDocumentSigningKey explicitly) instead of calling
is_missing_document_signing_key_error(&e); ensure other error cases are
preserved and converted into the new typed error variants as appropriate.

Comment on lines +31 to +43

@thepastaclaw thepastaclaw May 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Substring-matching a String error is brittle — model the case as a typed variant

is_missing_document_signing_key_error decides control flow by error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER) against the formatted error string returned by get_selected_wallet (which still returns Result<_, String>). This is exactly the stringly-typed pattern the recent #739 TaskError migration moved away from, and it has three concrete failure modes:

  1. Any future tweak to the wording (capitalisation, punctuation, i18n) silently breaks the suppression — and there is no compile-time signal.
  2. The marker is a sentence fragment embedded mid-message (format!("Identity {} ({}) {}", ...)); callers are matching on a substring of a constructed string, which is easy to invalidate by accident.
  3. Other get_selected_wallet callers (e.g. register_contract_screen.rs:69, update_contract_screen.rs:75) use .unwrap_or(None) and silently swallow all errors — they could benefit from the same structural distinction to drop only this case while still surfacing real failures.

A dedicated error type (e.g. enum WalletLookupError { MissingDocumentSigningKey { identity_id, label }, DpnsContractMissing(...), NoKeyProvided } with Display for the user-facing rendering) would let call sites match Err(WalletLookupError::MissingDocumentSigningKey { .. }) => None instead of grepping a sentence, and would also keep Display text out of a const so i18n becomes possible later.

source: ['claude', 'codex']

🤖 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/identities/mod.rs`:
- [SUGGESTION] lines 31-43: Substring-matching a `String` error is brittle — model the case as a typed variant
  `is_missing_document_signing_key_error` decides control flow by `error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER)` against the formatted error string returned by `get_selected_wallet` (which still returns `Result<_, String>`). This is exactly the stringly-typed pattern the recent #739 `TaskError` migration moved away from, and it has three concrete failure modes:

1. Any future tweak to the wording (capitalisation, punctuation, i18n) silently breaks the suppression — and there is no compile-time signal.
2. The marker is a sentence fragment embedded mid-message (`format!("Identity {} ({}) {}", ...)`); callers are matching on a substring of a constructed string, which is easy to invalidate by accident.
3. Other `get_selected_wallet` callers (e.g. `register_contract_screen.rs:69`, `update_contract_screen.rs:75`) use `.unwrap_or(None)` and silently swallow *all* errors — they could benefit from the same structural distinction to drop only this case while still surfacing real failures.

A dedicated error type (e.g. `enum WalletLookupError { MissingDocumentSigningKey { identity_id, label }, DpnsContractMissing(...), NoKeyProvided }` with `Display` for the user-facing rendering) would let call sites match `Err(WalletLookupError::MissingDocumentSigningKey { .. }) => None` instead of grepping a sentence, and would also keep `Display` text out of a const so i18n becomes possible later.

Comment on lines +41 to +43

@thepastaclaw thepastaclaw May 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: No test pins the marker/message contract together

Suppression in ContactRequests::new / QRCodeGeneratorScreen::new (and the proposed ProfileScreen::new fix) depends on the invariant that get_selected_wallet always embeds MISSING_DOCUMENT_SIGNING_KEY_MARKER verbatim in its returned error string. No test asserts this round-trip — if anyone reformats the message, the helper silently starts returning false and the regression returns. Add a small #[test] that constructs an identity without a document-signing key, calls get_selected_wallet, and asserts is_missing_document_signing_key_error(&err) returns true. This is a low-cost guard until the call is migrated to a typed error.

source: ['claude']

🤖 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/identities/mod.rs`:
- [SUGGESTION] lines 41-43: No test pins the marker/message contract together
  Suppression in `ContactRequests::new` / `QRCodeGeneratorScreen::new` (and the proposed `ProfileScreen::new` fix) depends on the invariant that `get_selected_wallet` always embeds `MISSING_DOCUMENT_SIGNING_KEY_MARKER` verbatim in its returned error string. No test asserts this round-trip — if anyone reformats the message, the helper silently starts returning `false` and the regression returns. Add a small `#[test]` that constructs an identity without a document-signing key, calls `get_selected_wallet`, and asserts `is_missing_document_signing_key_error(&err)` returns true. This is a low-cost guard until the call is migrated to a typed error.


/// Retrieves the appropriate wallet (if any) associated with the given identity.
///
/// # Description
Expand Down Expand Up @@ -78,8 +92,24 @@ pub fn get_selected_wallet(
qualified_identity
.document_signing_key(&preorder_document_type)
.ok_or_else(|| {
"Identity doesn't have an authentication key for signing document transitions"
.to_string()
use dash_sdk::dpp::identity::accessors::IdentityGettersV0;
use dash_sdk::dpp::platform_value::string_encoding::Encoding;
let identity_label = qualified_identity
.alias
.as_deref()
.or_else(|| {
qualified_identity
.dpns_names
.first()
.map(|n| n.name.as_str())
})
.unwrap_or("");
format!(
"Identity {} ({}) {}",
identity_label,
qualified_identity.identity.id().to_string(Encoding::Base58),
MISSING_DOCUMENT_SIGNING_KEY_MARKER,
)
Comment on lines +95 to +112

@thepastaclaw thepastaclaw Mar 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Display impl for Identifier outputs Base58 — same encoding used elsewhere in the UI. No mismatch here.

@thepastaclaw thepastaclaw May 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a6bd8f3 — the identity id is now formatted explicitly with Encoding::Base58.

})?
Comment on lines +97 to 113

@thepastaclaw thepastaclaw May 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Empty identity label produces a double space in the rendered message

When alias is None and dpns_names is empty, identity_label becomes "" (line 106 unwrap_or("")), so the formatted message reads "Identity (4ZmR…) doesn't have an authentication key for signing document transitions" — note the double space and dangling "Identity ". The recent a6bd8f37 commit intentionally chose "" over a placeholder like "Unknown", so the format must branch on whether a label exists.

💡 Suggested change
Suggested change
let identity_label = qualified_identity
.alias
.as_deref()
.or_else(|| {
qualified_identity
.dpns_names
.first()
.map(|n| n.name.as_str())
})
.unwrap_or("");
format!(
"Identity {} ({}) {}",
identity_label,
qualified_identity.identity.id().to_string(Encoding::Base58),
MISSING_DOCUMENT_SIGNING_KEY_MARKER,
)
})?
let identity_label = qualified_identity.alias.as_deref().or_else(|| {
qualified_identity
.dpns_names
.first()
.map(|n| n.name.as_str())
});
let id_b58 = qualified_identity.identity.id().to_string(Encoding::Base58);
match identity_label {
Some(label) => format!(
"Identity {} ({}) {}",
label, id_b58, MISSING_DOCUMENT_SIGNING_KEY_MARKER,
),
None => format!(
"Identity ({}) {}",
id_b58, MISSING_DOCUMENT_SIGNING_KEY_MARKER,
),
}

source: ['claude']

} else {
// Fallback: directly use the provided selected key.
Expand Down
Loading