refactor: return typed TaskError from identity backend tasks#730
Merged
refactor: return typed TaskError from identity backend tasks#730
Conversation
Replace raw base64-encoded CBOR error with actionable messages when adding a duplicate key to an identity. Match structured SDK error variants (ConsensusError::StateError) instead of string parsing. - Add TaskError variants: DuplicateIdentityPublicKey, DuplicateIdentityPublicKeyId, IdentityPublicKeyContractBoundsConflict - Add broadcast_error_message() helper matching both SDK error paths (StateTransitionBroadcastError and Protocol/ConsensusError) - 5 unit tests covering all variants + fallback - Manual test scenarios document Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… per-identity DuplicatedIdentityPublicKeyStateError and DuplicatedIdentityPublicKeyIdStateError are platform-wide uniqueness constraints, not per-identity. Updated error messages to reflect that keys must be globally unique across all identities. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Only unique key types (ECDSA_SECP256K1, BLS12_381) require platform-wide uniqueness. Non-unique types (ECDSA_HASH160, BIP13_SCRIPT_HASH, EDDSA_25519_HASH160) can be shared across identities. Simplified messages to avoid misleading users about key uniqueness rules. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…identity - broadcast_error() now returns TaskError directly instead of String - add_key_to_identity() returns Result<..., TaskError> — typed variants flow through without String round-trip - run_identity_task() returns Result<..., TaskError> — other arms auto-convert via From<String> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ad-hoc string errors with a typed Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Backend as BackendTask
participant SDK as SDK
participant Storage as LocalStorage
participant Wallet as Wallet
UI->>Backend: request add_key(identity_id, public_key)
Backend->>Wallet: unlock/sign if needed
Backend->>SDK: broadcast state transition (add key)
alt SDK returns Consensus/SDK error
SDK-->>Backend: SdkError
Backend-->>UI: TaskError::SdkError / mapped variant
else SDK returns success + tx info
SDK-->>Backend: BroadcastResult(tx_info, actual_fee)
Backend->>Storage: update local identity public keys
Backend->>Wallet: adjust balance by actual_fee/estimate
Backend-->>UI: BackendTaskSuccessResult (including FeeResult)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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. Comment |
…-key-error # Conflicts: # src/backend_task/identity/add_key_to_identity.rs
lklimek
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the backend identity task execution flow to use the shared typed TaskError envelope instead of plain String errors, and improves error handling in the “add key to identity” broadcast path by mapping known consensus errors into specific TaskError variants.
Changes:
- Switch
run_identity_taskto returnResult<_, TaskError>and adapt match arms to propagate/convert errors via?. - Replace string-based broadcast error messages in
add_key_to_identitywith a typedTaskErrormapping and update unit tests accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/backend_task/identity/mod.rs | Converts identity task runner to return TaskError, enabling typed error propagation across backend task orchestration. |
| src/backend_task/identity/add_key_to_identity.rs | Introduces broadcast_error that returns TaskError variants for known consensus errors; updates add_key_to_identity signature and adjusts tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The Platform SDK changed Identifier::to_string() to require an explicit Encoding argument. Updated both production and test code to use to_string(Encoding::Base58) instead of the no-arg variant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/backend_task/identity/add_key_to_identity.rs (1)
26-57: Keep the rawSdkErrorvisible when mapping to typed variants.
broadcast_error()now collapses duplicate-key failures into unitTaskErrorvariants. After that boundary, the original broadcast code/cause is gone, so later logs only see the coarse variant.As per coding guidelines, "Ensure Display produces user-friendly text for MessageBanner and Debug provides technical details for logs."Minimal way to preserve the technical context
fn broadcast_error(error: &SdkError) -> TaskError { + tracing::warn!("AddKeyToIdentity broadcast failed: {:?}", error); + let consensus_error = match error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/add_key_to_identity.rs` around lines 26 - 57, broadcast_error currently discards the original SdkError when converting to typed TaskError variants; preserve the technical context by extending the TaskError variants (e.g., DuplicateIdentityPublicKey, DuplicateIdentityPublicKeyId, IdentityPublicKeyContractBoundsConflict) to carry a diagnostic payload (a String or the SdkError itself) and populate that payload in broadcast_error with the original error (e.g., format!("{:?}", error)); also ensure the fallback TaskError::Generic continues to include the full error. Update the TaskError enum and the returns inside broadcast_error (the match arms that return TaskError::DuplicateIdentityPublicKey, TaskError::DuplicateIdentityPublicKeyId, and TaskError::IdentityPublicKeyContractBoundsConflict) to include the diagnostic/source field so logs can access the raw SdkError while keeping user-facing messages unchanged.
🤖 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/identity/mod.rs`:
- Around line 551-583: The identity task branches are converting errors into
plain Strings (e.g., refresh_identity is mapped with .map_err(|e| format!(...))
and helpers top_up_identity_from_platform_addresses and transfer_to_addresses
return Result<..., String>), which forces callers to fall back to
TaskError::Generic; change these flows to use the typed TaskError enum instead:
update the signatures of top_up_identity_from_platform_addresses and
transfer_to_addresses (and any helper that returns Result<..., String>) to
return Result<BackendTaskSuccessResult, TaskError> (or map their internal errors
into specific TaskError variants), and remove the .map_err(... format!) in the
refresh_identity branch so it propagates a TaskError; ensure run_identity_task
and the
IdentityTask::RefreshIdentity/TopUpIdentityFromPlatformAddresses/TransferToAddresses
branches propagate the typed TaskError rather than converting to String.
---
Nitpick comments:
In `@src/backend_task/identity/add_key_to_identity.rs`:
- Around line 26-57: broadcast_error currently discards the original SdkError
when converting to typed TaskError variants; preserve the technical context by
extending the TaskError variants (e.g., DuplicateIdentityPublicKey,
DuplicateIdentityPublicKeyId, IdentityPublicKeyContractBoundsConflict) to carry
a diagnostic payload (a String or the SdkError itself) and populate that payload
in broadcast_error with the original error (e.g., format!("{:?}", error)); also
ensure the fallback TaskError::Generic continues to include the full error.
Update the TaskError enum and the returns inside broadcast_error (the match arms
that return TaskError::DuplicateIdentityPublicKey,
TaskError::DuplicateIdentityPublicKeyId, and
TaskError::IdentityPublicKeyContractBoundsConflict) to include the
diagnostic/source field so logs can access the raw SdkError while keeping
user-facing messages unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ce0cd96-9138-4417-9b7a-c4fb8cc91db8
📒 Files selected for processing (2)
src/backend_task/identity/add_key_to_identity.rssrc/backend_task/identity/mod.rs
Contributor
Author
|
@copilot please address comments |
6 tasks
Contributor
lklimek
commented
Mar 11, 2026
…-key errors (#731) * Initial plan * fix: log raw SdkError in broadcast_error to preserve technical context Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: embed raw SdkError as source_error field in typed TaskError variants Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: store Box<SdkError> as typed source in TaskError variants instead of debug string Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * style: fix rustfmt formatting issues in error.rs and add_key_to_identity.rs Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/backend_task/identity/add_key_to_identity.rs (1)
89-101:⚠️ Potential issue | 🟠 MajorThis task is still mostly string-wrapping its errors.
Even after changing the return type to
TaskError, the fetch/persistence failures in this path still go through ad-hoc strings /Generic(...). That means callers still cannot match those failures structurally, and the original error chain is dropped. Please add#[from]conversions for the upstream error types and propagate them directly instead of formatting them first.As per coding guidelines, “When adding new backend error types, add a
#[from]variant toTaskErrorrather than converting toString.”Also applies to: 186-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/add_key_to_identity.rs` around lines 89 - 101, The code is converting upstream errors into strings instead of propagating typed TaskError variants; update the error handling so calls like sdk.get_identity_nonce(...).await and Identity::fetch_by_identifier(...).await use the ? operator to propagate their original error types, and add corresponding #[from] variants to the TaskError enum for the SDK/DB error types they return (so you can remove map_err(|e| format!(...)) and the manual Generic(...) conversions); also apply the same change at the other occurrences mentioned (around lines 186-188) to ensure callers can pattern-match the original errors.
🤖 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/identity/add_key_to_identity.rs`:
- Around line 64-79: The None fallback currently constructs TaskError::Generic
with a stringified boxed SdkError, which leaks raw SDK text to the UI; add a
dedicated TaskError variant (e.g., IdentityPublicKeyBroadcastError { #[source]
source_error: SdkError }) to the TaskError enum, implement/adjust Display for
TaskError to return a user-friendly message for that new variant while keeping
the SdkError as the source (so Debug retains technical details), and update the
None arm in add_key_to_identity.rs to return the new
IdentityPublicKeyBroadcastError with the boxed SDK error instead of
format!("Broadcasting error: {boxed}").
---
Duplicate comments:
In `@src/backend_task/identity/add_key_to_identity.rs`:
- Around line 89-101: The code is converting upstream errors into strings
instead of propagating typed TaskError variants; update the error handling so
calls like sdk.get_identity_nonce(...).await and
Identity::fetch_by_identifier(...).await use the ? operator to propagate their
original error types, and add corresponding #[from] variants to the TaskError
enum for the SDK/DB error types they return (so you can remove map_err(|e|
format!(...)) and the manual Generic(...) conversions); also apply the same
change at the other occurrences mentioned (around lines 186-188) to ensure
callers can pattern-match the original errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c38421e-050f-4946-b25b-2e503832ae94
📒 Files selected for processing (2)
src/backend_task/error.rssrc/backend_task/identity/add_key_to_identity.rs
- Convert standalone `broadcast_error()` fn to `impl From<SdkError> for TaskError` (per @lklimek review), moving logic and tests to `error.rs` - Add `BroadcastError` variant with user-friendly Display message instead of leaking raw SDK text via `TaskError::Generic` (per CodeRabbit review) - Change `refresh_identity`, `top_up_identity_from_platform_addresses`, and `transfer_to_addresses` to return `Result<_, TaskError>` instead of `Result<_, String>` (per CodeRabbit review) - Simplify callers in `run_identity_task` — direct `.await` without unnecessary `Ok(self.foo().await?)` wrapping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend_task/identity/refresh_identity.rs (1)
59-69:⚠️ Potential issue | 🟠 MajorReturn the refreshed identity here.
update_local_qualified_identity()persistsqualified_identity_to_update, but the success result still wraps the originalqualified_identity. Any immediate consumer ofBackendTaskSuccessResult::RefreshedIdentitywill see stale status/balance/keys until it reloads from storage.💡 Minimal fix
- Ok(BackendTaskSuccessResult::RefreshedIdentity( - qualified_identity, - )) + Ok(BackendTaskSuccessResult::RefreshedIdentity( + qualified_identity_to_update, + ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/refresh_identity.rs` around lines 59 - 69, The success result currently returns the original qualified_identity while update_local_qualified_identity(&qualified_identity_to_update) persists the updated data; change the return to wrap the updated object (qualified_identity_to_update) so BackendTaskSuccessResult::RefreshedIdentity contains the refreshed identity (ensure you move/clone as needed to satisfy ownership).
♻️ Duplicate comments (1)
src/backend_task/identity/mod.rs (1)
649-655: 🛠️ Refactor suggestion | 🟠 MajorKeep these failures structured instead of flattening them into
Generic.Both closures turn the original error into formatted text, so callers lose source chaining and the user-facing display path goes back to showing backend internals. Prefer a dedicated typed variant or helper that stores the original error as
#[source].As per coding guidelines, "Ensure
Displayproduces user-friendly text forMessageBannerandDebugprovides technical details for logs."Also applies to: 715-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/mod.rs` around lines 649 - 655, Replace the flattening to TaskError::Generic in the mapping closure used around top_up_from_addresses so you preserve the original error as a source; create or use a dedicated TaskError variant (e.g., TaskError::TopUpFromPlatform { msg: String, #[source] source: anyhow::Error } or similar) and return that instead of formatted text, and do the same for the related mapping at the other occurrence (around lines 715-720) so callers can access the original error via source while Display remains user-friendly and Debug retains technical details.
🤖 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/identity/add_key_to_identity.rs`:
- Around line 122-125: The final map_err is converting a typed DB TaskError into
a Generic string; replace the map_err and map chain with direct propagation
using the existing typed TaskError: call
self.update_local_qualified_identity(&qualified_identity)? to let the DB-backed
TaskError propagate, then return
Ok(BackendTaskSuccessResult::AddedKeyToIdentity(fee_result)); this preserves the
TaskError variant and source chain instead of wrapping it in TaskError::Generic.
---
Outside diff comments:
In `@src/backend_task/identity/refresh_identity.rs`:
- Around line 59-69: The success result currently returns the original
qualified_identity while
update_local_qualified_identity(&qualified_identity_to_update) persists the
updated data; change the return to wrap the updated object
(qualified_identity_to_update) so BackendTaskSuccessResult::RefreshedIdentity
contains the refreshed identity (ensure you move/clone as needed to satisfy
ownership).
---
Duplicate comments:
In `@src/backend_task/identity/mod.rs`:
- Around line 649-655: Replace the flattening to TaskError::Generic in the
mapping closure used around top_up_from_addresses so you preserve the original
error as a source; create or use a dedicated TaskError variant (e.g.,
TaskError::TopUpFromPlatform { msg: String, #[source] source: anyhow::Error } or
similar) and return that instead of formatted text, and do the same for the
related mapping at the other occurrence (around lines 715-720) so callers can
access the original error via source while Display remains user-friendly and
Debug retains technical details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b50e454e-98a3-44c5-9082-99c64358fd90
📒 Files selected for processing (4)
src/backend_task/error.rssrc/backend_task/identity/add_key_to_identity.rssrc/backend_task/identity/mod.rssrc/backend_task/identity/refresh_identity.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/backend_task/identity/refresh_identity.rs:69
- The function updates
qualified_identity_to_updatewith the refreshed identity/status, but the success result returns the originalqualified_identityinput, which can be stale. Return the updated identity instead so the result accurately reflects the refresh outcome.
Ok(BackendTaskSuccessResult::RefreshedIdentity(
qualified_identity,
))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dly messages Rename TaskError::BroadcastError to TaskError::SdkError and add sdk_error_user_message() helper that inspects the SDK error variant to produce actionable, user-friendly Display text for MessageBanner. Covers: broadcast rejections, timeouts, stale nodes, DAPI client errors, unavailable servers, cancellation, already-exists, nonce overflow/not-found. Fallback includes the SDK's own Display text for unmatched variants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Define rules for user-facing error text: audience (Everyday User persona), structure (what happened + what to do), tone (calm, direct, no jargon), and where technical details belong (details panel, not the message). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove "contact support" as an acceptable action — users must self-resolve - Add Fluent i18n readiness rule: simple sentences, no fragment concatenation, placeholders only for dynamic values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend_task/error.rs (1)
307-369: LGTM!The tests provide comprehensive coverage:
- All three specific consensus error mappings are tested
- The
StateTransitionBroadcastError.causepath is explicitly tested (line 348-359)- Fallback to
TaskError::SdkErrorfor unrecognized errors is verifiedConsider adding a test for
sdk_error_user_message()output to verify user-friendly messages are generated correctly (e.g., testingTimeoutReachedorStaleNodeproduce expected strings), though this is optional since the pattern is straightforward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/error.rs` around lines 307 - 369, Add a unit test that asserts sdk_error_user_message(...) returns the expected user-friendly string for known SdkError variants (e.g., call sdk_error_user_message(SdkError::TimeoutReached { .. }) and sdk_error_user_message(SdkError::StaleNode { .. }) and assert the returned strings match the expected human messages); place the test alongside the existing tests in src/backend_task/error.rs and reference the sdk_error_user_message function and the SdkError::TimeoutReached / SdkError::StaleNode variants to locate the code. Ensure you construct the SdkError variants with any required fields, call sdk_error_user_message, and use assert_eq! (or assert!(contains)) to validate the exact or substring user-facing message. Make the test names descriptive (e.g., from_sdk_error_user_message_timeout_reached and from_sdk_error_user_message_stale_node) and keep them small and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend_task/error.rs`:
- Around line 307-369: Add a unit test that asserts sdk_error_user_message(...)
returns the expected user-friendly string for known SdkError variants (e.g.,
call sdk_error_user_message(SdkError::TimeoutReached { .. }) and
sdk_error_user_message(SdkError::StaleNode { .. }) and assert the returned
strings match the expected human messages); place the test alongside the
existing tests in src/backend_task/error.rs and reference the
sdk_error_user_message function and the SdkError::TimeoutReached /
SdkError::StaleNode variants to locate the code. Ensure you construct the
SdkError variants with any required fields, call sdk_error_user_message, and use
assert_eq! (or assert!(contains)) to validate the exact or substring user-facing
message. Make the test names descriptive (e.g.,
from_sdk_error_user_message_timeout_reached and
from_sdk_error_user_message_stale_node) and keep them small and deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 229f4599-655d-49e2-bdd1-b73bb829e8e7
📒 Files selected for processing (2)
CLAUDE.mdsrc/backend_task/error.rs
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
- Remove "key hash" jargon from DuplicateIdentityPublicKeyId message
- Include platform/SDK error text as a Fluent-friendly { $error }
placeholder in broadcast rejection and fallback messages
- Remove "review the details" references (not visible in basic mode)
- Never refer users to "contact support" or "details panel"
- Update CLAUDE.md error guidelines accordingly
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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/backend_task/error.rs`:
- Around line 193-235: In the From<SdkError> implementation (where kind is
computed by inspecting consensus_error and matching ConsensusError variants),
add a fallback that checks for a message-only StateTransitionBroadcastError: if
the match for SdkError::StateTransitionBroadcastError yields a broadcast_err
with cause == None, inspect broadcast_err.message (case-insensitive) for
keywords like "duplicate" (and/or "duplicate id" if you want to distinguish) and
map it to the appropriate ConsensusKind (e.g., DuplicateKey or DuplicateKeyId)
before falling through to TaskError::SdkError; update the mapping that converts
ConsensusKind to TaskError variants (DuplicateIdentityPublicKey,
DuplicateIdentityPublicKeyId, IdentityPublicKeyContractBoundsConflict)
accordingly, and add a regression test that constructs a
StateTransitionBroadcastError with cause=None and a message containing
"duplicate" to assert it produces TaskError::DuplicateIdentityPublicKey (or the
chosen variant) instead of TaskError::SdkError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b1892ec-eeeb-4a1c-a9c9-740c4ee876dc
📒 Files selected for processing (2)
CLAUDE.mdsrc/backend_task/error.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ped variants - Clarify rule 4: "technical details" means raw error strings/codes/SDK internals, not user-meaningful identifiers; add exception note pointing to new rule 7. - Add rule 7: Base58 IDs (contract, identity, document) are allowed in user-facing messages — they are opaque-but-copyable handles, not jargon. - Add rule 8: prefer granular TaskError variants with #[source] over TaskError::Generic(format!(...)); Generic is a last resort. - Fix "generic message" wording in Error banners section to avoid confusion with TaskError::Generic. - Add soft guideline: consider moving repeated banner messages into TaskError variants for centralised wording and testability. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ssages - Add TaskError::IdentityNotFoundLocally variant with actionable Display - Fix sdk_error_user_message: remove raw e.message from StateTransitionBroadcastError arm; use fixed, jargon-free string - refresh_identity: bare ? on SDK fetch (From<SdkError> classifies); use IdentityNotFoundLocally; fix channel error to fixed string - add_key_to_identity: bare ? on nonce/identity fetches; use IdentityNotFoundLocally for None identity; fixed string for IdentityUpdateTransition build failure; bare ? + Ok() on DB update - identity/mod.rs: bare ? on top_up_from_addresses and transfer_credits_to_addresses (From<SdkError> handles classification) Addresses review threads 11, 12, 13, 14, 16, 20, 21. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ariant - From<SdkError>: add .or_else() message-based fallback for StateTransitionBroadcastError with cause=None; if message contains "duplicate" (case-insensitive) map to DuplicateIdentityPublicKey — guards against #714 regression when DAPI omits structured cause - Add regression test for cause=None message-only duplicate-key path - Rename misleading test from_sdk_error_unknown_falls_back_to_broadcast_error → from_sdk_error_generic_variant_falls_back_to_sdk_error - Add TaskError::IdentitySaveError { #[source] source: rusqlite::Error } with user-friendly Display; use at both update_local_qualified_identity callsites in top_up and transfer paths (threads 7, 8) - Add TODO(i18n/ux) comment to sdk_error_user_message fallback arm (thread 17) Addresses review threads 7, 8, 9, 17, 19. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let wallet_guard = wallet.read().map_err(|e| e.to_string())?; | ||
| let wallet_guard = wallet | ||
| .read() | ||
| .map_err(|e| TaskError::Generic(e.to_string()))?; |
There was a problem hiding this comment.
wallet.read() poisoning is mapped to TaskError::Generic(e.to_string()), which will likely show a technical lock-poisoning message to the user and also loses the original error type. Convert this into a user-friendly TaskError variant (with a #[source] capturing the poison error) so Display stays actionable while details remain available for debugging.
Suggested change
| .map_err(|e| TaskError::Generic(e.to_string()))?; | |
| .map_err(|e| TaskError::WalletLockPoisoned { source: e })?; |
Contributor
Author
|
Related to #660 |
- Add IdentityUpdateTransitionError variant to preserve ProtocolError from try_from_identity_with_signer (was discarded via |_| Generic) - Add InternalSendError variant to preserve SendError from channel send (was discarded via |_| Generic in refresh_identity) - Wire IdentitySaveError in add_key_to_identity (bare ? produced TaskError::Sqlite with raw rusqlite text instead of friendly message) - Add TODO for LockPoisoned variant in mod.rs wallet.read() path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- IdentityUpdateTransitionError.source_error: Box<dyn Error> → Box<SdkError> (ProtocolError converts via SdkError::Protocol(e)) - InternalSendError: struct variant → unit variant (SendError carries no diagnostic information worth preserving in the chain) - CLAUDE.md rule 8: document source field type conventions — Box<SdkError> for SDK-originated errors, concrete domain type for non-SDK errors, omit #[source] when upstream carries no diagnostic value Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes issue #714 — duplicate key errors from the Dash platform were not classified into user-friendly
TaskErrorvariants, causing raw SDK error text to surface in the UI.StateTransitionBroadcastError { cause: None }could arrive from DAPI with the error signal only in themessagestring; structuredcauseinspection alone missed itFrom<SdkError>— detects"duplicate"in broadcast message whencauseisNoneDuplicateIdentityPublicKey,DuplicateIdentityPublicKeyId,IdentityPublicKeyContractBoundsConflicttyped variants with user-friendlyDisplayand#[source]chainsIdentityNotFoundLocally,IdentitySaveError,IdentityUpdateTransitionError,InternalSendErrortyped variants — replacesTaskError::Generic(format!(...))calls that discarded source errorsIdentityUpdateTransitionErrorpreservesProtocolErrorviaBox<SdkError>(converted viaSdkError::Protocol(e));InternalSendErroris a unit variant (channelSendErrorcarries no diagnostic value)IdentitySaveErrorinadd_key_to_identity(bare?was producing transparentTaskError::Sqlitewith raw rusqlite text)#[source]throughout — technical details stay in collapsible debug panel, not the user messageLockPoisonedvariant inwallet.read()poison pathCLAUDE.mderror message guidelines: Base58 IDs allowed in messages; prefer granular typed variants overGeneric;Box<SdkError>for SDK-originated source fields; i18n-ready message rulesTest plan
cargo test --all-features --workspacepasses (unit tests cover all new variants and the message-based fallback regression)cargo clippy --all-features --all-targets -- -D warningscleanDebugdetails are preserved in the collapsible panel (source chain intact)🤖 Co-authored by Claudius the Magnificent AI Agent