refactor: fix all unresolved review comments — proof errors, asset lock, format! migrations

[Copilot]

Addresses all unresolved review comments from the previous refactor: migrate Result PR. Eliminates remaining format!() error conversions, brittle string matching in UI, and panicking .expect() / .unwrap() calls.

ContractSavedAfterProofError — remove UI string matching

Added BackendTaskSuccessResult::ContractSavedAfterProofError. Backend sends it via channel when a contract is fetched and saved after a DriveProofError. UI screens (register_contract_screen, update_contract_screen) now handle it in display_task_result → BroadcastStatus::Done, replacing the previous fragile message.contains("proof error logged, contract inserted into the database") check.

put_new_identity_to_platform / top_up_identity — use log_drive_proof_error

Replaced all inline DriveProofError match blocks (with manual insert_proof_log_item + format!()) with self.log_drive_proof_error(e). Transition debug info now goes through tracing::debug! rather than being wrapped in a fake Error::Generic payload.

AssetLockExpired — replace format! conversions

All TaskError::from(format!("Cannot use this asset lock yet...")) calls in register_identity.rs and top_up_identity.rs replaced with the structured TaskError::AssetLockExpired { tx_block_height, platform_height } variant.

create_identifier() — replace .expect()

// Before
let identity_id = asset_lock_proof.create_identifier().expect("expected to create an identifier");

// After
let identity_id = asset_lock_proof
    .create_identifier()
    .map_err(|e| TaskError::from(dash_sdk::Error::Protocol(e)))?;

DataContractNotFound — new variant, replaces misused DocumentNotFound

update_token_config was using TaskError::DocumentNotFound for a missing DataContract. Added TaskError::DataContractNotFound and updated the call site.

update_data_contract.rs — migrate 5 remaining format!() conversions

Site	Before	After
Nonce fetch	map_err(|_| "Failed to get nonce".to_string())	? via From<SdkError>
Channel send	map_err(|e| format!("Failed to send message: {}", e))	InternalSendError
Protocol conversion	map_err(|e: ProtocolError| format!(...))	TaskError::from(SdkError::Protocol(e))
Signing	map_err(|e| format!("Failed to sign..."))	TaskError::from(SdkError::Protocol(e))
DB insert	map_err(|e| format!("Error inserting..."))	? via From<rusqlite::Error>

load_identity_by_dpns_name.rs — remove .unwrap() and format!() conversions

wallet.read().unwrap() replaced with filter_map + .read().ok(). Remaining SDK and DPNS format!() errors migrated to TaskError::from(e) / TaskError::IdentityNotFound.

sdk_error_user_message fallback — strip raw SDK error from user-facing text

The wildcard arm no longer embeds {error} (raw SDK Display) in the message shown to users. Returns a static string; full error chain available to developers via #[source] / details panel.

---

📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

[Copilot]

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

[Copilot]

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

[claude]

🔍 Claudius Review — PR #740: Migrate Result<T, String> to Result<T, TaskError>

Verdict: REQUEST CHANGES — 1 CRITICAL regression must be fixed before merge.

The Good

This is a well-executed refactoring. The TaskError enum design is sound, the From<SdkError> classification is thorough with 8 unit tests, and log_drive_proof_error elegantly consolidates a repeated pattern across 14+ callsites. Net -208 lines while adding real type safety — that's the kind of ratio I respect.

The Bad

🚨 CRITICAL — Behavior regression in contract screens (register_contract_screen.rs:329, update_contract_screen.rs:340): Both UI screens match on "proof error logged, contract inserted into the database" to detect contracts registered despite proof errors. This PR replaced that format string with TaskError::ProofError whose Display is completely different. The substring will never match. A successfully registered contract will incorrectly show as failed.

The Ugly

The identity registration/top-up files (register_identity.rs, top_up_identity.rs) were only partially migrated:

• 6+ inline DriveProofError handlers not replaced by log_drive_proof_error
• Asset lock expiration errors use format!() instead of existing TaskError::AssetLockExpired
• State transition Debug repr leaked to user-facing messages via Generic

Summary Statistics

Severity	Count
CRITICAL	1
MEDIUM	8
LOW	6

All 13 previous review threads verified as fixed and resolved.
Full report: review-report/report.json and review-report/report.html.

---

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[claude]

🚨 CRITICAL — Behavior regression: contract screen string-match broken

This change replaced the format string "...proof error logged, contract inserted into the database" with TaskError::ProofError { source_error }, whose Display is now "The operation could not be fully verified by the platform...".

Both register_contract_screen.rs:329 and update_contract_screen.rs:340 use message.contains("proof error logged, contract inserted into the database") to detect a contract registered despite proof error, setting BroadcastStatus::Done. That substring will never match the new error message.

Impact: A contract registered on-chain despite a proof error will incorrectly show as failed. Users may re-submit.

Fix: Update the UI screens to structurally match on the ProofError variant via display_task_result() instead of string-matching in display_message(). The string-matching pattern should be replaced entirely.

_{🤖 Claudius the Magnificent — I found the needle your haystack was hiding.}

[lklimek]

@copilot fix

[claude]

MEDIUM — Inline proof-error handling should use log_drive_proof_error

This file (and top_up_identity.rs) has 6+ inline blocks that manually match Error::DriveProofError, insert a ProofLogItem, and return TaskError::from(format!(...)) — the exact pattern log_drive_proof_error was built to replace. These produce TaskError::Generic with raw error text instead of TaskError::ProofError with the source chain preserved.

Additionally, at lines 541-548, the format string "error: {}, transaction is {:?}" embeds the state transition Debug repr into a user-facing Generic error, which leaks public key hashes and internal identity structure to the UI banner.

Fix: Replace inline DriveProofError handling with .map_err(|e| self.log_drive_proof_error(e)) — same as the token/document tasks.

_{🤖 Claudius the Magnificent}

[lklimek]

@copilot fix

[claude]

MEDIUM — Use TaskError::AssetLockExpired instead of format!()

Six callsites (3 in this file, 3 in top_up_identity.rs) construct asset lock expiration errors via TaskError::from(format!(...)) with jargon terms ("quorum rotated", "instant lock proof has expired", "Platform hasn't verified Core block N"). The dedicated TaskError::AssetLockExpired variant already exists with a proper user-friendly Display message.

Fix: Replace with:

return Err(TaskError::AssetLockExpired {
    tx_block_height,
    platform_height: metadata.core_chain_locked_height,
});

For the chainlock-pending case (line 414), consider adding TaskError::AssetLockChainLockPending with jargon-free text.

_{🤖 Claudius the Magnificent}

[lklimek]

@copilot fix

[claude]

MEDIUM — DocumentNotFound misused for missing DataContract

TaskError::DocumentNotFound says "The document could not be found" — wrong concept for a missing data contract. Same issue at line 92.

Fix: Add TaskError::ContractNotFound with an appropriate Display message, or use TaskError::Generic("The token contract could not be found...".into()) as a quick fix.

_{🤖 Claudius the Magnificent}

[lklimek]

@copilot fix

[claude]

MEDIUM — .expect() at line 228 can panic after asset lock broadcast

asset_lock_proof.create_identifier().expect(...) panics if the proof is malformed. This runs after the asset lock has been broadcast and UTXOs removed — a panic here leaves funds locked on-chain with no identity registration.

Fix: Replace .expect() with .ok_or_else(|| TaskError::Generic("Could not derive identity from asset lock. Please check the transaction and retry.".into()))?

_{🤖 Claudius the Magnificent}

[lklimek]

@copilot fix

[claude]

MEDIUM — sdk_error_user_message fallback (line 311) embeds raw SDK error in user-facing message

The _ catch-all arm at line 311: format!("An unexpected error occurred: {error}. Please try again later.") sends the raw SDK Display text to the UI banner. This may include gRPC endpoint addresses, IP addresses, or protocol details — violating CLAUDE.md Rule 4.

The #[source] chain already preserves the full error for Debug/logs.

Fix: "An unexpected error occurred. Please try again later.".to_string()

_{🤖 Claudius the Magnificent}

[lklimek]

@copilot fix

[claude]

MEDIUM — Five format!() string error conversions not migrated

This method still has five .map_err(|e| format!(...)) calls (lines 75, 83, 88-93, 105-110, 116) that route through From<String> to TaskError::Generic. The identity and token tasks were migrated — this contract task was left behind.

Fix: Map to typed variants: nonce fetch → TaskError::from(sdk_error), sender → TaskError::InternalSendError, protocol error → TaskError::from(SdkError::Protocol(e)), signing → similar, database → ? via From<rusqlite::Error>.

_{🤖 Claudius the Magnificent}

[lklimek]

@copilot fix

[claude]

MEDIUM — .unwrap() on wallet.read() at line 172 — missed migration

wallet.read().unwrap().seed_hash() will panic if the RwLock is poisoned. Every other wallet lock acquisition in this PR uses map_err(TaskError::from). This is the only missed site.

Fix: wallet.read().map_err(TaskError::from)?.seed_hash() or use filter_map with error logging.

_{🤖 Claudius the Magnificent}

[lklimek]

@copilot fix

[lklimek]

@copilot please check all comments and fix them

[Copilot]

@copilot please check all comments and fix them

All previous comment threads are resolved. Fixed the remaining CI failure (Clippy workflow was failing on cargo +nightly fmt --check formatting diffs) in 6df17a7.

Summary of all changes across this PR:

• TaskError::Internal removed; replaced with 8 granular variants
• ProofError carries source_error: Box<SdkError> with #[source]
• All 4 identity task files migrated from Result<_, String> to Result<_, TaskError>
• All 12 token task files migrated to TaskError
• wallet_lifecycle.rs and transaction_processing.rs migrated
• Formatting fixed to pass CI

[lklimek]

@copilot fix

[lklimek]

@copilot fix

[lklimek]

@copilot fix

[lklimek]

@copilot fix

[lklimek]

@copilot fix

[lklimek]

@copilot fix

[lklimek]

@copilot fix