refactor: replace UserInput(String) with typed TaskError variants#751
Conversation
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
UserInput(String) with 31+ typed TaskError variants
| #[error( | ||
| "Could not connect to Dash Core. Please check that Dash Core is running and your settings are correct." | ||
| )] | ||
| CoreRpcClientCreationFailed { detail: String }, |
There was a problem hiding this comment.
@copilot merge CoreRpcClientCreationFailed and RpcProviderCreationFailed into one error
There was a problem hiding this comment.
Done in commit 2301035. CoreRpcClientCreationFailed has been removed and its callsite in context/mod.rs now uses RpcProviderCreationFailed { detail: e.to_string() }.
|
|
||
| /// A context provider could not be bound to the current AppContext. | ||
| #[error("Could not register the network provider. Please restart the application.")] | ||
| ContextProviderBindFailed { detail: String }, |
There was a problem hiding this comment.
@copilot merge ContextProviderBindFailed into SdkInitializationFailed
There was a problem hiding this comment.
Done in commit 2301035. ContextProviderBindFailed has been removed. Both bind_app_context() callsites in context/mod.rs now use SdkInitializationFailed. The doc comment on SdkInitializationFailed was updated to mention it also covers provider binding failures.
|
|
||
| /// The current identity is not an eligible recipient of the token's distribution. | ||
| #[error("{reason}")] | ||
| NotTokenDistributionRecipient { reason: &'static str }, |
There was a problem hiding this comment.
@copilot we don't want to include string reason here. Consider separate variants , with field for context data (allowed recipients, actual recipient, etc.)
There was a problem hiding this comment.
Done in commit 2301035. Replaced NotTokenDistributionRecipient { reason: &'static str } with three typed variants carrying structured context:
NotContractOwner { contract_owner: String }— shows the owner's base58 IDNotDesignatedTokenRecipient { designated_recipient: String }— shows the designated recipient's base58 IDNotEvonode— no context needed (the message is self-explanatory)
| #[error( | ||
| "Could not start the SDK. Please check your network settings and restart the application." | ||
| )] | ||
| SdkInitializationFailed { detail: String }, |
There was a problem hiding this comment.
@copilot review all errors in this file that use detail as string and check if we can just replace it with the source error (rename field to source, annotate #[source], use concrete erorr type). See DpnsFetchError for an example.
There was a problem hiding this comment.
Done in commit 2301035. Reviewed all detail: String fields. Five converted to #[source] with concrete types (following the DpnsFetchError pattern):
AssetLockAddressDerivationFailed→#[source] source: dashcore::address::ErrorAddressConversionFailed→#[source] source: Box<ProtocolError>InvalidRecipientAddress→#[source] source: dashcore::address::Error(keptaddress: Stringfor the user message)AddressNetworkMismatch→#[source] source: dashcore::address::ErrorSighashComputationFailed→#[source] source: dashcore::sighash::Error
The remaining detail: String fields (WalletAddressProviderSetupFailed, SpvClearDataFailed, UtxoUpdateFailed, WalletKeyLookupFailed, WalletAddressDerivationFailed, SdkInitializationFailed, etc.) could not be changed because their upstream functions return String errors — there is no concrete typed error to propagate.
…ient, use #[source] for typed errors Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
UserInput(String) with 31+ typed TaskError variantsUserInput(String) with typed TaskError variants
ab31046
into
task/660-i18n-ready-strings-general-rule
) * 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>
TaskError::UserInput(String)was a catch-all used in ~70 places across 25+ files — effectivelyGeneric(String)renamed, defeating the goal of typed error granularity. This PR deletesUserInputand replaces every callsite with a purpose-specific variant.New
TaskErrorvariantsSdkInitializationFailed(covers provider binding too),RpcProviderCreationFailed(covers Core RPC client creation too),InvalidCoreWalletName,NoCoreWalletsLoadedSpvClearDataFailed,SpvStartFailed,SpvBroadcastFailedUtxoUpdateFailed,AssetLockTransactionBuildFailed,WalletPaymentFailedWalletKeyLookupFailed,WalletAddressDerivationFailedInvalidRecipientAddress,AddressNetworkMismatch,NoUtxosAvailable,InsufficientFunds,OutputTooSmallForFee,SighashComputationFailedNoIdentitiesFound,NotContractOwner,NotDesignatedTokenRecipient,NotEvonodeWalletIdentityNotFound,WalletIdentityKeyMismatch,NoMatchingWalletKeys,WalletKeyDerivationPathNotFound,NoWalletIdentitiesFoundKeyInputValidationFailed,PublicKeyMapBuildFailed,WalletInfoDeterminationFailedNoVotingIdentity,NoDocumentSigningKeyNew
DashPayErrorvariantsMissingAuthenticationKey— replaces string-checked "no suitable authentication key" acrossprofile.rs,contact_info.rs,contact_requests.rsContactRequestAlreadySent { to: String }— replaces string-formatted duplicate-request checkToken distribution recipient variants
NotTokenDistributionRecipient { reason: &'static str }is replaced with three typed variants that carry structured context data:NotContractOwner { contract_owner: String }— includes the contract owner's base58 IDNotDesignatedTokenRecipient { designated_recipient: String }— includes the designated recipient's base58 IDNotEvonode— for evonode-only distributionsdetail: String→#[source]conversionsFive variants now use a
#[source]-annotated concrete error type (following theDpnsFetchErrorpattern) instead of adetail: Stringfield:AssetLockAddressDerivationFailed { #[source] source: dashcore::address::Error }AddressConversionFailed { #[source] source: Box<ProtocolError> }InvalidRecipientAddress { address: String, #[source] source: dashcore::address::Error }AddressNetworkMismatch { #[source] source: dashcore::address::Error }SighashComputationFailed { #[source] source: dashcore::sighash::Error }Remaining
detail: Stringvariants are kept as-is because their upstream functions returnStringerrors.Notable structural fix
The fragile string-prefix match in
load_identity_from_wallet.rsis replaced with a proper variant match:DashPay callsites
DashPay functions now return proper
DashPayErrorvariants (MissingEncryptionKey,MissingDecryptionKey,ValidationFailed,InvalidQrCode,UsernameResolutionFailed, etc.) which auto-convert toTaskErrorviaFrom<DashPayError>.📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.