fix: show user-friendly error for duplicate identity keys#729
Merged
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>
Contributor
📝 WalkthroughWalkthroughThese changes address unreadable internal errors when attempting to add duplicate keys to identities. They introduce three new error variants to properly categorize duplicate key scenarios and implement error message mapping logic to convert SDK consensus errors into user-friendly messages, supported by manual test documentation covering five core scenarios and edge cases. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
… 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>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves the UX when adding identity keys by mapping structured Dash SDK broadcast/consensus errors to actionable, user-friendly messages instead of displaying raw/encoded error payloads.
Changes:
- Add
broadcast_error_message()helper to translate specificConsensusError::StateErrorvariants into user-facing messages. - Introduce 3 new
TaskErrorvariants for duplicate-key and contract-bounds conflict scenarios. - Add unit tests for the supported SDK error paths and a manual testing checklist doc.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/backend_task/identity/add_key_to_identity.rs | Adds structured error-to-message mapping for identity key broadcast failures + unit tests. |
| src/backend_task/error.rs | Adds new TaskError variants with user-friendly Display messages. |
| docs/ai-design/2026-03-11-duplicate-key-error/manual-test-scenarios.md | Documents manual verification steps for the new error handling behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 68-70: Update the error string for the
DuplicateIdentityPublicKeyId enum variant in error.rs so it correctly states
that the conflict is a duplicate publicKeys[].id within the same identity rather
than a platform-wide key hash collision; replace the current message in the
#[error(...)] attribute for DuplicateIdentityPublicKeyId with text that tells
the user the key ID is already used in this identity and to choose a unique key
ID.
In `@src/backend_task/identity/add_key_to_identity.rs`:
- Around line 25-53: Change broadcast_error_message to return TaskError (not
String): map the SDK consensus cases to the structured TaskError variants
already used (TaskError::DuplicateIdentityPublicKey,
TaskError::DuplicateIdentityPublicKeyId,
TaskError::IdentityPublicKeyContractBoundsConflict { contract_id: format!("{}",
e.contract_id()) }) and for any other/unhandled broadcast error return a fixed,
non-raw TaskError (e.g. TaskError::Generic("Failed to broadcast state
transition".into()) or your project’s dedicated broadcast-failure variant). Then
update the callers that currently do map_err(format!(...)) to accept/propagate
TaskError (adjust the surrounding function signatures to Result<..., TaskError>
and replace map_err(|e| broadcast_error_message(&e)) or equivalent so the
structured TaskError is propagated instead of a String).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa8ec6a6-e70a-4ef7-9f31-05bb89da9597
📒 Files selected for processing (3)
docs/ai-design/2026-03-11-duplicate-key-error/manual-test-scenarios.mdsrc/backend_task/error.rssrc/backend_task/identity/add_key_to_identity.rs
Comment on lines
+25
to
+53
| fn broadcast_error_message(error: &SdkError) -> String { | ||
| let consensus_error = match error { | ||
| SdkError::StateTransitionBroadcastError(broadcast_err) => broadcast_err.cause.as_ref(), | ||
| SdkError::Protocol(ProtocolError::ConsensusError(ce)) => Some(ce.as_ref()), | ||
| _ => None, | ||
| }; | ||
|
|
||
| if let Some(ce) = consensus_error { | ||
| match ce { | ||
| ConsensusError::StateError(StateError::DuplicatedIdentityPublicKeyStateError(_)) => { | ||
| return TaskError::DuplicateIdentityPublicKey.to_string(); | ||
| } | ||
| ConsensusError::StateError(StateError::DuplicatedIdentityPublicKeyIdStateError(_)) => { | ||
| return TaskError::DuplicateIdentityPublicKeyId.to_string(); | ||
| } | ||
| ConsensusError::StateError( | ||
| StateError::IdentityPublicKeyAlreadyExistsForUniqueContractBoundsError(e), | ||
| ) => { | ||
| return TaskError::IdentityPublicKeyContractBoundsConflict { | ||
| contract_id: format!("{}", e.contract_id()), | ||
| } | ||
| .to_string(); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| format!("Broadcasting error: {}", error) | ||
| } |
Contributor
There was a problem hiding this comment.
Return a typed, sanitized error from this helper.
broadcast_error_message() flattens the new TaskError variants back into String, so this task still crosses the boundary as Result<_, String> and impl From<String> for TaskError will rebuild them as Generic. The same helper also falls back to format!("Broadcasting error: {}", error), which still surfaces raw SDK text for any unhandled broadcast/consensus error. Return TaskError here, keep the structured variants intact, and use a fixed user message for unknown broadcast failures.
💡 Minimal direction
-fn broadcast_error_message(error: &SdkError) -> String {
+fn broadcast_error(error: &SdkError) -> TaskError {
let consensus_error = match error {
SdkError::StateTransitionBroadcastError(broadcast_err) => broadcast_err.cause.as_ref(),
SdkError::Protocol(ProtocolError::ConsensusError(ce)) => Some(ce.as_ref()),
_ => None,
};
if let Some(ce) = consensus_error {
match ce {
ConsensusError::StateError(StateError::DuplicatedIdentityPublicKeyStateError(_)) => {
- return TaskError::DuplicateIdentityPublicKey.to_string();
+ return TaskError::DuplicateIdentityPublicKey;
}
ConsensusError::StateError(StateError::DuplicatedIdentityPublicKeyIdStateError(_)) => {
- return TaskError::DuplicateIdentityPublicKeyId.to_string();
+ return TaskError::DuplicateIdentityPublicKeyId;
}
ConsensusError::StateError(
StateError::IdentityPublicKeyAlreadyExistsForUniqueContractBoundsError(e),
) => {
- return TaskError::IdentityPublicKeyContractBoundsConflict {
- contract_id: format!("{}", e.contract_id()),
- }
- .to_string();
+ return TaskError::IdentityPublicKeyContractBoundsConflict {
+ contract_id: e.contract_id().to_string(),
+ };
}
_ => {}
}
}
- format!("Broadcasting error: {}", error)
+ match error {
+ SdkError::StateTransitionBroadcastError(_)
+ | SdkError::Protocol(ProtocolError::ConsensusError(_)) => {
+ TaskError::Generic("Failed to broadcast the key update. Please try again.".into())
+ }
+ _ => TaskError::Generic(format!("Broadcasting error: {}", error)),
+ }
}
...
- .map_err(|ref e| broadcast_error_message(e))?;
+ .map_err(|e| broadcast_error(&e))?;The surrounding task signature and the other local map_err(format!(...)) sites need the matching TaskError conversion too. As per coding guidelines: src/backend_task/**/*.rs: Backend tasks should return Result<T, TaskError> from src/backend_task/error.rs.
Also applies to: 107-107
🤖 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 25 - 53,
Change broadcast_error_message to return TaskError (not String): map the SDK
consensus cases to the structured TaskError variants already used
(TaskError::DuplicateIdentityPublicKey, TaskError::DuplicateIdentityPublicKeyId,
TaskError::IdentityPublicKeyContractBoundsConflict { contract_id: format!("{}",
e.contract_id()) }) and for any other/unhandled broadcast error return a fixed,
non-raw TaskError (e.g. TaskError::Generic("Failed to broadcast state
transition".into()) or your project’s dedicated broadcast-failure variant). Then
update the callers that currently do map_err(format!(...)) to accept/propagate
TaskError (adjust the surrounding function signatures to Result<..., TaskError>
and replace map_err(|e| broadcast_error_message(&e)) or equivalent so the
structured TaskError is propagated instead of a String).
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
Closes #714
ConsensusError::StateError) — no string-based pattern matchingTaskErrorvariants with actionableDisplaymessagesbroadcast_error()returns typedTaskErrordirectly — no String round-tripadd_key_to_identity()andrun_identity_task()returnResult<..., TaskError>for end-to-end typed error propagationError messages
DuplicatedIdentityPublicKeyStateErrorDuplicatedIdentityPublicKeyIdStateErrorIdentityPublicKeyAlreadyExistsForUniqueContractBoundsErrorSdkErrorNote: 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 — error messages intentionally avoid claiming "all keys must be globally unique".
Test plan
StateTransitionBroadcastErrorpath + generic fallbackcargo clippy --all-features --all-targets -- -D warnings— cleancargo +nightly fmt --all— cleandocs/ai-design/2026-03-11-duplicate-key-error/manual-test-scenarios.md🤖 Co-authored by Claudius the Magnificent AI Agent