fix(ui): improve auth key error message and suppress on startup auto-select#761
fix(ui): improve auth key error message and suppress on startup auto-select#761thepastaclaw wants to merge 2 commits into
Conversation
…select
1. Better error message: Include the identity alias/DPNS name and ID
in the error, so users with multiple identities can identify which
one lacks a signing key.
Before: Identity doesn't have an authentication key for signing
document transitions
After: Identity 'my-evonode' (7xK3...) doesn't have an
authentication key for signing document transitions
2. Suppress banner on auto-select: The ContactRequests and
QRCodeGenerator screens auto-select the first identity during
construction. If that identity lacks a document signing key (e.g.,
evonode identities), the error was shown immediately before the user
did anything. Now these screens silently set wallet to None on
auto-select — the error only appears when the user explicitly
selects an identity that lacks the key.
Closes dashpay#743
📝 WalkthroughWalkthroughAuto-selection of the first identity wallet now suppresses the specific "missing document-signing key" error (no banner) while still surfacing other wallet-fetch errors via global MessageBanner. The identities module formats that missing-key error with identity label and Base58 id and exposes a helper to detect it. ChangesAuto-selection Error Handling
Error Message Enhancement
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 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
| .or_else(|| qualified_identity.dpns_names.first().map(|n| n.name.as_str())) | ||
| .unwrap_or("unknown"); | ||
| format!( | ||
| "Identity '{}' ({}) doesn't have an authentication key for signing document transitions", |
There was a problem hiding this comment.
claw remove quotes, and when no name found, put empty string. We will just have a few spaces when identity label is not availble.
There was a problem hiding this comment.
Done — removed quotes and switched the fallback to empty string. 83f602c
There was a problem hiding this comment.
Done in 83f602c — removed quotes and switched to empty string when no name is found.
There was a problem hiding this comment.
Done in a6bd8f3 — preserved the no-quotes formatting and empty-string fallback after the amended update.
There was a problem hiding this comment.
Pull request overview
Improves identity-related UX by making the “missing document signing key” error actionable (includes identity label + id) and preventing the banner from appearing during startup auto-selection flows.
Changes:
- Enrich missing signing key error with identity alias/DPNS name and identity id.
- Suppress error banner triggered by constructor-time auto-select in ContactRequests and QRCodeGenerator.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ui/identities/mod.rs | Expands the missing signing key error to include identity label + id. |
| src/ui/dashpay/qr_code_generator.rs | Stops showing the error banner during startup auto-selection of the first identity. |
| src/ui/dashpay/contact_requests.rs | Stops showing the error banner during startup auto-selection of the first identity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use dash_sdk::dpp::identity::accessors::IdentityGettersV0; | ||
| let identity_label = qualified_identity | ||
| .alias | ||
| .as_deref() | ||
| .or_else(|| qualified_identity.dpns_names.first().map(|n| n.name.as_str())) | ||
| .unwrap_or("unknown"); | ||
| format!( | ||
| "Identity '{}' ({}) doesn't have an authentication key for signing document transitions", | ||
| identity_label, | ||
| qualified_identity.identity.id() | ||
| ) |
There was a problem hiding this comment.
The Display impl for Identifier outputs Base58 — same encoding used elsewhere in the UI. No mismatch here.
There was a problem hiding this comment.
Fixed in a6bd8f3 — the identity id is now formatted explicitly with Encoding::Base58.
| // Get wallet for the selected identity (don't show error on auto-select; | ||
| // some identities like evonodes may lack document signing keys) | ||
| new_self.selected_wallet = | ||
| get_selected_wallet(&identities[0], Some(&app_context), None) | ||
| .or_show_error(app_context.egui_ctx()) | ||
| .unwrap_or(None); | ||
| get_selected_wallet(&identities[0], Some(&app_context), None).unwrap_or(None); |
There was a problem hiding this comment.
Fair point in theory, but in practice the only other error path here is DPNS preorder document type not found — which means the DPNS contract is broken/missing, and the app would fail in many other places too. Selectively filtering error types here adds complexity for a scenario that's already catastrophic. The current approach keeps the auto-select path clean.
There was a problem hiding this comment.
Fixed in a6bd8f3 — startup auto-select now suppresses only the expected missing document-signing-key error and surfaces other get_selected_wallet errors with a banner.
| get_selected_wallet(&identities[0], Some(&app_context), None) | ||
| .or_show_error(app_context.egui_ctx()) | ||
| .unwrap_or(None); | ||
| get_selected_wallet(&identities[0], Some(&app_context), None).unwrap_or(None); |
There was a problem hiding this comment.
Same rationale as the QRCodeGenerator comment above — the only other error is a missing DPNS contract, which is a global failure, not something worth special-casing here.
There was a problem hiding this comment.
Fixed in a6bd8f3 — same selective handling as QRCodeGenerator: only the missing signing-key case is suppressed, other errors are shown.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ Review complete (commit a6bd8f3) |
Per lklimek's review: remove single quotes around identity label in error message, and use empty string instead of 'unknown' when no alias/DPNS name is available.
thepastaclaw
force-pushed
the
fix/startup-auth-key-error-v1.0-dev
branch
from
83f602c to
a6bd8f3
Compare
![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
🤖 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/ui/identities/mod.rs`:
- Around line 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.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5389d311-d95d-43fe-abfa-635e38acbaf6
📒 Files selected for processing (3)
src/ui/dashpay/contact_requests.rssrc/ui/dashpay/qr_code_generator.rssrc/ui/identities/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ui/dashpay/qr_code_generator.rs
- src/ui/dashpay/contact_requests.rs
| const MISSING_DOCUMENT_SIGNING_KEY_MARKER: &str = | ||
| "doesn't have an authentication key for signing document transitions"; | ||
|
|
||
| /// 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) | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The fix is partial and built on a fragile substring marker. Codex correctly identifies that ProfileScreen::new — which is eagerly constructed at startup as part of every DashPayScreen — still calls .or_show_error() and will reproduce the exact bug this PR aims to fix. The substring-based detection is also at odds with the recent #739 typed-error migration, and the rendered message has both an empty-label formatting bug and a jargon problem.
Reviewed commit: a6bd8f3
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🔴 blocking: Startup banner still fires from `ProfileScreen::new` — fix is incomplete
src/ui/dashpay/profile_screen.rs (lines 206-210)
This PR suppresses the auto-select banner in ContactRequests::new and QRCodeGeneratorScreen::new, but ProfileScreen::new still calls get_selected_wallet(...).or_show_error(app_context.egui_ctx()).unwrap_or(None) at line 207–210. Confirmed in src/app.rs:288-297: AppState eagerly constructs contracts_dashpay_screen, dashpay_contacts_screen, dashpay_profile_screen, and dashpay_payments_screen, and DashPayScreen::new (src/ui/dashpay/dashpay_screen.rs:33-41) unconditionally embeds a ProfileScreen regardless of which subscreen is requested. So for an evonode-only user, the same "missing document-signing key" banner described in #743 will still appear at app startup — the regression is not actually eliminated.
💡 Suggested change
// 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 =
match get_selected_wallet(&identities[selected_idx], Some(&app_context), None) {
Ok(wallet) => wallet,
Err(e) if crate::ui::identities::is_missing_document_signing_key_error(&e) => None,
Err(e) => {
MessageBanner::set_global(app_context.egui_ctx(), &e, MessageType::Error);
None
}
};
🤖 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/ui/dashpay/profile_screen.rs`:
- [BLOCKING] lines 206-210: Startup banner still fires from `ProfileScreen::new` — fix is incomplete
This PR suppresses the auto-select banner in `ContactRequests::new` and `QRCodeGeneratorScreen::new`, but `ProfileScreen::new` still calls `get_selected_wallet(...).or_show_error(app_context.egui_ctx()).unwrap_or(None)` at line 207–210. Confirmed in `src/app.rs:288-297`: AppState eagerly constructs `contracts_dashpay_screen`, `dashpay_contacts_screen`, `dashpay_profile_screen`, and `dashpay_payments_screen`, and `DashPayScreen::new` (`src/ui/dashpay/dashpay_screen.rs:33-41`) unconditionally embeds a `ProfileScreen` regardless of which subscreen is requested. So for an evonode-only user, the same "missing document-signing key" banner described in #743 will still appear at app startup — the regression is not actually eliminated.
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.
- [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.
- [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.
| /// 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"; | ||
|
|
||
| /// 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) | ||
| } |
There was a problem hiding this comment.
🟡 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:
- Any future tweak to the wording (capitalisation, punctuation, i18n) silently breaks the suppression — and there is no compile-time signal.
- 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. - Other
get_selected_walletcallers (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.
| 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, | ||
| ) | ||
| })? |
There was a problem hiding this comment.
💬 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
| 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']
| const MISSING_DOCUMENT_SIGNING_KEY_MARKER: &str = | ||
| "doesn't have an authentication key for signing document transitions"; |
There was a problem hiding this comment.
🟡 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.
| pub fn is_missing_document_signing_key_error(error: &str) -> bool { | ||
| error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER) | ||
| } |
There was a problem hiding this comment.
🟡 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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR's intent (suppress the noisy startup banner and add identity context to the missing-signing-key error) is correct, but the implementation has two real defects that reach exactly the case the PR targets: ProfileScreen is built eagerly by DashPayScreen at startup yet was not patched, so the banner still appears, and identities without an alias or DPNS name (the evonode case in the PR description) render as 'Identity (id) ...' with awkward double whitespace and a dangling word. Architecturally, the suppression mechanism couples control flow to the substring of a translatable user-facing string, which is fragile and conflicts with project i18n rules.
Reviewed commit: a6bd8f3
Fresh current-run review completed for this dispatcher item.
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
🔴 blocking: ProfileScreen auto-select still surfaces the banner the PR claims to suppress
src/ui/dashpay/profile_screen.rs:206-210
DashPayScreen::new (src/ui/dashpay/dashpay_screen.rs:38) eagerly constructs ProfileScreen alongside ContactsList and PaymentHistory at startup, and AppState builds multiple DashPayScreen instances. ContactRequests::new and QRCodeGeneratorScreen::new were updated to drop the missing-signing-key error via is_missing_document_signing_key_error, but ProfileScreen::new still calls get_selected_wallet(&identities[selected_idx], Some(&app_context), None).or_show_error(app_context.egui_ctx()) on the same auto-select path. For an evonode identity without a document-signing key — the exact scenario in #743 — the global banner still fires at startup, so the user-visible bug is only partially fixed. Apply the same Err(e) if is_missing_document_signing_key_error(&e) => None arm here, or push the suppression down into get_selected_wallet/or_show_error so all auto-select callsites get it consistently.
💡 Suggested change
// Get wallet for the selected identity
new_self.selected_wallet =
match get_selected_wallet(&identities[selected_idx], Some(&app_context), None) {
Ok(wallet) => wallet,
Err(e) if crate::ui::identities::is_missing_document_signing_key_error(&e) => {
None
}
Err(e) => {
MessageBanner::set_global(app_context.egui_ctx(), &e, MessageType::Error);
None
}
};
🔴 blocking: Empty identity label produces malformed 'Identity (id) ...' message
src/ui/identities/mod.rs:97-112
When an identity has neither an alias nor any dpns_names, identity_label falls back to "" (line 106), and the format string "Identity {} ({}) {}" produces "Identity (4ZmR…) doesn't have an authentication key for signing document transitions" — note the double space and the dangling word Identity. This is the most common shape for non-user identities (evonodes), which is exactly the example called out in the PR description, so the message the PR is trying to clarify becomes garbled for the primary affected user. Branch on whether a label exists rather than emitting an empty placeholder.
💡 Suggested change
let identity_id = qualified_identity
.identity
.id()
.to_string(Encoding::Base58);
let identity_label = qualified_identity
.alias
.as_deref()
.or_else(|| {
qualified_identity
.dpns_names
.first()
.map(|n| n.name.as_str())
});
match identity_label {
Some(label) => format!(
"Identity {} ({}) {}",
label, identity_id, MISSING_DOCUMENT_SIGNING_KEY_MARKER,
),
None => format!(
"Identity ({}) {}",
identity_id, MISSING_DOCUMENT_SIGNING_KEY_MARKER,
),
}
🟡 suggestion: Substring-matching a user-facing error string is fragile and conflicts with i18n rules
src/ui/identities/mod.rs:31-43
is_missing_document_signing_key_error decides program behaviour (suppress vs. surface a banner) by error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER) against text that is also rendered verbatim to the user. Per CLAUDE.md, user-facing strings must be 'simple, complete sentences' and 'single extractable units with named placeholders' — but the marker is spliced into the middle of a larger sentence, so any future copy edit, rephrasing, or i18n extraction will silently break the suppression and the startup banner returns. It also concatenates a static fragment, which won't survive translation.
A typed signal removes the substring contract and matches the project's stated direction. Two options:
- Change
get_selected_wallet's return type to a small enum (e.g.Result<Option<Arc<RwLock<Wallet>>>, GetSelectedWalletError>with variants likeMissingDocumentSigningKey { identity_label: Option<String>, identity_id: String },DpnsPreorderTypeUnavailable(...),NoKeyProvided). Callers pattern-match the variant; the UI boundary formatsDisplay. - If keeping the
Stringerror: returnOk(None)for the missing-signing-key case (it is legitimately a 'no wallet for this identity' outcome) and let the screens render a hint when they actually need a signer.
Either approach erases the MISSING_DOCUMENT_SIGNING_KEY_MARKER constant and the is_missing_document_signing_key_error helper.
🟡 suggestion: Error message lacks a self-service action and uses everyday-user jargon
src/ui/identities/mod.rs:107-112
CLAUDE.md's error-message rules require what happened + what to do with a concrete self-service action, and explicitly prohibit jargon — state transition is cited as an example. The current message — "... doesn't have an authentication key for signing document transitions" — describes mechanism (authentication key, document transitions) without telling the user how to resolve it. Since this banner now appears only on user-initiated identity selection (auto-select suppresses it), actionability matters more than before. Reword along the lines of "Identity () cannot sign DashPay actions because it has no signing key. Add a signing key with high or critical security level to this identity, or pick a different identity."
🟡 suggestion: No tests pin the marker contract or the format-string output
src/ui/identities/mod.rs:31-43
Two new behaviours are now load-bearing without coverage: (1) the formatted error must contain MISSING_DOCUMENT_SIGNING_KEY_MARKER for is_missing_document_signing_key_error to suppress the banner, and (2) the label/id formatting (including the empty-label edge case). A future innocent edit to the format string will silently break suppression. A small unit test that builds a QualifiedIdentity with no signing key and asserts the substring is present and the rendered message is well-formed across the three label states (alias=Some, alias=None+dpns_name, alias=None+no dpns_name) would lock the contract until a typed-error refactor lands.
💬 nitpick: `use` statements buried inside closure body
src/ui/identities/mod.rs:95-96
use dash_sdk::dpp::identity::accessors::IdentityGettersV0; and use dash_sdk::dpp::platform_value::string_encoding::Encoding; are placed inside the ok_or_else closure. Hoisting them to the file's top-level use block (or at minimum to the function scope) makes the trait dependencies discoverable and matches the surrounding style — IdentityGettersV0 is already imported via the same path elsewhere in this codebase.
🤖 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/ui/dashpay/profile_screen.rs`:
- [BLOCKING] lines 206-210: ProfileScreen auto-select still surfaces the banner the PR claims to suppress
`DashPayScreen::new` (src/ui/dashpay/dashpay_screen.rs:38) eagerly constructs `ProfileScreen` alongside `ContactsList` and `PaymentHistory` at startup, and `AppState` builds multiple `DashPayScreen` instances. `ContactRequests::new` and `QRCodeGeneratorScreen::new` were updated to drop the missing-signing-key error via `is_missing_document_signing_key_error`, but `ProfileScreen::new` still calls `get_selected_wallet(&identities[selected_idx], Some(&app_context), None).or_show_error(app_context.egui_ctx())` on the same auto-select path. For an evonode identity without a document-signing key — the exact scenario in #743 — the global banner still fires at startup, so the user-visible bug is only partially fixed. Apply the same `Err(e) if is_missing_document_signing_key_error(&e) => None` arm here, or push the suppression down into `get_selected_wallet`/`or_show_error` so all auto-select callsites get it consistently.
In `src/ui/identities/mod.rs`:
- [BLOCKING] lines 97-112: Empty identity label produces malformed 'Identity (id) ...' message
When an identity has neither an `alias` nor any `dpns_names`, `identity_label` falls back to `""` (line 106), and the format string `"Identity {} ({}) {}"` produces `"Identity (4ZmR…) doesn't have an authentication key for signing document transitions"` — note the double space and the dangling word `Identity`. This is the most common shape for non-user identities (evonodes), which is exactly the example called out in the PR description, so the message the PR is trying to clarify becomes garbled for the primary affected user. Branch on whether a label exists rather than emitting an empty placeholder.
- [SUGGESTION] lines 31-43: Substring-matching a user-facing error string is fragile and conflicts with i18n rules
`is_missing_document_signing_key_error` decides program behaviour (suppress vs. surface a banner) by `error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER)` against text that is also rendered verbatim to the user. Per CLAUDE.md, user-facing strings must be 'simple, complete sentences' and 'single extractable units with named placeholders' — but the marker is spliced into the middle of a larger sentence, so any future copy edit, rephrasing, or i18n extraction will silently break the suppression and the startup banner returns. It also concatenates a static fragment, which won't survive translation.
A typed signal removes the substring contract and matches the project's stated direction. Two options:
1. Change `get_selected_wallet`'s return type to a small enum (e.g. `Result<Option<Arc<RwLock<Wallet>>>, GetSelectedWalletError>` with variants like `MissingDocumentSigningKey { identity_label: Option<String>, identity_id: String }`, `DpnsPreorderTypeUnavailable(...)`, `NoKeyProvided`). Callers pattern-match the variant; the UI boundary formats `Display`.
2. If keeping the `String` error: return `Ok(None)` for the missing-signing-key case (it is legitimately a 'no wallet for this identity' outcome) and let the screens render a hint when they actually need a signer.
Either approach erases the `MISSING_DOCUMENT_SIGNING_KEY_MARKER` constant and the `is_missing_document_signing_key_error` helper.
- [SUGGESTION] lines 107-112: Error message lacks a self-service action and uses everyday-user jargon
CLAUDE.md's error-message rules require *what happened + what to do* with a concrete self-service action, and explicitly prohibit jargon — `state transition` is cited as an example. The current message — `"... doesn't have an authentication key for signing document transitions"` — describes mechanism (`authentication key`, `document transitions`) without telling the user how to resolve it. Since this banner now appears only on user-initiated identity selection (auto-select suppresses it), actionability matters more than before. Reword along the lines of *"Identity <label> (<id>) cannot sign DashPay actions because it has no signing key. Add a signing key with high or critical security level to this identity, or pick a different identity."*
- [SUGGESTION] lines 31-43: No tests pin the marker contract or the format-string output
Two new behaviours are now load-bearing without coverage: (1) the formatted error must contain `MISSING_DOCUMENT_SIGNING_KEY_MARKER` for `is_missing_document_signing_key_error` to suppress the banner, and (2) the label/id formatting (including the empty-label edge case). A future innocent edit to the format string will silently break suppression. A small unit test that builds a `QualifiedIdentity` with no signing key and asserts the substring is present and the rendered message is well-formed across the three label states (alias=Some, alias=None+dpns_name, alias=None+no dpns_name) would lock the contract until a typed-error refactor lands.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused PR that suppresses a noisy startup banner and enriches an error with identity context. Two real concerns: an empty-label code path produces a malformed user-facing message ('Identity (id) ...' with double space), and the suppression discriminator is a substring match against user-facing wording — fragile post the #739 typed-error refactor and at odds with the i18n-ready string rule. A secondary UX concern is that suppressing the banner defers the failure to the Accept/Reject click in ContactRequests.
Reviewed commit: a6bd8f3
Fresh dispatcher gate review for this claimed queue item. review_poster.py detected an existing same-SHA review, so this records the fresh verification as a top-level review without duplicating inline threads.
🟡 4 suggestion(s) | 💬 2 nitpick(s)
🟡 suggestion: Empty label renders malformed message with double space
src/ui/identities/mod.rs:94-113
When the identity has no alias and no dpns_names, identity_label becomes "" via unwrap_or(""), so the format string "Identity {} ({}) {}" renders as "Identity (4ZmR…) doesn't have an authentication key for signing document transitions" — note the double space. Commit a6bd8f37 ("remove quotes from identity label and use empty string for unknown") created exactly this path. Branch on Some/None so the label segment (including its trailing space) is omitted when no label is known, e.g. "Identity ({id}) {marker}" vs "Identity {label} ({id}) {marker}". The existing Display for QualifiedIdentity (src/model/qualified_identity/mod.rs:295) is not directly substitutable because its no-label fallback is the identity id itself, which would produce "Identity {id} ({id}) ...".
🟡 suggestion: Suppression discriminator is a substring match on user-facing wording
src/ui/identities/mod.rs:31-43
is_missing_document_signing_key_error distinguishes the suppressible case via error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER). This couples error identity to display text, exactly what #739 (Result<T, String> → TaskError) was meant to eliminate, and it conflicts with the CLAUDE.md i18n rule that messages be a single extractable translation unit (the marker is a sentence-fragment tail). If the wording is ever reworded, localized, or wrapped, suppression silently breaks and the original startup-banner regression returns with no compile-time signal. Convert get_selected_wallet to return a small typed error (e.g. enum GetSelectedWalletError { MissingDocumentSigningKey { label, id }, DpnsPreorderType(#[source] …), NoKeyProvided }); auto-select callsites then match Err(GetSelectedWalletError::MissingDocumentSigningKey { .. }) => None. That removes the marker constant, the helper, and the fragility — and aligns this helper with the rest of the codebase post-#739. The 30+ existing callsites mostly stringify or display the error, so the swap is largely mechanical.
🟡 suggestion: Suppressing the startup banner defers the failure to the Accept/Reject click
src/ui/dashpay/contact_requests.rs:110-118
When the missing-key error is suppressed, selected_wallet is set to None. The list view treats None as wallet_locked = false (contact_requests.rs:834-836) and renders active Accept/Reject buttons (line 843+). Both action paths still require an authentication key in the backend: accept_contact_request() fails with DashPayError::MissingAuthenticationKey (backend_task/dashpay/contact_requests.rs:643) and reject_contact_request() reaches create_or_update_contact_info() with the same requirement (backend_task/dashpay/contact_info.rs:403). Net effect: an auto-selected identity that lacks the key is now silent on arrival, but the first click still fails after the confirmation dialog instead of guiding the user to switch identities or add a key. Consider disabling the buttons (or showing an inline note on the row) when selected_wallet is None due to this specific case, so the limitation is visible before the user commits to an action.
🟡 suggestion: No test coverage for the suppression helper or the new error format
src/ui/identities/mod.rs:92-113
Both the new is_missing_document_signing_key_error helper and the wording produced at line 107-112 are now load-bearing for the suppression behaviour at startup, with no unit or UI test covering either. The most likely regression — someone editing the wording without updating the marker constant, or a future i18n pass — would silently reintroduce the original noisy-banner bug from #743 with no test failure. A small inline #[test] asserting is_missing_document_signing_key_error(&format!("Identity {} ({}) {}", "label", "id", MISSING_DOCUMENT_SIGNING_KEY_MARKER)) is true (and obviously-unrelated strings are false) is the cheapest insurance while the substring coupling exists; switching to a typed error (other finding) makes this redundant.
💬 nitpick: Concatenated format violates the i18n-ready string guideline
src/ui/identities/mod.rs:107-112
CLAUDE.md: "All user-facing strings must be simple, complete sentences. Avoid concatenating fragments… Each string should be extractable as a single translation unit with named placeholders." The current format!("Identity {} ({}) {}", label, id, MARKER) splices a sentence-fragment tail onto a positional prefix — a translator would receive two disjoint pieces with locked English word order. Inline the full sentence as a single template with named placeholders ("Identity {label} ({id}) doesn't have an authentication key for signing document transitions."). Combined with the typed-error suggestion, this lives naturally on the variant's #[error("...")] attribute as one extractable unit.
💬 nitpick: Suppression block is duplicated across two callsites
src/ui/dashpay/contact_requests.rs:110-118
The exact match arm is repeated in contact_requests.rs:110-118 and qr_code_generator.rs:84-92. At two callsites this is borderline, but if a third auto-select path needs the same policy (or the typed-error refactor lands), a small helper — e.g. auto_select_wallet(&identity, &app_context, ctx) -> Option<Arc<RwLock<Wallet>>> — keeps the suppression policy in one place. Take it or leave it.
🤖 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/ui/identities/mod.rs`:
- [SUGGESTION] lines 94-113: Empty label renders malformed message with double space
When the identity has no `alias` and no `dpns_names`, `identity_label` becomes `""` via `unwrap_or("")`, so the format string `"Identity {} ({}) {}"` renders as `"Identity (4ZmR…) doesn't have an authentication key for signing document transitions"` — note the double space. Commit `a6bd8f37` ("remove quotes from identity label and use empty string for unknown") created exactly this path. Branch on `Some/None` so the label segment (including its trailing space) is omitted when no label is known, e.g. `"Identity ({id}) {marker}"` vs `"Identity {label} ({id}) {marker}"`. The existing `Display for QualifiedIdentity` (`src/model/qualified_identity/mod.rs:295`) is not directly substitutable because its no-label fallback is the identity id itself, which would produce `"Identity {id} ({id}) ..."`.
- [SUGGESTION] lines 31-43: Suppression discriminator is a substring match on user-facing wording
`is_missing_document_signing_key_error` distinguishes the suppressible case via `error.contains(MISSING_DOCUMENT_SIGNING_KEY_MARKER)`. This couples error identity to display text, exactly what #739 (`Result<T, String>` → `TaskError`) was meant to eliminate, and it conflicts with the CLAUDE.md i18n rule that messages be a single extractable translation unit (the marker is a sentence-fragment tail). If the wording is ever reworded, localized, or wrapped, suppression silently breaks and the original startup-banner regression returns with no compile-time signal. Convert `get_selected_wallet` to return a small typed error (e.g. `enum GetSelectedWalletError { MissingDocumentSigningKey { label, id }, DpnsPreorderType(#[source] …), NoKeyProvided }`); auto-select callsites then `match Err(GetSelectedWalletError::MissingDocumentSigningKey { .. }) => None`. That removes the marker constant, the helper, and the fragility — and aligns this helper with the rest of the codebase post-#739. The 30+ existing callsites mostly stringify or display the error, so the swap is largely mechanical.
- [SUGGESTION] lines 92-113: No test coverage for the suppression helper or the new error format
Both the new `is_missing_document_signing_key_error` helper and the wording produced at line 107-112 are now load-bearing for the suppression behaviour at startup, with no unit or UI test covering either. The most likely regression — someone editing the wording without updating the marker constant, or a future i18n pass — would silently reintroduce the original noisy-banner bug from #743 with no test failure. A small inline `#[test]` asserting `is_missing_document_signing_key_error(&format!("Identity {} ({}) {}", "label", "id", MISSING_DOCUMENT_SIGNING_KEY_MARKER))` is `true` (and obviously-unrelated strings are `false`) is the cheapest insurance while the substring coupling exists; switching to a typed error (other finding) makes this redundant.
In `src/ui/dashpay/contact_requests.rs`:
- [SUGGESTION] lines 110-118: Suppressing the startup banner defers the failure to the Accept/Reject click
When the missing-key error is suppressed, `selected_wallet` is set to `None`. The list view treats `None` as `wallet_locked = false` (`contact_requests.rs:834-836`) and renders active Accept/Reject buttons (line 843+). Both action paths still require an authentication key in the backend: `accept_contact_request()` fails with `DashPayError::MissingAuthenticationKey` (`backend_task/dashpay/contact_requests.rs:643`) and `reject_contact_request()` reaches `create_or_update_contact_info()` with the same requirement (`backend_task/dashpay/contact_info.rs:403`). Net effect: an auto-selected identity that lacks the key is now silent on arrival, but the first click still fails after the confirmation dialog instead of guiding the user to switch identities or add a key. Consider disabling the buttons (or showing an inline note on the row) when `selected_wallet` is `None` due to this specific case, so the limitation is visible before the user commits to an action.
Issue
Closes #743 — On startup, DET shows a confusing "Identity doesn't have an authentication key for signing document transitions" error banner with no indication of which identity is affected.
Supersedes #750 — recreated on top of v1.0-dev after the error handling refactor (#739) was merged, per lklimek's request.
Changes
Better error message: Include the identity alias/DPNS name and ID in the error, so users with multiple identities can identify which one lacks a signing key.
Identity doesn't have an authentication key for signing document transitionsIdentity 'my-evonode' (4ZmR...) doesn't have an authentication key for signing document transitionsSuppress error banner on startup auto-select: In
ContactRequestsandQRCodeGeneratorconstructors, the first identity is auto-selected. If it lacks a document signing key (e.g. evonodes), the error banner no longer fires. User-initiated selection still shows errors correctly.Testing
cargo check,cargo clippy,cargo fmt— all cleanSummary by CodeRabbit