Skip to content

refactor: preserve typed SdkError in TaskError variants for duplicate-key errors#731

Merged
lklimek merged 5 commits into
fix/714-duplicate-key-errorfrom
copilot/sub-pr-730
Mar 11, 2026
Merged

refactor: preserve typed SdkError in TaskError variants for duplicate-key errors#731
lklimek merged 5 commits into
fix/714-duplicate-key-errorfrom
copilot/sub-pr-730

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 11, 2026

  • Add source_error: String field to typed TaskError variants (previous iteration)
  • Change source_error from String to Box<dash_sdk::Error> (typed API error) with #[source] for proper error chaining
  • Change broadcast_error to take SdkError by value; borrow &error in a block to classify the consensus kind, then move into the boxed variant
  • Update call site: .map_err(|ref e| broadcast_error(e)).map_err(broadcast_error)
  • Update all 5 unit tests to pass sdk_err by value
  • Fix cargo fmt failures: correct import order in error.rs and match arm/assert formatting in add_key_to_identity.rs

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Refactor return type of broadcast_error and add_key_to_identity test: strengthen contract_id assertion in test_contract_bounds_conflict_error Mar 11, 2026
@lklimek
Copy link
Copy Markdown
Contributor

lklimek commented Mar 11, 2026

@copilot what's your progress

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot what's your progress

The work is complete. The reviewer's suggestion was already fully addressed in the base branch by commits 6da02fc (added exact equality check) and ec5e11d (switched to Encoding::Base58 in both production code and the test). No additional code changes were needed in this PR.

@lklimek
Copy link
Copy Markdown
Contributor

lklimek commented Mar 11, 2026

@copilot did you check the code rabbit comment with "Keep the raw SdkError visible when mapping to typed variants".

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot did you check the code rabbit comment with "Keep the raw SdkError visible when mapping to typed variants".

Yes, addressed in 1106f0e. Added tracing::warn!("AddKeyToIdentity broadcast failed: {:?}", error); at the top of broadcast_error() so the full raw SdkError is logged before it gets mapped into a coarse typed TaskError variant.

@lklimek
Copy link
Copy Markdown
Contributor

lklimek commented Mar 11, 2026

@copilot we want it to be included into the task error, add field to a variant or new variant

… variants

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot we want it to be included into the task error, add field to a variant or new variant

Done in 8210e4f. Added source_error: String field (populated with format!("{:?}", error)) to all three variants — DuplicateIdentityPublicKey, DuplicateIdentityPublicKeyId, and IdentityPublicKeyContractBoundsConflict. The raw SdkError is now embedded in the variant itself and visible via Debug, and the intermediate tracing::warn! log was removed since the context is now preserved in the error.

Copilot AI changed the title test: strengthen contract_id assertion in test_contract_bounds_conflict_error refactor: preserve raw SdkError in typed TaskError variants for duplicate-key errors Mar 11, 2026
Comment thread src/backend_task/error.rs Outdated
…nstead of debug string

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Copilot AI changed the title refactor: preserve raw SdkError in typed TaskError variants for duplicate-key errors refactor: preserve typed SdkError in TaskError variants for duplicate-key errors Mar 11, 2026
@lklimek lklimek marked this pull request as ready for review March 11, 2026 19:00
@lklimek
Copy link
Copy Markdown
Contributor

lklimek commented Mar 11, 2026

@copilot clippy job failed (cargo fmt)

…ity.rs

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot clippy job failed (cargo fmt)

Fixed in 15f2941. Two rustfmt issues: import order (SdkError before dashcore_rpc) in error.rs, and match arm / matches! macro formatting in add_key_to_identity.rs.

@lklimek lklimek merged commit 05e781a into fix/714-duplicate-key-error Mar 11, 2026
@lklimek lklimek deleted the copilot/sub-pr-730 branch March 11, 2026 21:58
lklimek added a commit that referenced this pull request Mar 12, 2026
* fix: show user-friendly error for duplicate identity keys (#714)

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>

* fix: correct error messages — duplicate keys are globally unique, not 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>

* fix: remove incorrect "globally unique" claim from error messages

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>

* refactor: return typed TaskError from broadcast_error and add_key_to_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>

* Update src/backend_task/identity/add_key_to_identity.rs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix: use explicit Encoding::Base58 for Identifier::to_string() calls

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>

* refactor: preserve typed SdkError in TaskError variants for duplicate-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>

* refactor: address PR review comments for typed TaskError migration

- 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>

* refactor: replace BroadcastError with SdkError variant and user-friendly 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>

* docs: add error message tone and form guidelines to CLAUDE.md

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>

* docs: refine error message guidelines — no support redirects, i18n-ready

- 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>

* refactor: make error messages i18n-ready and self-contained

- 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>

* docs: refine error message guidelines — Base58 IDs allowed, prefer typed 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>

* refactor: address PR review comments — typed errors and clean user messages

- 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>

* fix: add message-based duplicate-key fallback and IdentitySaveError variant

- 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>

* refactor: preserve source errors in typed TaskError variants

- 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>

* refactor: use concrete SdkError type in error variants; drop dyn Error

- 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>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants