refactor: remove TaskError::Generic and TaskError::Other, replace with specialized variants#744
Merged
lklimek merged 6 commits intoMar 13, 2026
Conversation
…ized variants
- Remove Generic(String) and Other(Box<dyn Error>) variants from TaskError
- Add SerializationError, IdentifierParsingError, IdentityCreationError,
InvalidPrivateKey, DpnsFetchError, AssetLockNotValidForWallet,
AssetLockInstantLockExpiredNotChainlocked, PlatformFetchError variants
- Update From<String> bridge to map to UserInput instead of Generic
- Replace TaskError::Generic("Identity not found") with IdentityNotFoundLocally
- Replace TaskError::Generic("Contract not found") with DataContractNotFound
- Replace TaskError::Generic(e.to_string()) for bincode with SerializationError
- Fix instant-lock fallback to use Debug repr instead of Display for string matching
- Fix poisoned lock spinning in transaction_processing.rs
- Fix silently dropped wallet locks in load_identity.rs and load_identity_by_dpns_name.rs
- Change contract_token_db::remove_wallet to return TaskError instead of String
- Use typed variants in register_identity.rs and top_up_identity.rs
- Use ? operator for lock errors in core/mod.rs
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
- Fix wallet removal atomicity: acquire write lock before DB removal - Fix proof-log write failures: warn instead of silently dropping - Fix domain jargon in AssetLock* error messages (no "asset lock", "instant lock") - Fix token metadata update atomicity: use upsert instead of remove+insert Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
…dd TODO notes Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Remove TaskError::Generic and TaskError::Other variants
refactor: remove TaskError::Generic and TaskError::Other, replace with specialized variants
Mar 13, 2026
lklimek
reviewed
Mar 13, 2026
| CoreTask::StartDashQT(network, custom_dash_qt, overwrite_dash_conf) => self | ||
| .start_dash_qt(network, custom_dash_qt, overwrite_dash_conf) | ||
| .map_err(|e| TaskError::from(e.to_string())) | ||
| .map_err(|e| TaskError::UserInput(e.to_string())) |
Contributor
There was a problem hiding this comment.
@copilot the task is to define new granular variants in TaskError that will describe each error condition as close as possible. Don't use any catch all of generalized existing variants of they are not a perfect fit for the situation. Apply this comment to whole PR. - check context of change and select or create variant that is close match to the situation, and has the same expected user action. Refer to CLAUDE.md for guidelines.
Contributor
Author
There was a problem hiding this comment.
Applied across the whole PR in b7d5fc6 and c574744:
start_dash_qtio::Error → newDashCoreStartError { source: std::io::Error }(preserves source chain)- Mining network restriction → new
OperationNotAvailableOnNetwork { operation, allowed_networks }(structured fields for the specific condition) From<String>bridge now maps toLegacyError(String)instead ofUserInput—UserInputis reserved exclusively for actual user input validation errors- All other changes in the PR already used granular variants (IdentifierParsingError, InvalidPrivateKey, DpnsFetchError, AssetLockNotValidForWallet, etc.)
…r, OperationNotAvailableOnNetwork, and LegacyError variants Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
…tAvailableOnNetwork display Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
lklimek
merged commit d44ade4
into
task/660-i18n-ready-strings-general-rule
2 checks passed
lklimek
added a commit
that referenced
this pull request
Mar 15, 2026
) * docs: promote i18n-ready strings to general coding rule (#660) Move the i18n guideline out of the error-messages section into general rules, broadening its scope to all user-facing strings (labels, tooltips, messages). Remove direct Fluent reference, fix placeholder syntax to Rust-style `{ name }`, and renumber remaining error rules. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: add typed error variants for migration (#660) Add new TaskError variants (LockPoisoned, Database, CoreRpc, P2P), new domain error types (P2PError, WalletError::KeyDerivation, WalletError::Sighash, DashPayError::QueryCreation, DashPayError::CryptoKeyParsing, DashPayError::PrivateKeyResolution, ConfigError::SaveError), and replace the transparent Sqlite variant with a user-friendly Database variant. Drop Clone/PartialEq from DashPayError to support #[source] fields and fix the two downstream callsites that depended on them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: replace string errors with typed errors in spv/config/infra (#660) - core_p2p_handler: eliminate ReadMessageError(String), use P2PError variants throughout internal methods; keep public API returning String for callers outside scope - config: replace ConfigError::LoadError with SaveError in save() for all I/O operations (writeln, sync_all, persist) - backend_task/mod: add map_err(TaskError::from) bridges for wallet task functions that still return String - model/wallet: fix KeyDerivation source type mismatch (key_wallet::Error != bip32::Error) by reverting to e.to_string() at derivation_path() callsites Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor: replace string errors with typed TaskError in model/database/context (#660) - Use WalletError::AddressError in utxos.rs for dashcore address errors - Use TaskError::from(e) for RPC errors in broadcast_raw_transaction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor: replace string errors with typed TaskError in backend_task/ (#660) Migrate callsites in src/backend_task/ from Result<T, String> to Result<T, TaskError>: - broadcast_state_transition: map SDK error via TaskError::from - contract: direct ? for rusqlite, TaskError::from for SDK errors - document: all 7 document operations migrated - mod.rs: run_wallet_task return type updated to TaskError - register_contract: sender error → InternalSendError, SDK → TaskError::from - update_data_contract: sender error → InternalSendError, format strings → Generic - wallet/*: lock poison → Generic, SDK errors → TaskError::from, ? for rusqlite Build, clippy, and fmt all pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor: replace string errors with typed TaskError in ui/ (#660) Migrate rusqlite error callsites in src/ui/ to typed TaskError variants: - `IdentityTokenInfo::try_from_*` functions in tokens_screen/structs.rs now return `Result<Self, TaskError>` — DB errors become `TaskError::Database` via `?` instead of `.map_err(|e| e.to_string())`. Callers use `MessageBanner::set_global` with the error which accepts `impl Display`, so no caller changes were needed. - `save_private_info` in contact_profile_viewer.rs now returns `Result<(), TaskError>` — the rusqlite error is wrapped explicitly as `TaskError::Database { source: e }` for clear error semantics. All other `map_err(|e| e.to_string())` callsites in src/ui/ were reviewed and left unchanged — they are in UI-local validation functions (clipboard, form validation, address generation) that do not feed into the backend task error system. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: remove useless TaskError::from conversions in wallet task dispatch (#660) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: fix all unresolved review comments — proof errors, asset lock, format! migrations (#740) * Initial plan * refactor: add typed TaskError variants, eliminate Generic from backend_task files Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: eliminate Result<T,String> from context/transaction_processing, add typed TaskError variants Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: migrate context/wallet_lifecycle.rs from Result<T,String> to Result<T,TaskError> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: add source_error to ProofError, add log_drive_proof_error helper, migrate token tasks to TaskError Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: replace TaskError::Internal with granular variants, migrate identity tasks to TaskError Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * style: apply cargo +nightly fmt to fix CI formatting failures Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: fix all unresolved review comments (Claude + lklimek) 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: remove TaskError::Generic and TaskError::Other, replace with specialized variants (#744) * Initial plan * refactor: remove TaskError::Generic and TaskError::Other, add specialized variants - Remove Generic(String) and Other(Box<dyn Error>) variants from TaskError - Add SerializationError, IdentifierParsingError, IdentityCreationError, InvalidPrivateKey, DpnsFetchError, AssetLockNotValidForWallet, AssetLockInstantLockExpiredNotChainlocked, PlatformFetchError variants - Update From<String> bridge to map to UserInput instead of Generic - Replace TaskError::Generic("Identity not found") with IdentityNotFoundLocally - Replace TaskError::Generic("Contract not found") with DataContractNotFound - Replace TaskError::Generic(e.to_string()) for bincode with SerializationError - Fix instant-lock fallback to use Debug repr instead of Display for string matching - Fix poisoned lock spinning in transaction_processing.rs - Fix silently dropped wallet locks in load_identity.rs and load_identity_by_dpns_name.rs - Change contract_token_db::remove_wallet to return TaskError instead of String - Use typed variants in register_identity.rs and top_up_identity.rs - Use ? operator for lock errors in core/mod.rs Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: address remaining PR 739 review comments - Fix wallet removal atomicity: acquire write lock before DB removal - Fix proof-log write failures: warn instead of silently dropping - Fix domain jargon in AssetLock* error messages (no "asset lock", "instant lock") - Fix token metadata update atomicity: use upsert instead of remove+insert Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: address code review comments - improve error messages and add TODO notes Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: replace UserInput catch-all with granular DashCoreStartError, OperationNotAvailableOnNetwork, and LegacyError variants Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: improve DashCoreStartError message and simplify OperationNotAvailableOnNetwork display 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 #739 review comments — structural error matching, lock poisoning cleanup, parameterized RequestType - Parameterize `log_drive_proof_error` with `RequestType` instead of hardcoding `BroadcastStateTransition` (26 callsites across 15 files) - Replace all `map_err` to `LockPoisoned` with `?` operator using blanket `From<PoisonError<T>>` impl; add doc comment explaining the intentional design - Change `CreditCalculationOverflow` variant to carry raw data fields (`amount`, `credits_per_duff`) instead of pre-formatted text - Add `TaskError::AssetLockInstantLockProofInvalid` variant with structural SDK error matching, replacing fragile `format!("{:?}").contains(...)` in register_identity and top_up_identity - Replace `StaleNode` string matching with structural `Error::StaleNode(_)` match in contested names queries - Add `is_unique_constraint_violation()` helper for SQLite error matching in import_mnemonic_screen - Fix wrong error variant: `DocumentNotFound` → `DataContractNotFound` in update_token_config - Add TODO for `BroadcastError(String)` pending `ScreenLike::display_message` trait redesign Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining PR #739 review comments — lock poisoning, error interpolation, StaleNode matching - Remove all remaining explicit `map_err` to `LockPoisoned` across 10 files, using `?` with blanket `From<PoisonError<T>>` impl - Fix `context/mod.rs` sites returning `Result<_, String>` to use `TaskError::from(e).to_string()` instead of discarding resource info - Add `StaleNode` structural matching to `query_dpns_vote_contenders.rs` for consistency with `query_dpns_contested_resources.rs` - Interpolate `operation` and `allowed_networks` fields in `OperationNotAvailableOnNetwork` error message - Narrow `is_unique_constraint_violation` to check `extended_code: 2067` (SQLITE_CONSTRAINT_UNIQUE) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address final PR #739 review comments — jargon removal, dual signal fix - Replace protocol jargon ("asset lock", "instant lock proof", "chain lock proof") with user-friendly language in three TaskError Display messages per CLAUDE.md error guidelines - Fix dual success+error signal in register_contract.rs: return Ok(ContractSavedAfterProofError) instead of sending success via channel then returning Err(ProofError) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: add 6 new TaskError variants for LegacyError removal Add PlatformInfoFetchError, EncryptionError, WalletDatabasePersistError, AvatarProcessingError, MasterKeyNotFound, and TokenQueryError variants to prepare for removing From<String> and LegacyError. Also fix test assertion for updated jargon-free error message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: migrate system/grovestark/mnlist/platform_info to TaskError Replace Result<_, String> with Result<_, TaskError> in four domain dispatcher modules, eliminating reliance on From<String> for TaskError in these paths. - system_task: use ? on rusqlite errors (via #[from]) - grovestark: use ? on GroveSTARKError (via #[from]) - mnlist: replace .unwrap() on RwLock with proper error handling - platform_info: wrap SDK errors in TaskError::SdkError, use TaskError::Generic for document property extraction errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove From<String> for TaskError and LegacyError variant Remove the `impl From<String> for TaskError` bridge and `TaskError::LegacyError(String)` variant. All 45 files that relied on string-to-TaskError conversion now use typed variants. Migration patterns applied: - RwLock/Mutex `.map_err(|e| e.to_string())?` → bare `?` (From<PoisonError>) - rusqlite `.map_err(|e| e.to_string())?` → bare `?` (Database #[from]) - dashcore_rpc errors → bare `?` (From<dashcore_rpc::Error>) - SDK errors → bare `?` (From<SdkError>) - String literals → typed variants (UserInput, WalletNotFound, etc.) - Model/SPV boundary → .map_err(TaskError::UserInput) at callsite New variants used: PlatformInfoFetchError, EncryptionError, WalletDatabasePersistError, AvatarProcessingError, MasterKeyNotFound, TokenQueryError. 45 files changed, 859 insertions(+), 1000 deletions(-) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address 16 review findings across error handling and conventions Error design (error.rs): - Remove dead AvatarProcessingError variant (CODE-004) - Add ContractSchemaMismatch variant for contested index (CODE-006) - Add WithdrawalDocumentParsingError variant for withdrawal parsing (CODE-007) - Route 9 platform_info.rs callsites through From<SdkError> classification (CODE-001) Correctness fixes: - Fix dual signal in update_data_contract: return Ok instead of send+Err (CODE-002) - Return Ok(None) instead of Err(QueryReturnedNoRows) in contract_token_db (CODE-003) P2P module: - Public API returns P2PError instead of String, enabling #[from] conversion (CODE-008) - Add ProtocolError variant for protocol violations instead of synthetic io::Error (CODE-009) Error message quality: - Rewrite ~20 DashPayError Display messages for Everyday User audience (PROJ-001) - Rewrite ConfigError::NoValidConfigs and WalletError::AddressError Display (PROJ-004) - Update CLAUDE.md rule 7: remove stale Generic references (PROJ-002) - Clarify i18n placeholder style in CLAUDE.md (PROJ-005) Mechanical improvements: - Replace explicit TaskError::Database{source:e} with ? via #[from] (CODE-010) - Replace ok_or_else with ok_or for unit-like TaskError variants (CODE-011) - Replace .unwrap() with .unwrap_or_else(|e| e.into_inner()) in identity_db (CODE-017) - Simplify lock poisoning double-conversion in context/mod.rs (CODE-018) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: replace `UserInput(String)` with typed `TaskError` variants (#751) * Initial plan * refactor: replace all UserInput usages with typed TaskError variants Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: improve error message clarity (reviewer feedback) Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: merge RPC/context variants, split NotTokenDistributionRecipient, use #[source] for typed errors 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 review feedback on TaskError migration — jargon, transparent errors, variant cleanup (#753) * Initial plan * refactor: address all review feedback — transparent errors, IdentitySaveError, DpnsFetchError, CLAUDE.md 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> * fix: resolve CI formatting failures (#754) * Initial plan * fix: run cargo +nightly fmt --all to fix CI formatting failures 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> * fix: address 3 PR review findings — error chain, transitional note, vote errors - DashPayError::SdkError: preserve error chain with #[source] Box<dash_sdk::Error> instead of stringifying to reason: String - CLAUDE.md rule 7: add transitional note acknowledging detail: String pattern - vote_on_dpns_name: route SDK errors through TaskError for user-friendly messages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review round 4 — typed errors, dead code, lock safety - vote_on_dpns_name: use ContractSchemaMismatch instead of fake SdkError::Generic - core/mod.rs: migrate 7 payment functions from Result<_, String> to Result<_, TaskError> - load_identity_from_wallet: replace .unwrap() with ? on wallet RwLock operations - Remove dead UiError type (src/ui/error.rs) — zero usages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: strengthen error variant policy — never use String fields for user messages CLAUDE.md rule 7: replace transitional note with strict rule — error variants must not contain String fields that hold user-facing messages. Use #[source] with typed errors, or fieldless variants. Replace existing String-field variants when encountered. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.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.
Eliminates the
Generic(String)andOther(Box<dyn Error>)catch-all variants fromTaskError, replacing all usages with typed error variants. Also addresses all unresolved review comments from PR #739.TaskError variant removal
Generic(String)andOther(Box<dyn Error + Send + Sync>)from the enumFrom<String>bridge now maps toLegacyError(s)as a temporary bridge for ~300 remainingResult<_, String>callsites —UserInputis reserved exclusively for actual user input validation errorsTaskError::GenericNew specialized variants
SerializationError { detail }Generic(e.to_string())for bincodeIdentifierParsingError { input }Generic(format!("Identifier error: ..."))IdentityCreationError { source: Box<ProtocolError> }Generic(format!("Failed to create identity: ..."))InvalidPrivateKey { detail }Generic("Voting private key is not valid")DpnsFetchError { source: Box<SdkError> }Generic(format!("Error fetching DPNS names: ..."))AssetLockNotValidForWalletGeneric("Asset Lock not valid for wallet")AssetLockInstantLockExpiredNotChainlockedfrom("Cannot use this asset lock...")PlatformFetchError { source: Box<SdkError> }from(format!("Failed to fetch address info..."))DashCoreStartError { source: std::io::Error }UserInput(e.to_string())forstart_dash_qtI/O failuresOperationNotAvailableOnNetwork { operation, allowed_networks }UserInput("Mining is only available...")for network restrictionsLegacyError(String)Generic(String)— temporary bridge forFrom<String>callersPR 739 review comment fixes
TaskError::LockPoisonedinstead of silently spinning (transaction_processing.rs) or dropping (load_identity.rs,load_identity_by_dpns_name.rs)Result<(), String>toResult<(), TaskError>(contract_token_db.rs)remove_tokenbeforeinsert_token—insert_tokenalready usesON CONFLICT DO UPDATEupsertlog_drive_proof_errornowtracing::warn!with context instead oflet _ =format!("{:?}", e)(Debug) instead ofe.to_string()(Display), since Display now returns user-friendly text that won't contain raw SDK error stringsAssetLockExpired,AssetLockAddressNotFound,AssetLockTransactionNotFoundInDatabase,AssetLockNoCreditOutputsdisplay messages to avoid "asset lock", "instant lock", "Platform", "Core block"log_drive_proof_errorinstead of inlineformat!()for proof errors inregister_identity_from_platform_addressesOriginal prompt
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.